Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project Review #29

Open
twitharshil opened this issue Sep 21, 2022 · 3 comments
Open

Project Review #29

twitharshil opened this issue Sep 21, 2022 · 3 comments

Comments

@twitharshil
Copy link
Collaborator

Hey Team,

First, I would like to congratulate you all on completing your project. I've taken a closer look at the features you implemented and learned a lot from this project. With my experience, I came up with certain key areas that I felt you should work on to improve the project quality, structure, and efficiencies in working on a team project like this.

Overall project review points:

Code Quality:

  • Consistency in file extension is important for the clean architecture of the project. Currently, I don’t see a naming convention followed in the entire project. Also, the names given to the files are not meaningful. Forex: droplist.jsx & droplist1.jsx is not a good names.
  • The team should start using more functions instead of just bringing the entire feature use case within the same function.
  • Missing comments in the project on the function implementations is one of the concerns I faced during the code review.
  • Project is not well groomed, I can still see console.log statements present in the project. This is not a good practice.
  • Variable names used in components are not meaningful

Code Functionality

  • Email verification checks are missing on the front end side of the project during login and sign. Forex: a@g is not a valid email address. A regex check here would have enhanced the feature
  • Alerts are being used in authentication flow. This is not a good experience in general in production-ready codes.
  • A lot of scope for improving the authentication flow. I wasn't able to login into the website https://tmetric.netlify.app/login seems like the functionality is broken

Frontend react + Redux

  • The project is not well broken into different sets of components or not. For example: homepage.jsx can be broken into multiple sets of components component.
  • Components structures are not correct. All the functionalities like header footer etc should be inside a different component.
  • Project directory seems to be a bit unmaintained right now. The directory structure of the project needs to be self-explanatory so that anyone reviewing the project can easily find what are the components maintained at the app level and feature level.

Backend

  • Hard-coded values are observed in the code.
  • Authentication flow broken
  • Queries need to be optimized for authentication. While authenticating the username and password to the server, it is taking a lot of time to get a response

GitHub commits & Documentation

  • PR titles and commit messages should be meaningful. PR titles & Descriptions should be present in each ad for every PR made to the project. Reviewers get insights into the changes made only through PR titles and descriptions. It is one of the best practices in the industry
  • I’ve seen other folks in the team not reviewing the PRs raised by other individuals. This is not a good practice for team projects.
  • Missing comments in the issues and no activities in the issues being created don't reflect very good
  • Project details should be at the top of the README file

Architecture & wrong patterns

  • Styling consistency is missing in the entire project. Follow one way of styling. Consistency is essential for a clean and maintainable code.
  • LocalStorage usage and hard-coded names being used in the project which is not a very good practice.
  • I’ve seen a lot of console warning messages while running the project locally. We should always try to make our project errors & warnings free for a better debugging experience
  • Indentation is missing in a lot of code files. Not sure if the team used any linting or not. It is highly recommended to use any linting extension when you start working on a project
@ayaznoori
Copy link
Owner

Email verification checks fixed

@ayaznoori
Copy link
Owner

console.log commented or removed

@ayaznoori
Copy link
Owner

Alerts are being used in authentication flow fixed or removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants