How to review Angular code

Original cover photo by Alvaro Reyes on Unsplash.

One of the most important responsibilities of a senior developer is reviewing code written by their fellow developers; if anything, there are lots of teams that don't even require one to be a senior developer to be able to review pull requests and code written by others in general (everyone can review anyone). In this article, we are going to explore common approaches and problems that are related to reviewing pull requests in Angular projects and answer several important questions.

How to notice important aspects in code?
How to spot obvious bugs?
How to write precise comments and be helpful?
How to ignore stuff if something is urgent?

So, let’s get started

What not to do?

Before we begin discussing approaches, let’s gloss over some stuff that should not be a part of a pull request review process

Do not write comments about missing trailing commas, variable names starting with an underscore, missing semicolons and so on. Use a linter for that
Do not push opinionated approaches unless existing code is not consistent with the code in the pull request, or you are the team lead (but even if you are, still think twice before rejecting an approach). If the code solves a problem in a way that is different from how that problem is tackled elsewhere in the codebase, comment about it; if the code has obvious problems because of the approach that was selected by the creator of the PR, comment about it. But if the approach is just different to your opinion, be careful about leaving any comments about it, even if you are the deciding team leader. There will be times when junior developers will know better than you (and that is completely okay, even cool!). This may result in unnecessary controversy and hinder the development process.
Do not repeat yourself. Sometimes a developer might choose an approach and repeat it multiple times when submitting a PR. Do not find all of those instances and comment the same thing under every appearance of the problem; rather, leave a generic comment describing that the approach needs to be changed across the PR related files.
Do not disregard deadlines - sometimes the fastest solution to a problem isn’t the most neat and beautiful; but approaching deadlines and extra work pressure can create situations where minimal solutions might be acceptable if they don’t violate existing structures too much. Be careful to acknowledge such situations and maybe keep a backlog of future refactorings.

Let’s now discuss some human stuff.

Be nice

An important thing to remember (always, not only in code review) is that people you interact with are human beings. Remember to do the following things:

Be polite when writing comments
Be precise and describe thoroughly why and how a piece of code can be improved; comments like “this does not feel good” are unacceptable.
Praise good code. If you only ever post comments about problems, your coworkers will feel that a PR review process is just an endless criticism fest
Ask questions. If a piece of code feels questionable, don’t jump to conclusions and post how it should be improved, but rather ask why the developer chose that approach; they might very well provide a good reason for it to be that way
Remember the power of please. If you think something should be changed, don’t write “change this method to use Array.map instead of Array.forEach”, but rather write “can we please change Array.forEach to Array.map?”

If you follow the guideline only in those two first paragraphs, you already will be a reviewer who does not cause problems; that is great. Let’s now try to become a reviewer that is very effective at the job!

Obvious things

Let’s start with noticing obvious and common pitfalls that people allow into their code, and become efficient at finding those:

Variable names. Pay attention to all new and changed variable names. Names need to be efficient at conveying meaning, not too complex, but not too simple either. Do not hesitate to comment i a variable name violates those principles
Common pitfalls. Sometimes developers fall for very common things that do not make the codebase any better. For example, usage of “Array.forEach” when “Array.map” and “Array.filter” could have been used. Familiarize yourself with common bad practices of JavaScript/TypeScript to notice such instances easier
Clever one-liners. Sometimes it is tempting to solve a more or less complex problem with a single line of code, but it is often better to write a bit more code that is more explicit, than something that feels like magic. If you notice a method that has one line of code, good if that line is simple enough; bad if you cannot immediately understand what that line of code achieves.

Now, let’s dive into what is specific to reviewing Angular codebases

Tips for Angular

Angular comes with its own list of bad and best practices (some of which are admittedly opinionated). Before you read this part of the guide, you can familiarize yourself with those using my following articles: Angular Bad Practices, Angular Bad Practices Revisited and Angular Best Practices. Also, definitely read and strictly follow the Angular Coding Style Guide. If you have even more time, you can watch my discussion with Santosh Yadav on his YouTube channel.

After exploring those, we can now start listing important things to pay attention to:

Explicit violations. First of all, look for explicit violations of the style guide (and the practices adopted by your own team). Things like incorrect component selectors, unnecessary class extensions (instead of DI) and so on. In 99% of the cases you won’t find any violations like this, but you might get all too sure that stuff like this won;t happen, and accidentally miss when it appears.
TypeScript issues. Look for too loosely typed variables, methods that do not have explicit return types but can return ambiguous values (in general pay lots of attention to types). Also look for type interconnections (you can read more about it in my article How not to trick the TypeScript compiler and not be tricked by it)
Too lengthy methods in components. While “not writing too verbose code” is a very generic advice, Angular developers often tend to push components to the extreme. Ideally components should receive data and render UI, but often developers would put lots of business logic inside components themselves. Be careful to notice things like that and propose solutions about where to move those pieces of code
Access to imported variables/constants directly instead of through dependency injection - this might be problematic for unit tests and also general readability of a component
Subclassing - components or services that extend other components or services are sometimes a good idea, but too often are used to just share functionality between classes. Whenever you encounter the extends keyword, examine if that is the case; if so, propose to refactor the class to use object composition/dependency injection instead of inheritance (remember, inheritance is an is-a relationship, not a has-a one)

Tips for RxJS

Most Angular codebases make at least some use of RxJS and stream based approaches; be careful to notice bad patterns, unnecessarily complicated code, and wrong usage of operators. Here are some things to pay attention to:

Too many subscriptions in the “ngOnInit” method - this is usually a red flag. Ideally, you would want to have exactly zero subscriptions for Observables, but sometimes there are situations it is necessary, but if you have more than one subscription, be careful to examine all of them to see if some can be refactored to just use the async pipe
More than 4 operators in an Observable pipe - usually lots of operators are not a problem, but sometimes they can introduce unnecessary complexity, and some operators have alternatives that do the same thing as a combination of 2
References to the component state in the Observable stream - be careful to notice any occurrences of the “this” keyword inside the operator callbacks, as usually this is not something to be encouraged; when noticing such instances, examine thoroughly why the component state was referenced and if/how it can be refactored. One way is to see if the properties changed inside the Observable pipeline are being displayed in the template; if so, use the async pipe. In general, avoid modifying to the this context of a component in Observables, unless it is by calling a third-party imperative function (like FormControl.disable, for example)
Pay special attention to Subjects - Subjects are often used to transport data from one part of application to another, and again can be abused to introduce unnecessary complexity into a system. Sometimes usage of a Subject can be discarded completely

In conclusion

Reviewing someone else’s code is often a challenging task, and requires both coding and social skills. Being attentive to details, clear with your comments and rigorous when upholding accepted practices is of huge importance. Hopefully, with this guide, you can become more efficient at reviewing code and finding issues