4

When doing PR reviews, should the reviewer be checking that the code is correct (e.g. the logic is correct, etc.) or should the only focus be higher level concepts (e.g. architectural/functional/etc.)?

I've been doing a lot of code correctness checking in PRs, mainly because we have been seeing some quality issues, and it really drains you, but also I keep catching lots of bugs. So, if I stop then more bugs at least in the short term will likely appear.

I'm not sure if I should be letting this stuff slip through as a matter of process and then if we are getting bugs the developer needs to improve or we need to improve our PR requirements e.g. testing etc. So rather make it a process problem or a hiring problem rather than patching up the real issue by scrutinizing code correctness in PRs.

I'm not sure what the best answer is here.

5
  • 4
    What is more important for your organization - nice looking code or correct code?
    – Doc Brown
    Commented May 29, 2022 at 19:28
  • @DocBrown I never mentioned nice looking code. I said correct code i.e. code that works. If you mean architecture/functional/etc. the architecture is important as well as the functionality. Code correctness is important too.. I'm just not sure it should be the reviewer's responsibility. Commented May 29, 2022 at 19:32
  • Does this answer your question? What is the effectiveness of code reviewing by just reading?
    – gnat
    Commented May 29, 2022 at 20:19
  • 3
    What is the difference between "correct (e.g. the logic is correct...)" and "functional"?
    – Thomas Owens
    Commented May 29, 2022 at 20:38
  • 6
    If you keep finding bugs in code review, you should ask why these bugs slipped by the unit tests. Commented May 29, 2022 at 20:42

3 Answers 3

8

There is more than one way to shave a cat.

In an ideal organization which has a good testing process, and where only experienced developers work who regularly write unit tests, you can leave verification of correctness fully to the authors and dedicated testers. Here PRs can be restricted to verify if there are enough (and enough of the right) unit tests, if the responsibilities between components haven't been compromised, or if a certain change should be refactored.

However, not every organization works this way. If you want to achieve a certain level of quality, someone in your organization has to do double checks and tests. Moreover, if a dev is reading and understanding some PR, they may find potential issues in the code which are unlikely to be found by a tester who treats the code just as black-box. I think it would be inefficient to ignore such issues, sweep them under the rug and just hope that a tester or the author will find the same issue by chance as well.

Of course, when this case occurs, in some organizations the reviewer may be able to ask the author of the PR to write an additional unit test to proof their code works. Or the reviewer may be able to give the tester a hint what they shall test specifically. But in some organizations, the reviewer must be the one who brings proof to the author that the issue they saw in code is a real one (by presenting some test case, manual or automated).

So in short, this is not a black-and-white situation - at the end of the day, you have to work out with your team what works best for your organization, how much testing shall be done as part of the review process and who will take the responsibility for correctness in which case. But what surely not works is saying to them "testing is not part of the PR process" and just hope that someone else will do it.

1
  • 1
    I like this answer. How deeply you inspect code for errors is very subjective. I usually look for obvious bugs — things that are immediately apparent. I will request additional unit tests if I think there might be deeper problems. I'm more concerned about architecture and tests in a code review. Honestly, I spend more time reviewing the tests than the application code. Commented May 31, 2022 at 14:58
4

If you attempt to double check code in PRs you are effectively writing the code yourself and will end up having no time for anything but PRs

Instead decide what the rules for passing a PR are and check that they have been followed. For example.

  1. Are there automated tests
  2. Do they pass
  3. Does the functionality described by the test match the requirements. ie. have the requirements been misinterpreted/misunderstood/skipped.
  4. Is the architectural design been followed. ie has a console app been written instead of an API

1 and 2 check that the person who wrote the code has checked that it worked so you dont have to.

3 checks that what they think "working" means is the same as what you think "working" means.

4 checks we are all heading in the same direction.

You may also catch some gotchas or bugs if you know the code base and problems that have come up before. But you shouldn't be spending too much time checking. That's what tests are for.

-1

A real quick and dirty(?) way to determine if the logic/mechanics of the code is error prone, is to note the amount of comments the engineer puts into his work. I used to be a service manager for a technical company that dealt mainly with electro-mechanical devices, and as a result I would get numerous calls from field engineers wanting help in solving a particular problem. I had to answer the phone, as there was no alternative at the time. But along came email and things changed. At that point, I made the engineers email me, which meant that they would have to sit down and put their issue into words. That meant that they had to sit down and actually think about the problem and what they had done so far to solve it. The number of help requests dropped dramatically after that. Part of it was that some of them were just downright lazy and didn't want to bother typing, but, for the most part, it forced the engineers to actually sit down and think about it. Lack of comments in code is just being lazy and selfish. A lot of engineers don't care if their code works first time, they rely on code reviews to finish their job.

2
  • I can't see how bug reports and code reviews are related
    – Basilevs
    Commented Dec 11, 2023 at 17:04
  • Really? That sounds a bit odd. Isn't a code review supposed to catch "bugs" before they happen? I mean, theoretically. Bug reports are simply practical examples of bad coding, yes? Using multiple "...else if" as opposed to a switch statement is better or worse? That seems like an unanswerable question, primarily in it's triviality. I try to remember that I am always at the mercy of the group that put together the language I am using Commented Dec 12, 2023 at 13:47

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