To-Do List in React

Question

I've written a to-do list app in React, as I am a beginner and this is my first app, I feel it can be improved. Some assistance in improving it, and how or why improvements are better would be much appreciated.

Here is a working demo of it.

var ToDoContainer = React.createClass({
    getInitialState: function(){
        var tasks = [
                {task:"Go to the gym", completed:false, id:1 }, 
                {task:"Do yoga", completed:false, id:2 },
                {task:"Buy groceries", completed:true, id:3 },
                {task:"Get tire fixed", completed:true, id:4}
        ];

        return {
            tasks:tasks,
            numCompleted:null
        }
    },
    addTask: function(task){
        this.setState({
            tasks:this.state.tasks.concat(task) 
        }); 
    },
    clearComplete: function(){

        var remainingTasks = this.state.tasks.filter(function(task){
            if (!task.completed) return task;
        });

        this.setState({
            tasks:remainingTasks
        });

    },
    markComplete: function(e){
        console.log('markComplete')
        var id = e.target.getAttribute('data-id');
        var tempTasks = this.state.tasks;

        tempTasks.forEach(function(task){
            if (id == task.id) {
                if (task.completed){
                    task.completed = false; 
                }
                else if (!task.completed) {
                    task.completed = true;  
                }
            }
        })

        this.setState({
            tasks:tempTasks
        });
    },
    render: function(){
        var numCompleted = this.state.tasks.filter(function(task){
            return task.completed;
        }).length

        var numRemaining = this.state.tasks.filter(function(task){
            return !task.completed;
        }).length

        return (
            <div className="rowFluid">
                <div className="well row col-md-4 col-md-offset-4">
                    <h3 className="text-left">You have {numRemaining} tasks to do:</h3>
                    <TaskList tasks={this.state.tasks} check={this.markComplete} />
                    <AddTask addNew={this.addTask} />
                    <ClearTask clear={this.clearComplete} numCompleted={numCompleted}/>
                </div>
            </div>
        )
    }
});

var TaskList = React.createClass({
    handleCheck: function(e){
        console.log('handleCheck')
        this.props.check(e);
    },
    getInitialState: function(){
        return {
            data:this.props.tasks
        }
    },
    render: function(){
        var that = this;

        var task = this.props.tasks.map(function(task, index){
                return <li key={index} className={task.completed ? "complete" : "incomplete"} >
                            <input type="checkbox" checked={task.completed} onChange={that.handleCheck} data-id={task.id}/>
                            {task.task}
                        </li>
        });

        return (
            <div>
                <ul className="list-unstyled">
                    {task}
                </ul>
            </div>
        )
    }
});

var AddTask = React.createClass({
    getInitialState: function(){
        return{
            newTask:""
        }
    },
    handleChange: function(e){
        this.setState({
            newTask:e.target.value
        });
    },
    handleSubmit: function(e){
        e.preventDefault();

        this.props.addNew({task:this.state.newTask, completed:false, id:Date.now()});
        this.setState({
            newTask:""
        });
    },
    render: function(){
        var inputStyle = {
            marginRight: "10px"
        }

        return (    
            <div className="text-left">
                <form onSubmit={this.handleSubmit}>
                    <input type="text" value={this.state.newTask} onChange={this.handleChange} style={inputStyle}/>
                    <input type="submit" value="Add Task" disabled={this.state.newTask == ""} />
                </form>
            </div>

        )
    }
});

var ClearTask = React.createClass({

    clear: function(){
        this.props.clear();
    },
    render: function(){
        var buttonStyle = {
            marginTop: "10px"
        }

        return (
            <button onClick={this.clear} value="Clear Completed" clear={this.clearComplete} disabled={this.props.numCompleted==0} className="text-center" style={buttonStyle}>Clear Completed</button>
        )   
    }
})


ReactDOM.render(<ToDoContainer />, document.getElementById('app'));

Community · Accepted Answer · 2017-05-23 12:40:49Z

return {
    tasks:tasks,
    numCompleted:null
}

numCompleted isn't being used anywhere. Remove if from the data. Also, it's a "computed property", one you can compute off from tasks. I notice you compute it on render. Consider putting it in a function instead.

getCompletedTaskCount: function(){
  return this.state.tasks.filter(task => task.completed).length;
},
getNotCompletedTaskCount: function(){
  return this.state.tasks.filter(task => !task.completed).length;
},
clearComplete: function(){
    this.setState({
        tasks: this.state.tasks.filter(task => !task.completed);
    });
},

Since you're using React, I assume you use Babel, and thus we have access to ES6 transpilation. ES6 provides new syntax to simplify your code. In the example above, I use arrow functions and their ability to implicitly return the expression value.

Also, I notice that you count the complete and incomplete tasks on render. I suggest you move these logic into functions so that they can be reused. It also makes them visible, and reduces clutter in the render function.

