Code review: Common mistakes found in developers code

Joanna

Programmers mindset

Code review is not only a learning opportunity for the person whose code is being reviewed. It is also a great way to spot recurring patterns of mistakes across the team. After reviewing dozens of pull requests, certain classics start to emerge – issues that appear regardless of a developer’s level of experience.

Some of these are minor oversights, while others can have a serious impact on an application’s performance or stability. While many problems come from working under time pressure, others are the result of missing or weak development habits. Knowing these patterns is valuable, because awareness of common pitfalls makes them much easier to avoid in your own code.

The most common issues found during code reviews

Below is a list of problems that regularly surface during reviews, from the obvious ones to more subtle, but equally important, issues.

1. Duplicate database queries

A true classic that shows up surprisingly often:

if (get_user_data($userId)) {
    $userData = get_user_data($userId);
    // ...
}

I’ve caught myself making this kind of mistake as well. The problem is simple – the exact same database query is executed twice. Once in a condition, and again when assigning the result. If the query is complex or the database is under load, this means unnecessary double use of resources.

The solution is just as simple:

$userData = get_user_data($userId);
if ($userData) {
    // ...
}

We assign the query result to a variable and then simply check that value. The result is faster execution and less load on the database. It may seem obvious, but in the middle of complex business logic, details like this are easy to miss – especially during refactoring, when conditions are being changed.

2. Performing operations inside a loop that should run only once

Loops are places where inefficiency multiplies with every iteration. A common mistake is putting operations inside the loop body that don’t actually need to be there:

for (let i = 0; i < items.length; i++) {
    const container = document.querySelector('.container');
    const apiUrl = getApiUrl();
    // ...
}

In the example above, the DOM is queried for the same element on every iteration, and a function that returns the same URL is called each time. If the loop iterates over a thousand items, that’s a thousand unnecessary operations.

The correct approach:

const container = document.querySelector('.container');
const apiUrl = getApiUrl();

for (let i = 0; i < items.length; i++) {
    // ...
}

Anything that stays constant for the entire loop should be moved outside of it. It’s a small change that can noticeably improve performance, especially when working with large data sets.

3. Attaching event listeners to each element individually and re-attaching them when elements are added dynamically

Adding event listeners without event delegation leads to an excessive number of active handlers that the browser has to maintain.

document.querySelectorAll('.button').forEach(button => {
    button.addEventListener('click', handleClick);
});

The problem has multiple layers. First, if there are 100 buttons, 100 separate event listeners are created, one for each element, which increases memory usage. Second, when new elements are added to the page without a full reload (for example, via AJAX), the listener has to be attached again to every newly added button.

The solution is event delegation:

document.addEventListener('click', (e) => {
    if (e.target.matches('.button')) {           // or e.target.closest('.button')
        handleClick(e);
    }
})

A single event listener at the document level (or better yet, at the container level) can handle all buttons – both existing ones and those added dynamically. This approach uses less memory, avoids duplication issues, and is much easier to manage.

4. Leaving unused code behind

Unused functions, variables, conditional logic, or commented-out blocks don’t just create noise in the codebase, they also make future analysis and debugging harder. Other developers start wondering whether the code is still needed, what it was meant to do, and whether it’s safe to remove. As a result, the project becomes less readable and more expensive to maintain.

Any code that is not being used should be removed before submitting it for review. If it’s ever needed again, it can always be retrieved from the git history. Code should contain only what is actually executed and actively used.

5. Copying code without understanding it

Copying snippets from other projects or contexts without understanding their logic and assumptions often leads to bugs that are difficult to track down. Just because a piece of code worked somewhere else doesn’t mean it will work in a different environment, with different dependencies, a different data model, or a different architecture.

Lack of understanding also increases long-term maintenance costs. A responsible developer avoids pasting code without thinking, taking time to understand it before deciding if it fits the project.

6. Missing variable validation

This mistake usually ends with “undefined variable” errors, warnings in logs, and in the worst case, production issues:

// We assume that $data['user'] is defined
$userName = $data['user']['name'];

// We assume that $items is an array
foreach ($items as $item) {
    // ...
}

