17

I'm a Software Engineer who sometimes need to review the code of my fellow team members. I often look at the source code and think; this looks fine. I'm having a hard time to think critical about it. I can be very critical when writing code myself since I'm active/highly involved with the code, but looking at it more passively I'm not very good in reviewing the code. Aside from that I run the code, test it, it looks quite good for me. Any idea's how I can be more critical in Code Reviews?

4
  • Assuming you didn't mean it, you may want to rephrase your title to avoid the implication that you would like to be critical of people. Dec 4, 2023 at 15:37
  • @PhilipKendall easily addressed in the answer. And I think makes this a more interesting question. Dec 4, 2023 at 15:40
  • 10
    Not intended to be critical of people, it's about the code.
    – Jay
    Dec 4, 2023 at 17:23
  • 13
    I think it might help if you clarify why you think this is an area where you need improvement. Have you missed things? Have there been bugs or other problems that could be addressed in the review but weren't? One of the biggest challenges I've seen in code reviews is that they can be hyper-focused on trivial details. Being able to look past such things is an asset, IMO.
    – JimmyJames
    Dec 4, 2023 at 18:55

8 Answers 8

33
  1. Look for anti-patterns.
  2. Lint the code if it has not already been done.
  3. If something looks really strange, talk to the developer before proceeding--maybe they are not ready to have their pull request examined.
  4. Look for hard-coded values that could have been put into environment variables, arguments, or some kind of config file.
  5. Look for security issues (eg: hardcoded passwords, keys in the repo, opportunities for SQL injection attacks, etc).
  6. Look for functions that could have been made more generic and reusable.
  7. Look for silly custom code that does something that a built-in language feature does better.
  8. Check any comments and ensure they make sense.
  9. When referring to someone else's code, be sure to just say "this code" or "the code", rather than "your code"; it's just better etiquette.
7
  • 31
    2a) Automate linting so it doesn't need to be done manually during code review.
    – Bergi
    Dec 4, 2023 at 23:04
  • 2
    excellent point @Bergi an automated CI/CD system like Jenkins would ideally be linting and running unit tests Dec 4, 2023 at 23:08
  • 9
    10) Think about how this code scales, eg what would happen it usages increases 10x or 100x. A reasonable amount of futureproofness should exists
    – Martijn
    Dec 5, 2023 at 9:02
  • 6
    11) Keep an eye out for deviations from the company's coding style guide which aren't covered by Lint - e.g. naming conventions. Dec 5, 2023 at 10:10
  • 2
    0. Make sure you understand what the code does (or is supposed to be doing). 6a. Make sure the code is "defensive". What happens if there is an error? What happens if the input is not in the expected format or something is missing? Make sure input is not trusted. 6a. Look for duplicated code (in the addition, and compared to the existing code) 12. What happens when input strings stray out of pure ASCII? When a french user types an accent, or someone types an Emoji? Or one of <>&'"$!%? For text inputs and outputs, what are they supposed to be? Actual text, or HTML?
    – jcaron
    Dec 5, 2023 at 15:27
13

A code review needs to find and point out any major flaws:

  • Does the code use the wrong approach to solve the problem? Do the design patterns fit?
  • Does the code actually solve the problem? Does it meet the functional requirements? Ideally there will be tests showing that it does, but the closer you get to the user (interface) the harder this is to verify.
  • Are there any (subtle) bugs that lead to failure? Look for missed edge cases, input parsing/validation and exception handling in particular.
  • Is the code clear and understandable? Is it documented well enough for you to immediately understand what it does and why? Are the explanations reasonable?
  • Does the code have any security issues? Missing authorisation checks, sql injections, all the non-obvious small things causing vulnerabilities. Scrutinise every single line, but also look for holes in the large picture.
  • Is the code efficient (enough)? Is there any missed optimisation potential, will it work with large inputs, does it introduce any bottlenecks?
  • Does the code fit into the rest of the application design? Does it duplicate some code existing elsewhere, is there any potential for reuse, should functionality have been generalised? Will a refactoring of other parts become necessary?

How much to do on each of these points depends a lot on your line of work and development workflow. If your team is doing sufficient technical planning together, the right approach would have been determined and agreed upon before the work started, so you only need to check whether that was actually implemented. If there is a dedicated QA team who will check the acceptance criteria to be met, you don't have to do that part. Identifying problematic edge cases is easier when looking at the code than when doing an end-to-end test, though.

Don't trust tests. Focus on the things that have not been tested automatically. For the parts where tests have been written, verify that they actually check the right things, match the test description, and that the assertions are correct (and would catch wrong results).

Regarding architectural/design decisions and non-functional requirements, experience with the whole application is required for the review. If you feel you are still missing that, code reviews serve as a learning opportunity. In addition to browsing the relevant (old and new) code, ask questions!

Finally, if you feel you are not as critical with their code as you would be with your own code, you could try pitching your own solution against theirs. Before reading the code to review, look at the requirements of the ticket. Try to come up with your own approach: what functions do you need to change and how, what new classes and methods would you introduce, how would you name them and what would they do, what patterns would you apply? You don't have to actually implement it, but get a good enough picture so that you feel you could start writing down the code of your solution.
Then, compare your approach against the code that was written. What have they implemented differently and why? Is either approach more straightforward? Did you break down the problem in the same way? What edge cases have you missed to identify, what edges cases have they missed?
Do not try to argue that your solution is superior, but focus on the differences. They will give you the insights on the potential flaws that you can criticise.


Apart from the major flaws that you need to look out for, any code will have lots of small flaws. Point them out or just propose a change (or even just do the change yourself if it is faster and uncontroversial). Do not start arguing, they are often subjective and have rather low priority.

Depending on how valued code quality is in your organisation, they may even be considered irrelevant to the code review. In the code of a perfectionist, you may rarely find any, but everybody has bad days. In either case, while reading through the code, you will come across

  • badly chosen names
  • violated naming conventions
  • code style and formatting issues or inconsistencies
  • typographical mistakes
  • inconsistencies in handling of values
  • misapplied patterns
  • overcomplicated code that could be simplified with standard library methods
  • hardcoded values that should have been named constants
  • leftover debugging commands
  • duplicated code that should be refactored
  • unnecessary changes to unrelated components
  • etc.

They may not all be significant, but finding them is a sign of having focused on the code with a critical mind instead of just skimming over. Some of these problems can be detected automatically by tooling and fixed before the review, but many (anti)patterns are too subtle or impossible to codify (let alone to match deterministically).

Code that "smells" also has a higher likelihood of containing a well-obscured major flaw. Look for these in the most unclear sections of the code. Over time, you will develop a hunch for such bugs.

10

I would suggest:

  1. Have more than one person review a PR - you can compare the issues that you found to what someone else (preferably a senior engineer) found. Talk to the other reviewer to understand why they felt that item was important.
  2. Spend more time on the review - It can be difficult to find complex issues without spending time to really understand:
    • What the code did before the PR.
    • What the change in the PR is actually doing.
    • Any complex interactions as a result of the new code.
  3. Understand what your company is looking for in the code reviews. One company/culture may consider a particular issue important, where as another may consider it to be "nit-picking".

For large PR's I will sometimes go back to the original commits to understand what each commit did - this is especially valuable if the original author did a good job with their commits (each commit is one logical change).

I don't do it every time, but there have been times that I have checked out the code, built it and then stepped through some test cases. The key point is that to do a really good job of reviewing some code, you need to really understand what it is meant to do and how that differs from what it actually does.

Note: What is "does" and is "meant to do" is more than just functional correctness, it also applies to performance, security and maintainability of the code.

1
  • 1
    To add to 1, it might be worth your time to look through some previous PRs that were reviewed by someone whose review feedback you think is valuable and try to understand the kinds of things they leave feedback on and why they gave that specific feedback Dec 5, 2023 at 12:26
7

A few points that have not been covered yet:

  • Is it maintainable and extensible (e.g., uses interfaces, collects configuration values in a visible spot instead of scattering them);
  • Is it testable (e.g., uses dependency injection instead of concrete classes);
  • Is it intelligible? E.g., is all non-obvious code well commented, is it only as complicated as necessary);
  • My pet peeve: Does it avoid code duplication? This affects maintainability, obviously, but also intelligibility: Factoring out common code makes it clear that the same processing is applied in all those places, but also makes the differences obvious, usually by parametrization;
  • The code must compile cleanly. Compile with "all warnings" (e.g., /W4 with VC, -Wall and perhaps -pedantic with gcc/clang) and demand that there be no warnings. The reason is that, while many of the warnings are not indicating problems, some do; and eliminating all of them makes sure you catch those. As an example, all narrowing conversions in C/C++ should be explicit, with a comment why the vales cannot overflow, or else a test ensuring that they don't.

For many of the above goals, there is a sweet spot between violating and overdoing them. Simple, straight-forward code is easy to debug but too redundant; high abstraction may be easy to extend but hard to debug and understand; a simple approach is too wordy and does not make clear what's happening, but a complicated one is, again, hard to understand and prove. Too many casts and comments may impair legibility and obscure control flow.

Often it is a matter of taste where one positions code on the various criteria scales, and the reviewer, in my opinion, should leave those matters of taste to the coder. Only obvious violations should be flagged (with the potential exception of compiling without warnings).


While I'm at it, let me digress a little and talk about the people involved.

  • It should be clear that a review comment is not a personal criticism. Different cultures hand out and take in criticism in very different ways. A German reviewer has no trouble pointing out error after error with a stern voice without thinking any of it, while an Asian may start a review of the same code with "good job" and then make some modest suggestions, always with a smile and an apology. A German receiving a review may think the reviewer is an idiot while an Asian in the same position is wondering whether they should change their career to gardening because they are obviously useless as programmers. I think politeness, humility, self-deprecation, a "No stupid questions" attitude and humor take away some of the tension inherent in a review.

  • The primary goal of a review is, of course, ensuring code quality. A secondary, but arguably even more important goal is to enable the programmer to write better code. The opinion of a colleague with a fresh perspective regarding a specific piece of code I have written is invaluable and cannot be gleaned from books. When I see code that is not wrong but also not great as a reviewer, I sometimes make non-mandatory remarks or suggestions (which are sometimes actually picked up by the coder). Such remarks can be made outside of the review system (e.g., personally or in an email).

  • Perhaps less obviously, the review is a learning opportunity for the reviewer. Reviews are dialogs. The reviewer may be as wrong as the coder. Hopefully, there is some great code to learn from.

