Would anyone like to code review my tasks app?

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 :slight_smile:

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

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

Any more suggestions to improve this?

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?

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

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. :+1: