Would anyone like to code review my tasks app?

[heenslice]

Hi everyone,

So I spent some time today building a full stack task tracker app using MERN stack. With this app the user will be able to see a list of tasks, add a task, remove a task and toggle a class from complete to incomplete.

I gave it my best effort to make the app as good as I could but I’m not sure whether or not I have followed best practice or if there are some big issues with how I have built the frontend in React.

I haven’t bothered with styling yet as I am mostly focused on learning React logic.

If anyone who is experienced with React would like to take a look I would be extremely grateful!

This is what the API endpoint looks like that I am making requests to:

[  
   {  
      completed:false,
      _id:"5d4f54fe04a5355d4c87d5f4",
      description:"task one",
      __v:0
   },
   {  
      completed:false,
      _id:"5d4f550904a5355d4c87d5f5",
      description:"task two",
      __v:0
   },
   {  
      completed:false,
      _id:"5d4f554504a5355d4c87d5f6",
      description:"task three",
      __v:0
   },
   {  
      completed:false,
      _id:"5d4f554a04a5355d4c87d5f7",
      description:"task four",
      __v:0
   }
]

This is the front-end code:

import React from 'react';
import ReactDOM from 'react-dom';
import axios from 'axios';

class App extends React.Component {
  constructor() {
    super();
    this.handleAddTask = this.handleAddTask.bind(this);
    this.handleDeleteTask = this.handleDeleteTask.bind(this);
    this.handleToggleComplete = this.handleToggleComplete.bind(this);

    this.state = {
      tasks: []
    };
  }

  getTasks() {
    axios.get('http://localhost:3000/tasks')
      .then(response => {
        this.setState({ tasks: response.data });
      })
      .catch(function (error) {
        console.log(error);
      });
  }

  handleAddTask(e) {
    e.preventDefault();
    const newTaskDescription = e.target.elements.task.value.trim();

    if(newTaskDescription.length) {
      axios.post('http://localhost:3000/tasks', {
        description: newTaskDescription
      }).then(() => {
        this.getTasks();
      });

      e.target.elements.task.value = '';
    }
  }

  handleDeleteTask(e) {
    e.preventDefault();
    const taskId = e.target.getAttribute('data-id');

    axios.delete(`http://localhost:3000/tasks/${taskId}`)
      .then(() => {
        this.getTasks();
      })
      .catch(function (error) {
        console.log(error);
      });
  }

  handleToggleComplete(e) {
    e.preventDefault();
    const taskId = e.target.getAttribute('data-id');
    const tasks = this.state.tasks;
    const taskIndex = tasks.findIndex(task => task._id === taskId);
    tasks[taskIndex].completed = !tasks[taskIndex].completed;

    this.setState(() => {
      return { tasks };
    }, () => {
      axios.put(`http://localhost:3000/tasks/${taskId}`, { completed: this.state.tasks[taskIndex].completed})
        .then(() => {
          this.getTasks();
        })
        .catch(function (error) {
          console.log(error);
        });
    });
  }

  componentDidMount() {
    this.getTasks();
  }

  render() {
    return (
      <div>
        <h1>Here is a list of Tasks</h1>
        
        <CreateTask 
          handleAddTask={this.handleAddTask}
        />

        <TaskList
          tasks={this.state.tasks}
          handleDeleteTask={this.handleDeleteTask}
          handleToggleComplete={this.handleToggleComplete}
        />

      </div>
    );
  }
}

const CreateTask = (props) => {
  return (
    <div>
      <form onSubmit={props.handleAddTask}>
        <input type="text" name="task" />
        <button>Add a task</button>
      </form>
    </div>
  );
};

const TaskList = (props) => {
  return (
    <div>
      {props.tasks.length === 0 && <p>Please add a task to get started :)</p>}
      <ul>
        {
          props.tasks.map((task) => {
            return <Task 
              id={task._id}
              key={task._id}
              completed={task.completed}
              description={task.description}
              handleDeleteTask={props.handleDeleteTask}
              handleToggleComplete={props.handleToggleComplete}
            />;
          })
        }
      </ul>
    </div>
  );
};

const Task = (props) => {
  return (
    <li className={props.completed ? 'completed' : ''}>
      {props.description}
      <form
        data-id={props.id}
        onSubmit={props.handleToggleComplete}
      >
        <button>{props.completed ? 'mark as complete' : 'mark as incomplete'}</button>
      </form>

      <form
        data-id={props.id}
        onSubmit={props.handleDeleteTask}
      >
        <button>Delete</button>
      </form>
    </li>
  );
};

As said if there is anything that I am doing that stands out as bad then please let me know.

Here’s a few things I’m unsure about:

1. I am using the getTasks() method every time my a user creates/deletes/edits a post. This works fine at the moment but it seems like quite a lot of API requests. I am considering just updating the dom for these events rather than requesting data from the API. That way I can ‘fake it’ to the users that the page has been updated. If they refresh then the data will be requested so I can’t see a situation where the DOM would be displaying data that is not in the API.
2. The handleToggleComplete method seems extremely complicated for what it is doing. However I couldn’t think of a more concise way to loop through the array, find the correct ID and toggle it.
3. Also related to the handleToggleComplete method, I wasn’t sure whether it was better to give each list item its own state or to do it in the way I did.

Any help or advise is much appreciated