Good reviews are costly. The time and effort spent should be utilized.

6

How to be (more) critical during Code Reviews of team members?

Don't be critical of team members. A code review is about code. Not about who wrote it.

I'm active/highly involved with the code, but looking at it more passively I'm not very good in reviewing the code.

Think of it as your code. The literature calls this taking ownership. When you review code think of it as having your name on it because it does. You're turning it in as your work as well because you approved it.

It also helps to maintain low friction when doing reviews. The longer I wait for reviews of my code the harder it is to address them because more of the code has leaked out of my brain. If you want me to change code, get the review to me fast.

Be sure to communicate why, not just what. You don't have to be an authority or a gate keeper. You're a co-pilot pointing out possible problems that should be considered.

Take that attitude and I'll always welcome your review.

5

Don't worry too much about it but definitely strive to improve your code reading skills.

Identifying code smells requires experience. Therefore by its very nature critiqing (rather than being critical of) code is a matter of experience. What kind of experience? Debugging.

Fixing a lot of bugs will slowly train your brain about what kinds of code annoys you. That annoying feeling in your gut is the experience of "smelling" bad code. Over time you will be triggered by certain kinds of coding patterns (not necessarily design patterns) and this forms your personal opinion of what is good and what is not good.

Experience will also inform you of what kinds of patterns don't matter - weather you use a switch or an if-else sometimes are equivalent but sometimes one is much easier to maintain. Again, this takes experience.

But like any kind of learning, being deliberate about it - actively looking for what kinds of coding patterns lead you to fixing bugs - can accelerate the development of your skill. So next time you go bug hunting and find the root cause, look at the surrounding code and how they may have contributed in the bug being manifested. Additionally, like any skill, learning from those who come before you can help. So read other people's opinions but always with a pinch of salt: it's OK if you don't agree with everything other people think especially if people themselves don't agree with each other.

PS: As a side note, being critical of your own work may not be the skill you're looking for. It may be born out of insecurities or the imposter syndrome. What you want is to be critical of all code - to have your own opinion.

1
  • 4
    As Douglas Crockford is fond of saying: style is a matter of taste until that style lead to creating bugs.
    – slebetman
    Dec 4, 2023 at 22:59
2

I can be very critical when writing code myself since I'm active/highly involved with the code, but looking at it more passively I'm not very good in reviewing the code

Then involve yourself with the code?

One method I use is asking myself: "If I had done this, how would I have solved it?", and then compare it to what they have written, and investigate the differences. Why did we do it differently? Which way is better? And why?

Your team may also have a coding standard / definition of done that details specific things to check.

2
  • Interesting take. This is good for ensuring we're not missing an obvious way to implement stuff.
    – justhalf
    Dec 5, 2023 at 5:22
  • 2
    This sounds like an excerpt from my answer, +1 :-) I've just hidden this too well amongst the other points in my essay, but it is the direct response to the lack of involvement
    – Bergi
    Dec 5, 2023 at 9:45
1

During PR code review we seek to answer a pair of questions about the code.

  1. Is it correct?
  2. Is it maintainable?

Answers of "yes, yes" suggest that we should merge the proposed edits to the main branch. If not, there is more work to be done, or the PR should tackle a problem of reduced scope.


is it correct?

A chunk of code is an argument, presented to the reader for their consideration. Sometimes an eloquent argument.

We see this all the time. An essay, a geometry theorem, a recipe, a poem, a function, each one lays out an argument and invites the reader to agree with it. The author shoulders a responsibility, not to be barely correct, but to be obviously correct. We should see overwhelming evidence that this way is definitely the right way.

In code we start by spelling out the contract a function fulfills. We can do this in the signature, with carefully chosen identifiers for function name and parameters. And also in formal documentation such as /** javadoc */ or """docstrings""", or less formal documentation such as # comments. Also with assertions run by production code, or with conditions verified by an automated test suite. All of these things can instill confidence that the argument presented is a correct one.


is it maintainable?

An author is proposing edits to a shared codebase. You will be maintaining that codebase. The edits will become your code.

Sometimes things break. At midnight. Or on the weekend. Would you be able to fix this code, under stressful conditions? Is the code correct? Is it obviously correct? Do you understand what it does? Could you diagnose and debug it? Could you confidently refactor or repair it?

This is a high bar. If the proposed code does not reach the bar, then do not approve it. Request re-work. Describe your concerns. Collaborate with the original author. Suggest a better solution or a reduced scope.

In the end we wind up with a code artifact that began as the author's code but ultimately is the team's code, which the team can support.

Not the answer you're looking for? Browse other questions tagged or ask your own question.