What to look for in a code review?

What are your go-to resources for “What to look for in a code review?”

I’m thinking of a range of audiences from people who’ve never contributed to a code review before to people who have but maybe feel like they’re missing something.

I’m not in a position where I do code reviews currently but in the past I always looked at the code review comments for any item I was going to test not only to check what had been flagged up had actually been fixed but the kinds of problems that were being found. I always thought this informed my testing more, gave context to potentially something I might not have realised was being changed and maybe some gaps in the PBI/story/item that needed more attention.

3 Likes

I had to fight hard to get involved in code reviews. The initial reaction of the developers in the team was: “Why would you as a tester be part of ‘our’ code review”. Glad I persisted. As @meowy24 said, it helps to inform your testing; especially when it comes to exploratory testing. I also managed to nip a few bugs in the bud. Some of the things I look out for, but not limited to:

  • Hard coded strings. Can cause potential issues with internationalisation.
  • Values used in the code. Good source of input for boundary value analysis tests.
  • If statements. Determines the path(s) you can take through the application.
  • code complexity. A hotbed for things that can go wrong.

One of the main things I very specifically do NOT look for is code syntax. This is something that should be taken care of by code linters. It is a waste of a reviewer’s time to go through the code to determine if the code is written in the right syntax. A program is a lot quicker and more accurate for that. If your organisation is not using such technology, please push / advocate for that. It’s a quick win, with a massive impact.

As a closing remark for everyone involved in code reviews, please keep them respectful, which makes them more useful

7 Likes

I do not currently understand most of the code my team is writing. I’m OK with that in the short term, I’m doing a short-course (it’s a short 8 hour course, which I have taken 8 weeks to not complete.) I became a scripter and stopped coding a long time back, so most of the new languages are hard for me to read.

But one thing I do know is code smell, code smell is where bugs lie in the code but the devs don’t want to fix them. Code smell looks very similar in all languages. As the non-codeer in my team, I “politely” ask ,“what does that code do”, rather than to say, “that code looks ugly”. If you spotted the right fragment of code, most of the time the developer will reply: “that code is ugly”. And you have it. How to spot code smell, well you can google that. It’s all the things that @pmichielsen pointed out above. It’s things like

  • why has this one function got so many branches or indentation levels (code complexity)
  • why is one function 500 lines long (legacy)? can we make it more unit-testable by breaking it up?

Show interest, but you don’t need to participate directly, and always eavesdrop on your team so you can pick up clues about functionality that may contain bugs. This is much easier if your desk is right amongst them (harder in lockdown I know, but still possible to do online).

4 Likes

Here is an excellent resource on code reviews by Angie Jones. It is NOT about low level coding details like “use descriptive names for your variables” or such. Its mainly about the people or social aspects of code reviews. But, those aspects are very important and are applicable to anyone who does code reviews (even developers, even highly experienced developers in famous companies). I am sure there are plenty of resources on technical aspects of code reviews, but I doubt if those can be covered in one or two short articles.

6 Likes

Wrote this back in 2014 and mostly it’s still relevant - http://simon-prior.uk/2014/06/27/overcoming-the-resistance-qa-involvement-in-peer-reviews/

For me it was always about understanding the code enough to know what’s going on, so it could influence my testing and give me ideas of what to try.

Also output of static analysis and coverage tools showing where Unit tests covered code and any identified issues were good resources to have at hand when reviewing the code as it gave you talking points with the developer

6 Likes

I’m a programmer, so possibly have a different perspective on this to testers, although I agree with everything that people have already said.

For me, the most important thing is attitude or respect (as others have said). Without trust and openness, the review is much less effective and you’re almost wasting everyone’s time. It’s an instance where the Golden Rule applies: do to others as you’d like them to do to you. I use a football (soccer) analogy for a decent tackle: go for the ball, and not the player. The code / design / test / etc. is under review, not the author.

I agree with other people that it’s worth remembering that a review is part of a bigger picture that also includes compilers / linters and tests, so focus on the areas where reviews can do things that other things can’t. A big part of this is spotting things that are missing, or spotting duplicate code.

I also point out code that’s hard to understand (which is related to, but not the same as, things like code complexity and code structure). If it’s hard to understand, it will be hard to debug, hard to change in the future, and hard to know it’s correct (which isn’t necessarily the same as passes its tests). I realise that not everyone in a review might feel confident to say this, e.g. if they’re a relatively junior tester, but it might be possible to phrase this as: Please can you explain this bit of the code to me? If the programmer has trouble explaining it, that’s a warning sign. I know that sometimes understandability needs to be sacrificed to get other things e.g. speed, but that should happen only when it’s needed and not as a default (no premature optimisation).

I do point out typos, not because I’m picky, but because our tools are generally stupid. For example, if your system deals with shopping carts, and there’s a method called AddToCrt rather than AddToCart, then if someone’s searching for cart-related things later (because e.g. they’re changing how carts work) then a search for “cart” won’t find AddToCrt - even a case-insensitive one.

6 Likes

I am not a coder and I can barely read any code that’s not SQL, but I look at code often. I look for developer comments: I look for large sections of code commented out, for TODOs, and other interesting remarks or just the way some comments are worded that can show lack of interest, lack of effort or plain old laziness.

