Code Review #2

Entry Level / Backend / NodeJS

Aviram Roisman's photo
Aviram Roisman
·

3 min read

General Impression : The project is written well, Im not sure if this project is a personal project to learn deeply certain things, or its a project to add to your Portfolio, if so.. You have to add some README.md file and add instruction to run this project, the standart is to run 

npm start

in your case -> it won't work, cause I should run 'build' first...

so its a small thing but its make the impression you thought about the person who reviewing it..

another point, is the env file, I don't have one, and its legit, but you could add some script to run and create a local env file or at least add default values, to make this project "testable"

besides that, its really good, you do use a lot of stuff that in my POV is unnecessary in this kind of project (small, personal..) but its ok..

server.ts

current code

const normalizePort = (val: string) => {
    var port = parseInt(val, 10);

    if (isNaN(port)) {
        return val;
    }

    if (port >= 0) {
        return port;
    }

    return null;
};

const onError = (error: NodeJS.ErrnoException) => {
    if (error.syscall !== 'listen') {
        throw error;
    }

    const addr = server.address();
    const bind = typeof addr === 'string' ? ('pipe ' + addr) : ('port ' + port);

    switch (error.code) {
        case 'EACCES':
            console.error(bind + 'requires elevated privileges');
            process.exit(1);
            break;
        case 'EADDRINUSE':
            console.error(bind + ' is already in use');
            process.exit(1);
            break;
        default:
            throw error;
    }
};

const onListening = () => {
    const addr = server.address();
    const bind = typeof addr === 'string' ? ('pipe ' + addr) : ('port ' + port);

    debug("Listening on " + bind);
};

suggestion:

I'd take this function out to some 'utils' file, it make it more difficult to understand what is going on there..

current Code

ServerGlobal.getInstance().logger.info(`Server is running on port ${process.env.PORT}`);

suggestion:

it's a bit weird, cause you add const port, why not to use it again? if there is a reason -> comment it

app.ts

current code

    res.status(200).send("Shop server is alive")

suggestion:

'status(200)' is irrelevant, cause its the default that is not a mistake, but its waste of time to write it

controller/auth.ts

current code

  const isEmailValid =
    /(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*|"(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21\x23-\x5b\x5d-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])*")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\[(?:(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9]))\.){3}(?:(2(5[0-5]|[0-4][0-9])|1[0-9][0-9]|[1-9]?[0-9])|[a-z0-9-]*[a-z0-9]:(?:[\x01-\x08\x0b\x0c\x0e-\x1f\x21-\x5a\x53-\x7f]|\\[\x01-\x09\x0b\x0c\x0e-\x7f])+)\])/.test(
      req.body.email
    );

suggestion:

I'd move this validation to some validations/utils file..

controller/product.ts

current code

     category: +req.body.category,
      gender: +req.body.gender,

comment:

I agree with this convention of + to change it to number, but I work in company that prefer Number() cause it more readable, so just to let you know that there is places that prefer otherway

middleware/security.ts

the 'bodyKeys' function, is a nice way to show your knowledge, but I think it's too much for this kind of project...

Node.jscode review