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

6 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).

3 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

5 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.

4 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.

3 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
2 Likes