Here’s an example of 2 things I found in code for a web app that’s currently live and anyone can access. This was during a practical training class.

5 Likes

Things I tend to do:

  • Review the story to get an idea of the scope of what should be in the PR (I’m guilty of skipping this more than I should and end up following the PR opener further into the weeds sometimes)
  • Look through the implementation to make sure it’s sane - most of the code I review these days is Java while my primary language is Python, so this started off mainly as shape of the code. I also use this to ask questions about things, most commonly about whether something is idiomatic for Java (e.g. recently I was asking about no-arg constructors, and if the pattern was more often to have this instantiate objects with default values or nulls)
  • Review the unit and integration tests to see that they’re testing the public methods as well as thinking about if there are cases that should be added
3 Likes

sometimes for dev code reviewers, some standpoints i like to look at are:

  1. is modelling the business concept correctly. the closer the code can match and model to business domain the less likely to have larger bugs and overall be more easily maintainable
  2. simple things like if bug like to see that there was test case added to avoid future regression issues
  3. especially within more heavy business logic portions of code (eg in finance many different type of financial calculations) like to review (as can easily find bugs easily and fast that may take longer or hardre from actual exploratory testing of api endpoint/ui/etc)
  4. sometimes just to see what portions of code were changed (are we solving the problem in correct area). for example we have separation between app and more core platform code (are we putting items within app that would be better served and re-usable if put in core platform).
  5. are we following previously established standards and practices
  6. the biggest issue usually see is when enhancement affects a specific feature but should be thought of in more general enhancement or coded in way to promote things like re-usablity and such, especially if enhancement points to something that think more broadly, having a “patchy” enhancement (ie just to specific area rather than something more holistic) is something that an happen often. overall want to promote higher maintainability
  7. does it handle failures gracefully (eg if using other api services how does it handle if those error, etc). ensuring the system is robust to failures (especially in world of web apis where are likely to expect network, etc failures from time to time)
2 Likes

I’ve not done too many reviews over the past couple of years but I tend to start with the changeset text.

  • Have they explained their goals and design choices?
  • What testing has been covered?

I’d then consider the unit tests. Do they have decent tests? Are there any gaps jumping out at me? If there are, I’ll try and find the code of interest first. Sometimes I’ve been pulled in to review code I just don’t understand. This is where testing, changeset description and comments come in.

One key thing that I believe people can overlook is logging and testability. If an edge case is caught and handled, will we know that was the case? Often there’s nasty race conditions and if we don’t log that it was handled, how will we test that it works?

My best advice for anyone doing code reviews is to ask questions. It is better to ask a dumb question than make a dumb assumption. Also look at the testing notes and unit test coverage. Challenge it if you think it is lacking.

One other thing that I like to do if I’m familiar with the code base is to look at the task before the code, think of what I’d do then view the code. If it’s different, look at why.

2 Likes

Wow Richard, those 2 rules actually blow away lot of code review process “fluffiness” and get to the meat of why we code review.

You reminded me that code reviews of each change are a bogus concept which does not really reduce bug injection rates if we do it as a “merge exercise” only. Or even if we do it in whole big bang per module thing, without clear goals and a balance of raised visibility and of testing work to go with it, code review is best if it is not an isolated activity.

Code/line/decision/ … coverage is Bullsh*t

This is why mutation testing exists. An automated review of your unit tests. (covers a lot of the basics in order to have very good unit tests) – Still requires manual effort of course to review the code but it’s such a great help.

But yea totally agree! :slight_smile:

1 Like

I think one important thing when doing a code review is having a written guide which points out the important parts that should for sure be covered in a code review. That is because depending on your circumstances you might have different points that have a high value for your team. For instance you could have customer sensitive data that you for sure don’t want to be leaked. Moreover if everyone does the codereview freestyle some important points might be forgotten.
That said here is what I would like to have in my code review:

  1. Use static code analysis tools - are there any possible flaws in the code pointed out?
  2. Does the code fit into the designed architecture?
  3. Is the code written in a way such that it’s easy to test it?
  4. Is the code written such that the intention is clear? If not what could be done to improve it?
  5. Was the documentation and the changelog updated.
  6. Are the written tests sufficient and are they written in a way such that it’s clear what they intend to test.
3 Likes

Excellent points @samuelm. I would hope that the written guide for reviewing was a more general guide e.g. also used by developers. I’ve worked on systems that dealt with sensitive data as you mention, and leaking it was as you said Concern Number 1. When I worked on billing systems there didn’t need to be such stress on sensitive data, but correctness of calculations was Concern Number 1 (followed by speed and scalability).

There needs to be a shared understanding of the context - what’s important to the customer / user, and what’s important internally. This can be used during code review, but also during programming, testing etc.

3 Likes

First of all thank you all for posting here about this topic, because today it motivated me so I started to reorganize our review process. So thanks a lot​:heart_eyes::heart_eyes:

Anyway when I was doing my research today about this topic I stumbled across another interesting thing to consider about reviews:

Be a human:
Also give compliments in a code review where appropriate. For you it might be a small thing but for the other person it might make the day.

Really loved that so I thought I will share it here!:sunglasses::sunglasses:

4 Likes