markComplete: function(e){
    console.log('markComplete')
    var id = e.target.getAttribute('data-id');
    var tempTasks = this.state.tasks;

    tempTasks.forEach(function(task){
        if (id == task.id) {
            if (task.completed){
                task.completed = false; 
            }
            else if (!task.completed) {
                task.completed = true;  
            }
        }
    })

    this.setState({
        tasks:tempTasks
    });
},

I suggest against console.log for debugging. The last thing you'd ever want to make a habit of is forgetting console calls in your code. I suggest you learn using breakpoints in dev tools. Much more powerful and doesn't leave trash in the code.

markComplete is misleading. It's not really just marking the tasks complete. It's toggling the complete state. You should name it appropriately, like toggleCompleteness.

return <li key={index} className={task.completed ? "complete" : "incomplete"} >

The problem here is your CSS classes. complete and incomplete are very generic. It could possibly be killed by other libraries in your app that use those very same names. With that, you'd start to think about namespacing, and your CSS tends to end up like:

.todo-list .task .complete{...}

The effect is that this style has a specificity of 0-3-0 (0 ids, 3 classes, 0 elements). To override it, you'll need 1-x-x (an id) or 0-4-0 (4 classes) or a 0-3-0 (3 classes, but defined after). This becomes a headache as the app grows larger, and make your CSS life a battle of overrides.

To counter this problem, I suggest you read about BEM. In a gist, it's a naming convention for CSS classes that allow you to build low-specificity, precisely targeted selectors that are easily identifiable. You don't even need to override if you can safely say this style only affects this component and nothing else.

this.props.addNew({task:this.state.newTask, completed:false, id:Date.now()});

The completed and id should be filled in by the addTask in the very top-most component, and that addNew should only be called with newTask. This is to isolate the knowledge of the data structure to just one location in the code.

A relevant scenario would be when your data structure changes. If you wrote it this way, you'd be hunting for hard-coded data structures. Whereas if you just isolated it in one place, you'd only change that portion of the code.

Also, don't use Date.now() as an ID. Generate GUIDs instead. Here's an handy implementation in JS. If you're just after a quick way to identify things, I personally use ('' + Math.random()).slice(2) for a quick ID solution as Date.now() can yield the same value if you run it in rapid succession (like in loops).

<div className="text-left">
    <form onSubmit={this.handleSubmit}>
        <input type="text" value={this.state.newTask} onChange={this.handleChange} style={inputStyle}/>
        <input type="submit" value="Add Task" disabled={this.state.newTask == ""} />
    </form>
</div>

A <form> is overkill here, as your handler doesn't use anything about the form aside from hooking to the onSubmit event. You can remove the <form>, replace the input button input with a <button> and put an onClick on it that calls handleSubmit.

var buttonStyle = {
    marginTop: "10px"
}

Another reason to use BEM convention is that inline styles are higher in specificity than IDs, and the only way to override them is to replace the inline style or use !important in a stylesheet. Again, you don't want your CSS to be a battle of overrides.

This was very thorough. Thank-you for taking the time to respond. — asdf
– asdf, Commented Jan 8, 2016 at 6:58

BenC · Accepted Answer · 2016-01-08 02:03:49Z

I can't offer a full review for you right now, but here are a couple little things:

A couple simplifications

In ToDoContainer.render, this:

var numCompleted = this.state.tasks.filter(function(task){
    return task.completed;
}).length

var numRemaining = this.state.tasks.filter(function(task){
    return !task.completed;
}).length

is doing extra work. numRemaining is just this.state.tasks.length - numCompleted, no need to do a second filter.

Also, in ToDoContainer.clearComplete, this:

var remainingTasks = this.state.tasks.filter(function(task){
    if (!task.completed) return task;
});

can be simplified slightly into

var remainingTasks = this.state.tasks.filter(function(task) {
    return !task.completed;
});

because filter only cares about the true/false-y result of your function.

Toggling a boolean

In ToDoContainer.markComplete, this:

if (task.completed){
    task.completed = false; 
}
else if (!task.completed) {
    task.completed = true;  
}

can be simplified into

task.completed = !task.completed;

which is a common idiom for toggling a boolean.

\$\begingroup\$ This will help to make my code more concise. Thank-you. \$\endgroup\$

asdf
– asdf

2016-01-08 07:02:52 +00:00
Commented Jan 8, 2016 at 7:02 — asdf
– asdf, Commented Jan 8, 2016 at 7:02

Stack Exchange Network

To-Do List in React

2 Answers 2

A couple simplifications

Toggling a boolean

You must log in to answer this question.

Hot Network Questions

To-Do List in React

2 Answers 2

A couple simplifications

Toggling a boolean

You must log in to answer this question.

Related

Hot Network Questions