Simple checkboxes with React I was given the following exercise as part of a job interview process. I submitted my code and received the feedback that my solution is "close" but not "correct". My code executes just fine and displays the desired results. Can somebody please tell me what would have made my code more "correct"? or - did I misunderstand the requirements? (I realize that the Constructor is no longer needed but Codepen was complaining without it).

Requirements:

Using data variable implement a small Form using checkboxes

[x] Sonia
[ ] Maria
[ ] John
[x] Michael


On every form change on any checkbox print its current state, which should look like data.

const data = {
  sonia: true,
  maria: false,
  john: false,
  michael: true
};


My solution:

class App extends React.Component{

    constructor() {
        super();

        this.state = {
            data: {
                Sonia: false,
                Maria: false,
                John: false,
                Michael: false
            }
        };

        this.handleClick = this.handleClick.bind(this);
    }

    handleClick(event) {

        let data = this.state.data;

        const target = event.target;
        const value = target.type === 'checkbox' ? target.checked : target.value;
        const name = target.name;

        data = this.state.data;

        for (const i in data) {
            if (i === name) {
                data[i] = value
            }
        }
        this.setState( {data:data});
    }

    render() {
        return (
            &lt;div&gt;
                &lt;form&gt;
                  {Object.keys(this.state.data).map((key, index) =&gt;
                        &lt;div key={index}&gt;
                            &lt;input
                                name={key}
                                id={index}
                                type="checkbox"
                                checked={this.state.data[key].value}
                                onChange={(event) =&gt; this.handleClick(event)} /&gt;

                            &lt;span className="padding"&gt;
                                &lt;label htmlFor={index} key={index}&gt;
                                    {key}
                                &lt;/label&gt;
                            &lt;/span&gt;
                        &lt;/div&gt;
                    )}
                &lt;/form&gt;
                &lt;div className="center"&gt;
                    &lt;h2&gt;Current State: &lt;/h2&gt;
                    &lt;ul&gt;
                        {Object.keys(this.state.data).map((key, index) =&gt;
                            &lt;li key={index}&gt;{ `${key}:  ${this.state.data[key]}`}&lt;/li&gt;
                        )}
                    &lt;/ul&gt;
                &lt;/div&gt;
            &lt;/div&gt;
        );
    }
}
ReactDOM.render(&lt;App /&gt;, document.getElementById('app'));
I think everything Sam Onela said is great. I'm gonna add a few more things.



Don't modify your state directly

In general, it's considered bad practice to directly modify your state object. According to the react docs:


  Never mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were immutable.


Instead you should either


Pass setState an object with only what you want to change (I chose this)
Clone your state, modify the clone, then pass it to setState


Stop using index like that

Using index in your loops introduces another variable you don't need, and actually makes things slightly less correct.

Don't use index for key prop

The key prop should be a unique key to identify that element.
The most unique things we have about each of these checkboxes is the name that goes with them.
If we rearranged this list, we would want the same names to stay checked, not the same indexes in the list.

Don't use index as an id

At least not directly.
You want your ids to be unique on the page, and using the ids 1, 2, 3, etc. seems like not a good way to be unique. It would be better to use something like ${checkbox}-${key} so we get "checkbox-Sonia", "checkbox-Maria", etc.

It also used to be the case that ids needed to start with a letter and not a number, but I believe this is no longer the case.

You don't need an extra data layer in your state

It just adds needless comlexity. Just put the checkbox values right in your state, and only move them when you need to do more complex things with your state.

this.state = {
  Sonia: false,
  Maria: false,
  John: false,
  Michael: false
};


handleClick can be way simpler

One line in fact:

handleClick(key) {
    this.setState({ [key]: !this.state[key] });
}


Heck, it's so small that I would probably even put it inline, but different people might disagree on that.

Revised Version

Here's what my revised version of the code would look like:

class App extends React.Component {
  constructor(props) {
    super(props);
    // I guess you don't have props, so it doesn't really matter,
    // but you need to pass them to `super()` if you want them
    // to be available on `this.props`

    // Doesn't need the extra `data` layer
    this.state = {
      Sonia: false,
      Maria: false,
      John: false,
      Michael: false
    };
  }

  // handleClick was inlined

  render() {
    return (
      &lt;div&gt;
        &lt;form&gt;
          {Object.keys(this.state.data).map((key) =&gt; (
            &lt;div key={key}&gt;
              &lt;input
                name={key}
                id={`checkbox-${key}`}
                type="checkbox"
                checked={this.state.data[key]}
                onChange={() =&gt; this.setState({ [key]: !this.state[key] })}
              /&gt;

              &lt;span className="padding"&gt;
                {/* Don't need a `key` prop here */}
                &lt;label htmlFor={`checkbox-${key}`}&gt;
                  {key}
                &lt;/label&gt;
              &lt;/span&gt;
            &lt;/div&gt;
          ))}
        &lt;/form&gt;
        &lt;div className="center"&gt;
          &lt;h2&gt;Current State: &lt;/h2&gt;
          &lt;ul&gt;
            {Object.keys(this.state).map(key =&gt; (
              &lt;li key={key}&gt;{`${key}: ${this.state.data[key]}`}&lt;/li&gt;
            ))}
          &lt;/ul&gt;
        &lt;/div&gt;
      &lt;/div&gt;
    );
  }
}


Note that this does still use an arrow function and not a bound method.
I think that this lets to code be much more readable, and the possible performance loss wouldn't be realized until you have components that implement PureComponent deeper in the call chain receiving this function (currently it goes directly to an &lt;input&gt;.