[ito2307]

@heenslice use this for make it more beautiful https://jsonformatter.curiousconcept.com/

[heenslice]

Thanks, I’ve formatted the code. Any tips/suggestions on the coding approach?

[heenslice]

Any more suggestions to improve this?

[RandellDawson]

Any reason you are not using fetch instead of adding the axios library?

[heenslice]

Thanks for looking at this for me.

No reason other than it is the way I learned to make AJAX requests was using axios. I could use fetch if you think it’s better.

What would the advantage be?

[RandellDawson]

What would the advantage be?

fetch is not a library. It is built-in to JavaScript all modern browsers support it.

[heenslice]

import React from 'react';
import ReactDOM from 'react-dom';

// Styles
import './styles/styles.scss';

// Global Variables
const dbURL = 'http://localhost:4000/tasks';

const CreateTask = (props) => {
  return (
    <div>
      <form onSubmit={props.handleAddTask}>
        <input type="text" name="task" />
        <button>Add a task</button>
      </form>
    </div>
  );
};

const TaskList = (props) => {
  return (
    <div>
      {props.tasks.length === 0 && <p>Please add a task to get started :)</p>}
      <ul>
        {
          props.tasks.map((task) => {
            return <Task
              id={task._id}
              key={task._id}
              completed={task.completed}
              description={task.description}
              handleDeleteTask={props.handleDeleteTask}
              handleToggleComplete={props.handleToggleComplete}
            />;
          })
        }
      </ul>
    </div>
  );
};

const Task = (props) => {
  return (
    <li className={props.completed ? 'completed' : ''}>
      {props.description}
      <form
        data-id={props.id}
        onSubmit={props.handleToggleComplete}
      >
        <button>{props.completed ? 'mark as complete' : 'mark as incomplete'}</button>
      </form>

      <form
        data-id={props.id}
        onSubmit={props.handleDeleteTask}
      >
        <button>Delete</button>
      </form>
    </li>
  );
};

class App extends React.Component {
  constructor() {
    super();

    this.handleAddTask = this.handleAddTask.bind(this);
    this.handleDeleteTask = this.handleDeleteTask.bind(this);
    this.handleToggleComplete = this.handleToggleComplete.bind(this);

    this.state = {
      tasks: []
    };
  }

  async getTasks() {
    try {
      const response = await fetch(`${dbURL}`);
      const data = await response.json();

      this.setState({ tasks: data });
    } catch(err) {
      console.log(err);
    }
  }

  componentDidMount() {
    this.getTasks();
  }

  async handleAddTask(e) {
    e.preventDefault();
    const newTaskDescription = e.target.elements.task.value.trim();

    if(newTaskDescription.length) {
      e.target.elements.task.value = '';

      try {
        const postOptions = {
          body: JSON.stringify({description: newTaskDescription}),
          method: 'POST',
          headers: {
            'Content-Type': 'application/json'
          }
        };

        await fetch(`${ dbURL }`, postOptions);
        this.getTasks();
      } catch(err) {
        console.log(err);
      }
    }
  }

  async handleDeleteTask(e) {
    e.preventDefault();
    const deleteOptions = {
      method: 'DELETE',
      headers: { 'Content-Type': 'application/json' }
    };

    try {
      const taskId = e.target.getAttribute('data-id');
      await fetch(`${dbURL}/${taskId}`, deleteOptions);

      this.getTasks();
    } catch(err) {
      console.log(err);
    }
  }

  async handleToggleComplete(e) {
    e.preventDefault();
    const taskId = e.target.getAttribute('data-id');
    const tasks = this.state.tasks;
    const taskIndex = tasks.findIndex(task => task._id === taskId);
    tasks[taskIndex].completed = !tasks[taskIndex].completed;

    const putOptions = {
      body: JSON.stringify({ completed: this.state.tasks[taskIndex].completed }),
      method: 'PUT',
      headers: {
        'Content-Type': 'application/json'
      }
    };

    this.setState(() => {
      return { tasks };
    }, async () => {
      await fetch(`${dbURL}/${taskId}`, putOptions);

      try {
        this.getTasks();
      } catch(err) {
        console.log(err);
      }
    });
  }

  render() {
    return (
      <div>
        <h1>Here is a list of Tasks</h1>

        <CreateTask 
          handleAddTask={this.handleAddTask}
        />

        <TaskList
          tasks={this.state.tasks}
          handleDeleteTask={this.handleDeleteTask}
          handleToggleComplete={this.handleToggleComplete}
        />

      </div>
    );
  }
}

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

Here’s a version of the app that has been changed to use the fetch api instead. I have also used async/await

[RandellDawson]

One issue I see is anytime you make fetch call (get, put, delete, etc.), you have a catch which displays the error to the console, but you give no warning to the user that something went wrong. A user wouldn’t know immediately if the the update or the get to retrieve the records failed.

The other thing I notice is you call this.getTasks() after the post, put, or delete. There really is no reason to do this, because if you make a successful post, put, or delete, you should be sending back some kind of “success” response along with the data you just added, updated, or deleted. Use that to update state instead of making yet another call to the database.

[heenslice]

Apologies it took so long to work on this but I’ve had a super busy week. I’ve created a git repo and actioned a the changes you suggested.

heenslice/MERN-stack-Tasks-App