Common Review Pitfalls to Avoid

Before diving into specific techniques, it's worth addressing what should stay out of the review process entirely. Many habits that seem helpful actually introduce noise and friction.

  • Avoid nitpicking style mechanics — missing semicolons, underscore-prefixed variables, or trailing comma inconsistencies belong to the linter, not to your review comments.
  • Avoid pushing personal preferences — unless the submitted code deviates from established patterns in the codebase or the chosen approach introduces real problems, refrain from commenting. Even as a team lead, think carefully before rejecting an alternative solution. Sometimes the junior developer's approach is genuinely better, and that's perfectly fine.
  • Avoid repetitive comments — if the same issue appears in multiple places, don't comment on every occurrence. Instead, leave one general note explaining that the pattern needs fixing throughout the PR.
  • Respect deadlines — the fastest solution isn't always the prettiest. Under time pressure, a minimal fix that doesn't badly break existing conventions should be accepted, with a note for future refactoring in the backlog.

Communication Matters

Always keep in mind that there's a person behind every pull request. Some basic courtesy goes a long way:

  • Use polite phrasing in every comment.
  • Explain precisely why and how a piece of code could be improved. Vague remarks like "this doesn't feel good" are never acceptable.
  • Point out good work too. If reviews contain only criticism, they quickly become a demoralizing experience.
  • Ask rather than assume. When something looks questionable, ask the author about their reasoning instead of immediately suggesting a fix. There might be a valid explanation.
  • Use the word please. Instead of saying "change this method to use Array.map," try "can we please switch Array.forEach to Array.map here?"

If you follow just these two sections, you're already a reviewer who doesn't create problems. Now, let's focus on becoming one who actively improves the codebase.

Spotting Low-Hanging Fruit

Some issues are easy to identify once you know what to look for:

  • Naming quality — inspect every new or changed identifier. Names should convey meaning effectively — not overly complex, not overly vague. Don't hesitate to question names that fall short.
  • Well-known mistakes — developers frequently settle into bad habits. Reaching for Array.forEach when Array.map or Array.filter would be more appropriate is a classic example. Knowing common JavaScript and TypeScript anti-patterns makes them much easier to catch.
  • Clever one-liners — compact code can be tempting, but more explicit solutions are often better than something that resembles magic. If a method is a single line, make sure it's simple enough to be immediately obvious. If not, it needs reworking.

Where Angular Codebases Need Attention

Angular brings its own set of conventions, some admittedly opinionated. A great resource to understand these is the Angular Coding Style Guide, along with the article Angular Bad Practices, Angular Bad Practices Revisited, and Angular Best Practices. If there's time, the recorded discussion with Santosh Yadav on his YouTube channel is worth watching.

With that background, here's a list of key things to examine:

  • Rule violations — start by looking for clear-cut violations of the style guide or your team's own practices. Issues like incorrect component selectors or unnecessary class inheritance where dependency injection would suffice are examples. Violations are rare in most PRs, but it's easy to become complacent and miss the one that does appear.
  • TypeScript matters — look out for overly loose typing, methods without explicit return types that can yield ambiguous results, and interconnected types in general. The article How not to trick the TypeScript compiler and not be tricked by it provides more depth on this.
  • Bloated components — although "avoid verbose code" is a general guideline, Angular devs are especially prone to pushing components too far. Ideally a component receives data and renders a view. When business logic starts infiltrating, catch it and suggest where it could live instead.
  • Importing values instead of injecting them — accessing imported variables or constants directly rather than through DI creates issues with unit testing and component readability. Watch for this pattern.
  • Inheritance abuse — subclassing to share functionality is a common trap. The extends keyword should raise a flag every time. If it's about sharing rather than true specialization (an is-a relation, not a has-a one), suggest refactoring to composition or DI.

Paying Attention to RxJS

Most Angular applications rely on RxJS at least in part. Faulty operator choices, overcomplicated streams, and anti-patterns in general are all worth watching for. Specific items to keep in mind:

  • Subscription sprawl in ngOnInit — several subscriptions right in the initializer suggests trouble. Ideally there are zero raw subscriptions for Observables, but that's not always possible. When there's more than one, check whether switching to the async pipe could simplify things.
  • Overloaded pipe chains — more than 4 operators in a single pipe isn't automatically wrong, but it can signal unnecessary complexity. Some operators have alternatives that combine into one what otherwise would need two transformations.
  • Component state leaks inside streams — spotting this inside operator callbacks is a cue for deeper investigation. See if the component properties being touched within the Observable pipeline are meant to be rendered in the template; if so, the async pipe is likely a better fit. Changing this context looks especially problematic unless it's from an imperative third-party call such as FormControl.disable.
  • Subjects under the microscope — these are commonly used to transport data between application sections, but they can also add complexity that no one needs. Consider whether the Subject itself is even necessary.

Final Thoughts

Code reviewing tests both your technical judgment and your ability to collaborate. Sensitivity to detail, lucid feedback, and consistency in upholding standards — these all matter immensely. Hopefully this guide equips you to review code more effectively and catch meaningful issues before they merge.