fac26 / code-reviews

0 stars 3 forks source link

Code Review for Dom and Iman #10

Open Psydwinder opened 1 year ago

Psydwinder commented 1 year ago

README

Project description There is a great introduction to WHAT the project is along with WHY it is being created and answers any initial questions the person viewing the repo might have.

It might be worth adding in a section about learning outcomes to solidify the WHY aspect of this project as well as giving an insight into what skills you have learnt in the process.

There are visual representations of HOW the APP is supposed to function, and that is an awesome idea as it helps show that you planned ahead and worked from that template when creating the actual app.

User stories

From the README there are some user stories that do not work as intended when testing the app. For example the first user story in the README says that the user should be able to start with an empty array. However, when you test the app there is a test item within the array to start with.

The base user stories of being able to ADD a task, DELETE a task and MARK IT as completed have all been implemented and there are tests to show that these work.

The only user story from the original documentation that hasn't been implemented feature that allows the user to FILTER COMPLETED or INCOMPLETE tasks.

Learning outcomes

Iman and Dom haven't written about their learning outcomes and therefore it is hard to guage what they have learnt this week.

UI bugs & Instructions

There are quite a few errors that show up in the console that mention a Reference error or are related to the testing JavaScript file.

Only one test is running (Should add taskinput to list). The delete test is not running and does not display any output onto the console.

Array and Input box does not appear empty when you start up the app. Consider using taskInput.innerHTML= ""; to empty the input box.

When the array and input box test are commented out (see image below) image

The error message is output multiple times upon reloading the page. This maybe because of the click event being run without an input in multiple tests.

Consider using LET and CONST instead of VAR. VAR is a global variable and can often cause errors and can sometimes be hard to pinpoint where the error is coming from.

It is best practice to define functions outside of another function that calls it. Consider moving the enterTask function outside of the raiseError function.

File structure

The file structure is clear and easy to understand to the point where you can easily see the purpose of each file.

Flow of control, Naming & Readability

Variables and functions have been named really well and therefore the code flow is to follow.

The template literal inside the enterTask function is very cleanly written and easy to follow, to the point where you wouldn't need to refactor it 👍