The problem appears when those assumptions are not met. A function is called without the required parameters, API data doesn’t have the expected structure, or the user is not authenticated. The result? Errors that could have been handled gracefully end up breaking parts of the functionality.

The correct approach:

if (isset($data['user']['name'])) {
    $userName = $data['user']['name'];
}

if (is_array($items)) {
    foreach ($items as $item) {
        // ...
    }
}

7. Not testing your own code before submitting it for review

Making changes without running the application, checking the console, or looking at linter warnings is one of the most destructive habits a developer can have. This approach shifts the responsibility for basic testing onto the reviewer, which slows the project down and lowers the overall standard of work.

The problem isn’t that the code contains bugs – that’s normal. The real issue is when the person submitting the code hasn’t even checked whether it works at all. The page wasn’t refreshed, the console wasn’t checked, and no linter or tests were run. Code review is not functional testing. It’s a review of logic, architecture, and potential issues.

Before sending your code to someone else:

  • Refresh the page and make sure it works
  • Check the console for errors
  • Run linters and tests
  • Click through the parts you changed
  • Verify basic usage scenarios

This is not just about respecting the reviewer’s time – it’s also about taking responsibility for the quality of the code you submit. If you haven’t verified that something works, you’re effectively pushing testing onto someone else. That’s unfair and leads to longer review -> fix -> re-review cycles.

8. Assigning a variable to another variable when the value can be used directly

Sometimes code contains constructs that technically work but act as unnecessary intermediaries:

$filter_type = is_category() ? 'category' : 'all';
$args['current_filter'] = $filter_type;

The $filter_type variable is used exactly once, only to assign its value to $args['current_filter']. There is no real reason for it to exist as a separate entity.

Simpler and more readable:

$args['current_filter'] = is_category() ? 'category' : 'all';

Of course, there are situations where such a variable makes sense – when it’s used multiple times, when its name carries important semantic meaning, or when it improves the readability of a complex expression. But if it’s only acting as a middleman between one expression and another, it unnecessarily adds noise and makes the code harder to read.

Every variable in the code should have a clear purpose. If its only role is to pass a value along, it can usually be removed.

9. Using inconsistent naming conventions within a project (or even a single file)

Inconsistent naming makes code harder to read, search, and understand, and it obscures relationships between different parts of the code. It also introduces chaos in teams, where each developer starts applying their own habits. Sticking consistently to a single convention (such as camelCase, snake_case, or PascalCase) is one of the fundamentals of professional software development.

10. Careless use of expensive JavaScript operations

If a scroll event fires on every pixel of scrolling, it can trigger hundreds of calls per second. If each of those calls queries the DOM, performs complex calculations, or modifies styles, the browser quickly becomes overwhelmed. The page starts to stutter, and the user experience suffers.

The problem gets even worse when the code repeats the same work over and over despite nothing actually changing, for example, recalculating window height on every scroll (even though it only changes on resize), or iterating over all headings and updating their classes when none of them changed state.

Solutions:

  1. Debounce or throttle
    Debounce and throttle limit how often a function is executed over time, so the browser doesn’t have to perform expensive operations on every single event (such as scroll, resize, or input). Debounce delays execution until the user stops the action, while throttle ensures the function runs at most once within a given time interval. Both techniques significantly reduce main-thread load and improve UI smoothness.
  2. Intersection Observer
    Intersection Observer allows code to react only when an element actually enters the visible viewport. This removes the need to manually track scroll position and calculate element offsets. It’s a more modern and far more efficient approach for lazy loading, animations, or activating sections.
  3. requestAnimationFrame
    Instead of reacting directly to fast, repetitive events, requestAnimationFrame can be used to synchronize work with the browser’s render cycle. This prevents CPU overload and results in smoother, more controlled animations and calculations.

Most problems found during code reviews are the result of rushing, tiredness, small oversights, or taking shortcuts. A few simple habits can make a big difference:

  • test your code before sending it for review
  • maintain consistency across the project
  • avoid duplicating the same logic in multiple places
  • use functions with an understanding of their cost

These small changes in approach make a real difference. The code becomes easier to read, reviewers have fewer comments, and the project moves forward more smoothly.