Simple checkboxes with React

Question

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 (
            <div>
                <form>
                  {Object.keys(this.state.data).map((key, index) =>
                        <div key={index}>
                            <input
                                name={key}
                                id={index}
                                type="checkbox"
                                checked={this.state.data[key].value}
                                onChange={(event) => this.handleClick(event)} />

                            <span className="padding">
                                <label htmlFor={index} key={index}>
                                    {key}
                                </label>
                            </span>
                        </div>
                    )}
                </form>
                <div className="center">
                    <h2>Current State: </h2>
                    <ul>
                        {Object.keys(this.state.data).map((key, index) =>
                            <li key={index}>{ `${key}:  ${this.state.data[key]}`}</li>
                        )}
                    </ul>
                </div>
            </div>
        );
    }
}
ReactDOM.render(<App />, document.getElementById('app'));

Answer 1

Main Question

Can somebody please tell me what would have made my code more "correct"?

It is difficult to know exactly what would have made my code more "correct" but it does look like the method handleClick is more complex than it needs to be. Specifically, there is a for loop that is not needed. Perhaps you were attempting to guard against cases where name is not actually a property on data. That could be achieved using Object.hasOwnProperty() - for example, the for loop could be replaced with this block of code:

if (data.hasOwnProperty(name)) {
    data[name] = value;
}

Or perhaps even simpler would be using the in operator. For an explanation of the differences, refer to this answer on SO.

if (name in data) {
    data[name] = value;
}

Useless re-assignment of data in handleClick method

At the beginning of the handleClick method, I see this line:

let data = this.state.data;

And then after storing constants for the event target and its name, there is this line:

data = this.state.data;

That line seems to serve no purpose.

Input type other than checkbox?

Are there other types of inputs besides checkboxes? If not, then the following line in handleClick() seems too complicated:

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

There appear to be no value attributes set on any elements.

Binding the method to the event handler

The handleClick method is called from an arrow function in the onChange event handler attribute:

onChange={(event) => this.handleClick(event)}

According to the ReactJS documentation:

The problem with this syntax is that a different callback is created each time the [component] renders. In most cases, this is fine. However, if this callback is passed as a prop to lower components, those components might do an extra re-rendering. We generally recommend binding in the constructor or using the class fields syntax, to avoid this sort of performance problem.¹

There is no need to add an extra anonymous function just to call that method. Bind the method directly, like below:

onChange={this.handleClick}

Checked attribute

The checked attribute of each checkbox input is set according to the following:

checked={this.state.data[key].value}

But there is no need to use .value - it can be simplified like below:

checked={this.state.data[key]}

---

¹_{https://reactjs.org/docs/handling-events.html}

Answer 2

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 (
      <div>
        <form>
          {Object.keys(this.state.data).map((key) => (
            <div key={key}>
              <input
                name={key}
                id={`checkbox-${key}`}
                type="checkbox"
                checked={this.state.data[key]}
                onChange={() => this.setState({ [key]: !this.state[key] })}
              />

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

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 <input>.