diff --git a/client/src/App.css b/client/src/App.css
index eb31e71..0e41bd7 100644
--- a/client/src/App.css
+++ b/client/src/App.css
@@ -4,3 +4,4 @@
} */
+/* Don't leave empty files in the codebase */
\ No newline at end of file
diff --git a/client/src/App.test.js b/client/src/App.test.js
index 1f03afe..554ec70 100644
--- a/client/src/App.test.js
+++ b/client/src/App.test.js
@@ -1,3 +1,5 @@
+// This file should be in a separate tests directory.
+
import { render, screen } from '@testing-library/react';
import App from './App';
diff --git a/client/src/Components/Homepage/Footer.jsx b/client/src/Components/Homepage/Footer.jsx
index 8781e55..402ecb4 100644
--- a/client/src/Components/Homepage/Footer.jsx
+++ b/client/src/Components/Homepage/Footer.jsx
@@ -61,6 +61,7 @@ export default function Footer() {
+ {/* The repetition of these tag blocks below could be avoided and replaced with loops. */}
TOGGL GLOBAL
Blog
@@ -112,7 +113,8 @@ export default function Footer() {
-
+ {/* The repetition of tags below could be avoided and replaced with a loop. */}
+
diff --git a/client/src/Components/Homepage/FooterSignup.jsx b/client/src/Components/Homepage/FooterSignup.jsx
index 89c9d08..50ab30f 100644
--- a/client/src/Components/Homepage/FooterSignup.jsx
+++ b/client/src/Components/Homepage/FooterSignup.jsx
@@ -1,3 +1,5 @@
+// Remove all of these commented lines.
+
// import { EmailIcon } from '@chakra-ui/icons';
// import {
// Flex,
diff --git a/client/src/Components/Homepage/secondHome.module.css b/client/src/Components/Homepage/secondHome.module.css
index cfb8a3e..efaeb7c 100644
--- a/client/src/Components/Homepage/secondHome.module.css
+++ b/client/src/Components/Homepage/secondHome.module.css
@@ -1,3 +1,6 @@
+/* Move the jsx and css files for the same component into its own folder
+like done for some other components. Follow one convention. */
+
/* second div style start from here */
.fixtop{
width: 100%;
diff --git a/client/src/Components/Loging/Login.jsx b/client/src/Components/Loging/Login.jsx
index 7e85a0d..0c5e5c7 100644
--- a/client/src/Components/Loging/Login.jsx
+++ b/client/src/Components/Loging/Login.jsx
@@ -58,7 +58,9 @@ export default function Login() {
.then((res) => res.json())
.then((res) => {
console.log(res)
-
+ // Never string match the error message. Later if the backend changes the error text
+ // this check will fail and the code will behave unexpectedly. Instead the status code
+ // or some status enum should be checked for here.
if(res.message === "required fields are email,password")
{
toast({
@@ -113,15 +115,18 @@ export default function Login() {
})
-
+ // This is poor error handling. Logging the error isn't enough.
.catch((err) => console.log(err))
}
return (
<>
+
+{/* Please format code. It is all over the place. */}
+ {/* Such urls should be put in some constants file */}
diff --git a/client/src/Components/NavComponents/responsive Nav/navElements.jsx b/client/src/Components/NavComponents/responsive Nav/navElements.jsx
index 3262c17..df434a0 100644
--- a/client/src/Components/NavComponents/responsive Nav/navElements.jsx
+++ b/client/src/Components/NavComponents/responsive Nav/navElements.jsx
@@ -1,3 +1,6 @@
+// Why does this file have a jsx extension when there is no jsx. This prevents the IDE
+// from properly highlighting the syntax and showing any suggestions.
+
import styled from "styled-components";
import { Link } from "react-router-dom";
import {FaBars} from "react-icons/fa";
diff --git a/client/src/Components/TimerPage/SubNav.js b/client/src/Components/TimerPage/SubNav.js
index 0cfb292..de2257d 100644
--- a/client/src/Components/TimerPage/SubNav.js
+++ b/client/src/Components/TimerPage/SubNav.js
@@ -15,6 +15,11 @@ import {
import { format } from "date-fns";
import Rangetimer from "./Rangetimer";
+// A common issue that all the components in the codebase suffer from is that the styling is defined inline
+// instead of separating that in a dedicated scss or css file. Inline css is usually okay when the component
+// is tiny and the css rules are relatively self contained. For such a big component, inline css makes this
+// component practically non-reusable, degrades code readbility and clutters the layout and logic contained
+// in the component with its styling rules which have little role to play in what the component is about.
const SubNav = () => {
const[open,setOpen]= useState(false)
const [date, setDate] = useState([
diff --git a/client/src/Components/TimerPage/api.js b/client/src/Components/TimerPage/api.js
index e936723..e3fc095 100644
--- a/client/src/Components/TimerPage/api.js
+++ b/client/src/Components/TimerPage/api.js
@@ -1,3 +1,6 @@
+// Don't put utility javascript code along with the components. Have a separate
+// utils folder for that.
+
import axios from 'axios'
export const postdata = (send) => {
console.log(send)
diff --git a/client/src/Components/prices/Apps.jsx b/client/src/Components/prices/Apps.jsx
index a8ad43f..5a21b11 100644
--- a/client/src/Components/prices/Apps.jsx
+++ b/client/src/Components/prices/Apps.jsx
@@ -34,6 +34,7 @@ const Apps = () => {
pb="50px"
>
+ {/* Passing styling properties as props is a very strange pattern. Never seen it in well written codebases. */}
MOBILE APPS
diff --git a/client/src/Components/prices/Faqs.jsx b/client/src/Components/prices/Faqs.jsx
index a4e5b55..399ac53 100644
--- a/client/src/Components/prices/Faqs.jsx
+++ b/client/src/Components/prices/Faqs.jsx
@@ -52,6 +52,10 @@ const Faqs = () => {
+ {/* Such strings should be declared in one place in a separate class or js file.
+ For example, see - https://github.com/mozilla/addons-frontend/blob/master/src/amo/constants.js
+ This makes managing static text easier. It also becomes easier to add support l10n and i18n
+ so the app can render text for different languages (like rtl languages). */}
You will be charged a monthly fee for each member of your team.
For paid plans, this fee applies even if you have under 5 active
diff --git a/client/src/Components/prices/PlanComparison.jsx b/client/src/Components/prices/PlanComparison.jsx
index 2f044e4..ab136ae 100644
--- a/client/src/Components/prices/PlanComparison.jsx
+++ b/client/src/Components/prices/PlanComparison.jsx
@@ -109,6 +109,8 @@ const PlanComparison = () => {
Time Tracking
+ {/* What is this for? I'm guessing this is for adding some column space.
+ If so, it is not the right way to do so. Please avoid such hacks. */}
diff --git a/client/src/Components/prices/PricesTop.jsx b/client/src/Components/prices/PricesTop.jsx
index e7907b9..4d8bba5 100644
--- a/client/src/Components/prices/PricesTop.jsx
+++ b/client/src/Components/prices/PricesTop.jsx
@@ -149,6 +149,12 @@ const navigate =useNavigate()
}}
textAlign={"left"}
>
+ {/* The following block could've been written in a much neater fashion by having a
+ containerwhich assembles each Flex > Box > BsCheckLg unit by looping over a prop
+ containingdata elements in an array. This is precisely a perfect example of anti-patterns
+ withReact components. Components are written for being reusable and helping maintain code
+ better. What is the point of using react and its component framework if y'all hardcode
+ the entire page like this? Same applies below as well */}
diff --git a/client/src/Components/sample.jsx b/client/src/Components/sample.jsx
index 67622be..7a4fa44 100644
--- a/client/src/Components/sample.jsx
+++ b/client/src/Components/sample.jsx
@@ -1,3 +1,5 @@
+// What is this for? Please remove such spurious components which serve no purpose.
+
import React from 'react'
const sample = () => {
diff --git a/client/src/Components/sidebar/Sidebar.jsx b/client/src/Components/sidebar/Sidebar.jsx
index f133502..041cce3 100644
--- a/client/src/Components/sidebar/Sidebar.jsx
+++ b/client/src/Components/sidebar/Sidebar.jsx
@@ -1,3 +1,53 @@
+// This file is just unreadable.
+// 1. Lots of unused imports
+// 2. Lots of commented code
+// 3. Absolutely 0 comments on what purpose the sidebar serves
+// 4. Tons of CSS mixed along with the component definition and contents
+//
+// Please write smaller and more well defined components which have a specific purpose.
+// Each component should have a few comments about what it does, a sample usage example if possible, etc.
+// A good component has a well defined purpose and usually it should be configurable so it can be used elsewhere too.
+// From the code below, it looks like Sidebar is a "container" which is a component that is composed of multiple smaller
+// independent components. I'm making this distinction because that helps to think about the UI in a different, more
+// compartmentalised way that makes organising, writing, reusing and maintaining code easier.
+//
+// Also, the current way of combining the entire JSX in a single return statement makes understanding the sub-components
+// extremely difficult. A better way to use JSX is to define different variables for different logical elements within
+// the component. For example (might not be strictly syntactically correct) -
+//
+// const Header = (props) => (
+//
+//
+//
+//
+//
+//
+//
+// );
+// }
+//
+// The example above makes reading and understanding what the component is doing is much easier.
+//
+// A good example of a cleanly written big component is -
+// https://github.com/mozilla/addons-frontend/tree/master/src/amo/components/AddonsCard.
+// It also separates its styling from the component and encapsulates all the points described above.
import React,{ReactNode, useEffect, useState} from 'react'
import {
diff --git a/client/src/Mainroutes/Navroutes.jsx b/client/src/Mainroutes/Navroutes.jsx
index c17274c..29fb883 100644
--- a/client/src/Mainroutes/Navroutes.jsx
+++ b/client/src/Mainroutes/Navroutes.jsx
@@ -1,5 +1,7 @@
import React from 'react'
import { Routes,Route } from 'react-router-dom'
+// Remove all unused imports from all the files please. Importing something adds an overhead
+// while loading the application / component. Avoid this.
import Footer from '../Components/Homepage/Footer'
import Homepage from '../Components/Homepage/Homepage'
import Navbar from "../Components/NavComponents/Navbar"
@@ -30,6 +32,7 @@ const Navroutes = () => {
} />
} />
} />
+ {/* Have consistency in naming your routes. Follow the same convention. */}
} />
}/>
{/* } /> */}
diff --git a/server/backend server/config.js b/server/backend server/config.js
index 47a48f7..27d0b11 100644
--- a/server/backend server/config.js
+++ b/server/backend server/config.js
@@ -1,6 +1,18 @@
const mongoose = require("mongoose");
require("dotenv").config();
+// Typically a database connection pool should be created and used throughout the application.
+// Having only a single connection limits the database operations that can be performed at once.
+// A connection pool is a resource which manages multiple database connections so that as needed
+// the APIs can acquire a connection, use it and return it back to the pool after use. The
+// advantage of a pool is that you can perform multiple independent database operations at once,
+// improving the performance of your application. Databases can support 100s of concurrent connections
+// so using only one connection also leads to wasteage of compute and memory resources of the
+// database server. Having only a single connection also means that in case of concurrent API
+// invocations only one API call can effectively make progress (the one which is using the database
+// connection at the moment). This means that the other API calls will have a much higher wait time
+// and thus higher than expected latency.
+// Mongoose supports connection pools. See - https://mongoosejs.com/docs/connections.html#connection_pools.
const connection = mongoose.connect(process.env.mongo_url);
module.exports = {
connection,
diff --git a/server/backend server/controller/authController.js b/server/backend server/controller/authController.js
index 8f8e223..c16a7c1 100644
--- a/server/backend server/controller/authController.js
+++ b/server/backend server/controller/authController.js
@@ -5,16 +5,33 @@ var jwt = require("jsonwebtoken");
const register = (req, res) => {
if ( !req.body.email || !req.body.password) {
- return res.send({message:"required fields are email,password"});
+ // Explicitly add status codes for error responses. Using a json object for the response body is
+ // good but not having the correct status code hurts. The statement should be -
+ // return res.status(400).send({message:"required fields are email,password"});
+ // Status code 400 is used for indicating bad requests, where the request was malformed or
+ // had missing parameters. Also a better way to write the same error message is -
+ // "Missing required fields - email, password"
+ return res.send({message:"required fields are email,password"});
}
const { email, password } = req.body;
bcrypt.hash(password, 8, async (err, hash) => {
// Store hash in your password DB.
if (err) {
- return res.send("error in bcrypt password");
+ // This is not a good way to return error responses. APIs should conform to REST principles.
+ // All error response should be formatted in a certain way so that the client knows what to expect.
+ // A proper json object with error message and any other relevant information must be sent back
+ // with an appropriate status code. For internal errors status code 401 should be used.
+ // Read about status codes here - https://restfulapi.net/http-status-codes/
+ // It should be something like -
+ // res.status(500).send({ error: "Encrypting the password failed" });
+ // Also, try to maintain uniformity with responses in the application. Like above and below, both
+ // error responses are json objects but here it is a string.
+ return res.send("error in bcrypt password");
}
const user = new UserModel({ email, password: hash });
+ // What if this operation fails? Always handle for failures and add fallback mechanisms.
await user.save();
+ // Use status code 201 here.
return res.send({message:"Register Successfull"});
});
};
@@ -28,6 +45,9 @@ const login = async (req, res) => {
try {
const find_user = await UserModel.findOne({ email });
if (!find_user) {
+ // Avoid words like 'Plz'. Also the real error is that the user was not found. So the
+ // error message should be something like - "User with email: ${email} not found".
+ // Should use status code 404. Similarly for all the error cases below.
return res.send({message:"Plz enter registered details"});
}
diff --git a/server/backend server/controller/taskController.js b/server/backend server/controller/taskController.js
index a67b32c..ea2da61 100644
--- a/server/backend server/controller/taskController.js
+++ b/server/backend server/controller/taskController.js
@@ -1,7 +1,51 @@
const TaskModel = require("../models/Task.model");
+// Each API should have a head comment which provides a good description about:
+// 1. what the API does
+// 2. what the request looks like
+// 3. what responses can be expected (success and failure cases both included)
+// post all the data
const createTask = async (req, res) => {
- try {
+ // The very first statement in any API function should be to log the incoming request.
+ // In case of any failure downstream, knowing the incoming request helps a lot during
+ // debugging and understanding what could've caused the failure. Logging is extremely helpful
+ // and logging the right things in the right place with the right messages can be the
+ // difference between spending hours in finding the root error causes vs just looking at the
+ // logs and figuring it out in 2 minutes.
+ //
+ // Each API function should follow the following structure -
+ //
+ // const <_http_method_><_resource_> = async (req, res) => {
+ // console.group('Received request from {req.user.id} to create task, with body: {req.body}.');
+ // try {
+ // console.log('Performing _operation_ with params: {_params_}.')
+ // // perform API specific operations here
+ // console.log('Successfully performed _operation_.');
+ // res.status(_status_code_)
+ // } catch (e) {
+ // console.error('Could not perform _operation_.', e.message);
+ // // add some retry logic here.
+ // if (numRetries > retryLimit) {
+ // res.status(500).send({ "error": "_relevant_error_message_goes_here_" });
+ // }
+ // }
+ // console.groupEnd();
+ // });
+ //
+
+ // Deserialisation can be done is a better way. Usually for serialisation and deserialisation
+ // static functions are defined in the model class itself. Something like this for example -
+ //
+ // In models/Task.model.js
+ //
+ // // This function deserialises the input json into a Task object.
+ // static from(task_json, user_json) {
+ // return Object.assign(new Task(), task_json, user_json);
+ // }
+ //
+ // Its invocation would be something like -
+ // const task = Task.from(req.body, req.user);
+ try {
const newTask = await TaskModel({
title: req.body.title,
id:req.body.id,
@@ -14,19 +58,36 @@ const createTask = async (req, res) => {
console.log(err);
}
};
+
const getAllTasks = async (req, res) => {
try {
const allTasks = await TaskModel.find();
// if (!allTasks) return res.send({message:"No Task found"});
return res.send(allTasks);
} catch (err) {
+ // Errors should be logged at the error level, not the default level.
console.log(err);
+ // Error message used below is not clear at all. It gives 0 idea about
+ // what happened at the server and whether the client should retry the
+ // request and hope for the correct response the next time. The error
+ // response, should contain a human readable / understandable message
+ // with the proper status code. In this case the status code would be
+ // 500. This statement should be something like -
+ // res.status(500).send({"error": _stringified_error_});
+ // Also, there should be some form of failure handling on the server
+ // end first, based on the error type. There should be some notion of
+ // whether the error was transient and would get fixed on retries.
+ // Error handling in APIs is very crucial for applications working at
+ // a large scale.
res.send("sth went wrong");
}
};
const editTasks = async (req,res) => {
try{
+ // What is the '+' for? It serves no purpose. Avoid such statements.
+ // Nit: Also req.params.id can be used inline in the 2 places. That
+ // will avoid creating a new variable.
const id= +req.params.id
const allTasks = await TaskModel.findOne({id:id})
const update_user = await TaskModel.findOneAndUpdate({id :id},
@@ -44,6 +105,24 @@ const editTasks = async (req,res) => {
const deleteTasks = async (req,res) => {
try{
const id= +req.params.id
+ // Always log before and after database (or any IO operations for that matter).
+ // Certain IO operations might take long to run so it might also be nice to
+ // read the logs and see how long it takes on average for some operation to
+ // finish. It will give some idea about what the critical path of the API is.
+ // This is helpful in figuring out what operations in the API flow should be
+ // focused on for optimisations.
+ //
+ // IO operations also need to have proper error handling. Having a catch-all
+ // for all kinds of errors is not a good way to do error handling because errors
+ // from different operations are different and should be treated as such. Some
+ // errors would be transient like say transaction timeout error or connection
+ // error but something like duplicate primary key or some other column constraint
+ // error is not. Knowing the distinction between error types greatly helps in
+ // making the API code more resilient to failures and improves the success rate
+ // of the API calls. Large applications with way more complex APIs and a lot more
+ // moving parts cannot tolerate points of failures. They all have some or the other
+ // fallback mechanism. Think about such things while writing APIs. Read more
+ // about how to better handle errors.
const data=await TaskModel.findOne({id:id})
const del = await TaskModel.findOneAndDelete({id :id});
res.send({message:"delted",data:del})
diff --git a/server/backend server/index.js b/server/backend server/index.js
index 82e49e0..fd1298d 100644
--- a/server/backend server/index.js
+++ b/server/backend server/index.js
@@ -1,3 +1,7 @@
+// This file is just all over the place. Ideally it should be 10 20 lines at max,
+// with 10 lines of imports and 10 lines just initialising the app object. It should
+// absolutely not contain any API logic, or routes munging.
+
const express = require("express");
const { connection } = require("./config");
const cors = require("cors");
@@ -12,6 +16,15 @@ const passport = require('passport');
require('./passport');
+// This is a mess again. All the routes should've been part of routes/index.js.
+// Defining half of the stuff there and the other half here is a poor practice.
+// Have consistency in your code. Decide and follow one way of doing things
+// within the team. This smells like lack of proper team co-ordination. Such
+// things should be discussed upfront and sorted out so everyone knows how to
+// go about things. Y'all should refer to and follow guidelines laid out by
+// the framework authors for best practices.
+// In a large project where at least 20 different people write code, if everyone
+// decided to do things their way, the code would become unmaintainable very soon.
const allRoutes = require("./routes/index");
const authentication = require("./middlewares/authentication");
const ProjectController = require("./routes/project")
@@ -28,6 +41,8 @@ app.use(morgan("tiny"));
app.use(express.json());
app.use(cookieParser());
app.use("/project", authentication,ProjectController)
+// Why is this called timer? Use the name of resource and not some random name for an
+// API route.
app.use("/timer", authentication,TaskController)
// Routes
app.use("", allRoutes);
@@ -39,6 +54,9 @@ app.get("/", (req, res) => {
// oauth
+// Delete all the commented code please to improve codebase health.
+// Seeing commented code in so many files hurts.
+
// app.use(cookieSession({
// name: 'google-auth-session',
// keys: ['key1', 'key2']
@@ -59,6 +77,9 @@ app.get("/", (req, res) => {
// const port = process.env.PORT || 8080
+// All of the APIs below should be in their respective controller and route files.
+// This is not the place to add any business logic.
+
app.get("/login",(req,res)=> {
res.send(`Login via google `)
})
@@ -114,7 +135,7 @@ app.get("/logout", (req, res) => {
})
-
+// Don't leave blank lines at random. This is very weird.
diff --git a/server/backend server/middlewares/authentication.js b/server/backend server/middlewares/authentication.js
index 932ac95..4d5d287 100644
--- a/server/backend server/middlewares/authentication.js
+++ b/server/backend server/middlewares/authentication.js
@@ -1,4 +1,8 @@
var jwt = require("jsonwebtoken");
+
+// Leave a blank line between the require statements and the actual code.
+// Also remove all commented log statements. It is better to keep your
+// code clean of dead or commented statements.
const authentication = (req, res, next) => {
// const token = req.headers.authorization;
const token = req.cookies.access_token ;
diff --git a/server/backend server/models/project.model.js b/server/backend server/models/project.model.js
index c253c65..4ddc8ef 100644
--- a/server/backend server/models/project.model.js
+++ b/server/backend server/models/project.model.js
@@ -1,3 +1,8 @@
+// Leave blank lines where necessary. This looks hotch potchy.
+// The naming convention for the model files is also not
+// consistent. Make sure to follow one convention (which should
+// always be the language standard). Read style guides for the
+// right naming practices.
const mongoose=require("mongoose")
const { Schema } = mongoose;
diff --git a/server/backend server/passport.js b/server/backend server/passport.js
index baec60b..614a958 100644
--- a/server/backend server/passport.js
+++ b/server/backend server/passport.js
@@ -1,3 +1,9 @@
+// Why is this file called passport? Because it uses some dependency called passport?
+// This is so wrong. Files should be named after the class or functionality they expose.
+// That gives the reader some context of what to expect before even opening the file.
+// Random names don't help at all. Also, using the same name as another package adds to
+// even more confusion.
+
const passport =require("passport")
const GoogleStrategy = require('passport-google-oauth2').Strategy;
require("dotenv").config()
@@ -35,6 +41,7 @@ passport.deserializeUser(function(user, done) {
done(null, user);
});
+// Add comments on what this block is about and what it does.
passport.use(new GoogleStrategy({
clientID:process.env.CLIENT_ID,
clientSecret:process.env.CLIENT_SECRET,
diff --git a/server/backend server/routes/auth.js b/server/backend server/routes/auth.js
index 6ec90b5..5400d2c 100644
--- a/server/backend server/routes/auth.js
+++ b/server/backend server/routes/auth.js
@@ -14,6 +14,7 @@ router.get("/hello", (req, res) => {
router.post("/register", register);
router.post("/login", login);
router.get("/logout", logout);
+// Either use snake case or camel case. This API path looks off.
router.get("/isloggedin", isLoggedIn);
module.exports = router;
diff --git a/server/backend server/routes/index.js b/server/backend server/routes/index.js
index 9447b1e..8258269 100644
--- a/server/backend server/routes/index.js
+++ b/server/backend server/routes/index.js
@@ -8,6 +8,8 @@ const authentication = require("../middlewares/authentication");
const router = express.Router();
router.use("/auth", authRoutes);
+// Resource names in the API path should always be singular. So this should be 'task'
+// and the one below should be 'user'.
router.use("/tasks", authentication , taskRoutes);
router.use("/users", authentication, usersRoutes);
diff --git a/server/backend server/routes/project.js b/server/backend server/routes/project.js
index d3ace15..aeb8f78 100644
--- a/server/backend server/routes/project.js
+++ b/server/backend server/routes/project.js
@@ -3,6 +3,10 @@ const express=require("express")
const ProjectController= express.Router()
const ProjectModel=require("../models/project.model")
+// Follow one convention. Define all the API functions in the controllers
+// folder and just import that here and define the route using that function.
+// Don't define the API function in line with the route. Avoid mixing such
+// things.
ProjectController.post("/create",async(req,res)=>{
const{id,name,client}=req.body
diff --git a/server/index.js b/server/index.js
index e69de29..386bb8b 100644
--- a/server/index.js
+++ b/server/index.js
@@ -0,0 +1,2 @@
+// Why does this empty file exist? Also, there are way too many index.js files
+// in the codebase under different sub-directories.
\ No newline at end of file
