3
\$\begingroup\$

I am learning React and wanted to implement a todo app using function components. Any suggestions and criticism is welcome, especially regarding best practices.

import React, {useState} from 'react';

class Todo {
  constructor(id, description, complete) {
    this.id = id;
    this.description = description;
    this.complete = complete;
  }
}

export default () => {

  const [idCounter, setIdCounter] = useState(1);
  const [todoList, setTodoList] = useState([]);
  const [todoDescription, setTodoDescription] = useState('');

  function addTodo() {
    setTodoList(
      [...todoList, new Todo(idCounter, todoDescription, false)]
    );
    setIdCounter(idCounter + 1);
    setTodoDescription('');
  }

  function markTodoItemComplete(id) {
    const todoListClone = todoList.slice();
    todoListClone[id - 1].complete = true;
    setTodoList(todoListClone);
  }

  return <>
    {todoList.map((todoItem) => {
      return (
        <React.Fragment key={todoItem.id}>
          <span>{todoItem.id} - {todoItem.description} - {todoItem.complete ? 'Completed' : 'Waiting'}</span>
          <button
            onClick={() => markTodoItemComplete(todoItem.id)}
            disabled={todoItem.complete}>
            Mark as complete
          </button>
          <br/>
        </React.Fragment>
      );
    })}
    <br/>
    <input
      onChange={e => setTodoDescription(e.target.value)}
      value={todoDescription}
    />
    <br/>
    <button
      disabled={!todoDescription}
      onClick={addTodo}>Add Todo
    </button>
  </>
}

enter image description here

\$\endgroup\$
1
\$\begingroup\$
 function markTodoItemComplete(id) {
    const todoListClone = todoList.slice();
    todoListClone[id - 1].complete = true;
    setTodoList(todoListClone);
  }

This seems me not good practice, what you could have done is just getting id(key) of array and just set it to true. like

 function markTodoItemComplete(id) {
    todoListClone[id].complete = true;
  }
\$\endgroup\$
4
  • \$\begingroup\$ I strongly support not to clone here. Please motivate changing the index expression. \$\endgroup\$
    – greybeard
    Feb 13 '20 at 8:26
  • \$\begingroup\$ is cloning here a crime @greybeard?? \$\endgroup\$ Feb 13 '20 at 8:55
  • \$\begingroup\$ (Cloning in itself is not a crime. It can be a useful tool and prevent unwanted consequences. It can introduce unwanted consequences, starting with unwarranted processing effort. Why do you ask? I was under the impression that we agree cloning in markTodoItemComplete() is not a good idea. I have second thought noticing that in your second code block you kept the (undeclared/undefined) name todoListClone.) \$\endgroup\$
    – greybeard
    Feb 13 '20 at 9:09
  • \$\begingroup\$ todoListClone[id].complete = true; will not update the view. \$\endgroup\$ Feb 20 '20 at 14:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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