Code Review in JavaScript: What to Check and How to Give Feedback

Code review in JavaScript: What to Check and How to Give Feedback

Code review is not just a formal check before merging a pull request. It is a powerful tool for improving code quality, sharing knowledge within the team, and preventing bugs at early stages. In the world of JavaScript and Node.js, where asynchrony and dynamic typing create many hidden pitfalls, competent review becomes critically important.

In this article, we will break down what exactly to pay attention to when reviewing JavaScript code, and how to give feedback so that it is useful, not offensive.

1. Logic, Architecture, and Security

The first thing to check is whether the code solves the given task. Do not immediately dive into formatting. Ask yourself questions:

• Does the logic work correctly? Are there any edge cases? What happens if null, undefined, or an empty array is passed?
• Are there any vulnerabilities? This is especially relevant in Node.js. Check whether raw data from a request is passed directly into an SQL query (SQL injection) or into eval(). Are HTML sanitizers being used?
• Is the architecture followed? Does the code violate SOLID principles? For example, is one function doing too much? Is business logic extracted from route handlers?

// BAD: Mixing logic and HTTP layerapp.get('/user/:id', async (req, res) => {    const user = await db.query(`SELECT * FROM users WHERE id = ${req.params.id}`); // SQL Injection!    res.json(user);});

// GOOD: Separation of concernsconst getUserById = async (id) => {    if (!id || typeof id !== 'string') throw new Error('Invalid ID');    return await db.query('SELECT * FROM users WHERE id = $1', [id]);};app.get('/user/:id', async (req, res) => {    const user = await getUserById(req.params.id);    res.json(user);});

2. Asynchrony and Working with Promises

This is the Achilles' heel of many JavaScript developers. When reviewing Node.js code, be sure to check:

• Error handling. Are all async functions wrapped in try/catch? Is the .catch() on promises forgotten? Are there any "orphaned" promises (unhandled promise rejections)?
• Correct use of async/await. Is there unnecessary await where promises could be run in parallel using Promise.all?
• Race conditions. Could the code execute incorrectly with parallel requests to the same resource (e.g., a file or a global variable)?

// BAD: Sequential execution of independent requestsconst user = await getUser(id);const posts = await getPosts(id);

// GOOD: Parallel executionconst [user, posts] = await Promise.all([getUser(id), getPosts(id)]);

// BAD: Forgotten error handlingrouter.get('/data', async (req, res) => {    const data = await fetchData(); // If fetchData fails, the server will "crash" too    res.json(data);});

// GOODrouter.get('/data', async (req, res, next) => {    try {        const data = await fetchData();        res.json(data);    } catch (error) {        next(error); // Pass the error to the global handler    }});

3. Code Quality, Style, and Naming

Code should be readable, like a good book. Even if the logic is correct, poor style will slow down development in the future.

• Naming variables and functions. Names should answer the questions "what is this?" and "what does it do?". Avoid single-letter variables (except for loop counters) and abbreviations like getUsrInf.
• Magic numbers and strings. Are they extracted into constants? const MAX_RETRIES = 3; is much clearer than