current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

Would this todo list benefit from breaking it down into more components? What would be the advantages? Am I breaking any best practices from what is seen here?

var TodoList = React.createClass({
    getInitialState: function(){
        return {
            todos:data,
            text:""
        };
    },
    handleChange: function(event){
        this.setState({text:event.target.value });
    },
    submitHandler: function(event){
        event.preventDefault();
        var todo = this.state.text;
        data.push({task:todo});
        this.setState({text:""});
        $('#input').val("")
    },
    render: function(){
        var rows = [];
        this.state.todos.forEach( function(todo, index){
            rows.push(<li key={index}>{todo.task}</li>)
        }); 

        return(
            <div className="well clearfix">
                <form onSubmit={this.submitHandler}>
                    <ul>
                        {rows}
                    </ul>
                    <br />
                    <input id="input" key={this.state.timestamp} onChange={this.handleChange} type="text" />
                    <br />
                    <br />
                    <button type="submit" className="btn btn-primary" disabled={this.state.text.length === 0}>Submit</button>
                </form>
            </div>
        )
    }
});

var data = [
    {task:"eat cheese"},
    {task:"go to gym"},
    {task:"do some yoga"},
    {task:"go to sleep"}
];

ReactDOM.render(<TodoList/>, document.getElementById('app') );
share|improve this question
up vote 1 down vote accepted

I think data should just be contained in the component state, because it belongs to the TodoList:

getInitialState: function() {
    return {
        todos: [{task: "eat cheese"}, ...],
        text: ""
    };
},

which would simplify submitHandler into

submitHandler: function(event) {
    event.preventDefault();
    var todo = this.state.text;
    this.setState({
        text: "",
        todos: this.state.todos.concat([todo])
    });
},

The jQuery $('#input').val("") call is unnecessary because React should be handling that state for us. Just make the value of the <input> reflect state.text and all will flow:

<input id="input" key={this.state.timestamp} onChange={this.handleChange}
       value={this.state.text} type="text" />

In render(), the declaration of rows and the forEach can be combined into a map:

var rows = this.state.todos.map(function(todo, index) {
    return (<li key={index}>{todo.task}</li>);
});

--

In general your code is very clear. Right now your app is simple and I don't think there's much need to break it down into smaller parts.

However, as your app inevitably grows and you add more features, separating, say, TodoItem and TodoList into distinct components would be useful. When a function gets long and starts doing five or six conceptually distinct things, you separate it into smaller functions and compose them together. Same thing when classes start representing many things at once.

So some advantages of breaking things down into smaller components are modularity and composability. Your app becomes easier to test and think about.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.