6

I have been given the task of reviewing a change request comprising of more than 20 changes, the problem is that we do not / cannot install a version of the software to be evaluated as the development environment cannot be easily re-created or installed. My question is, aside from just reviewing language grammar, is it really effective to review changes just by reading the code instead of by reading, running the code, and reading again?

  • 2
    The answer's in the name - code review. The reviewer goes over the code and tries to find obvious code smells and give advice regarding improvements and best practices. To prove the code works correctly, use tests. – idoby Mar 20 '15 at 9:59
  • 15
    "the development environment cannot be easily re-created or installed" - sounds like you have already found a priority issue without actually running the code ;-) – Doc Brown Mar 20 '15 at 10:12
  • 4
    This is know as a Fagan inspection. Studies have been done as to is effectiveness that are findable once you know it's technical name. – user40980 Mar 20 '15 at 11:31
13

Where I work, the author of the code changes is responsible for testing that those changes actually work, while the code reviewer is typically responsible for finding any issues with the readability/maintainability of those changes or spotting corner cases the author might not have thought to test for. In general, the reviewer looks for the sort of problems that the author is most likely to miss precisely because he wrote it. So it's completely okay for the reviewer to not test the changes, as long as the original author did test them.

And when the change is not to UI code, there should be unit tests, so it is also the author's responsibility to add or change a test to prove the code works as intended, and actually run the tests to make sure they all still pass. The reviewer will then check for readability/maintainability issues or missed corner cases within that test as well as in the "real" code changes. Though running a subset of the unit tests is so quick and easy that I usually do that myself when I'm reviewing something, even though I technically don't need to.

There are occasions where one of us really wants the reviewer to test it, because it's the sort of UI change that can't be effectively tested in any automated way, and at least two people need to bang on for a minute to prove it didn't break anything. In those cases we explicitly ask the reviewer to do that.

|improve this answer|||||
  • 5
    and of course the tests should also be reviewed as part of the code review process. I've often found flaws in tests that caused them to pass when they shouldn't, leading to production errors that can potentially cost millions. – jwenting Mar 20 '15 at 10:25
  • @jwenting Completely agreed. Once I had a test to make sure foo(A) returned true, which passed...because foo() had a bug that made it always return true. Now I always write "control" tests when working on that module. – Ixrec Mar 20 '15 at 15:54
5

is it really effective to review changes just by reading the code (...)?

If you have enough practice coding you will be able to spot bugs without software/coding environment. It's good practice to read raw text and try to pick out the errors (logic or syntax). It will help you become a stronger programmer.

|improve this answer|||||
  • "you will be able to debug without software/coding environment" -- how's that? – gnat Mar 20 '15 at 7:07
  • 2
    @gnat surprised you ask. Obviously, if you have enough programming experience you can debug code by thinking about what happens and detect logical mistakes without actually running the program. – miraculixx Mar 20 '15 at 7:15
  • @miraculixx that doesn't sound like a typical debugging process – gnat Mar 20 '15 at 7:18
  • @gnat the OP was about effectiveness of code reviewing by just reading, and this answer simply states that, well yes, given experience it is effective because you will be able to spot bugs before they need to be reproduced. If removing a bug is not _de_bugging, I don't know what it is. – miraculixx Mar 20 '15 at 7:34
  • 1
    @gnat I've done it successfully multiple times with other peoples' code. The sources of many kinds of bugs have some telltale patterns. Nobody said that this is the "typical debugging process", but it still is entirely possible and useful if you can't run it, for any reason. – Theodoros Chatzigiannakis Mar 20 '15 at 9:42
4

Code review is mostly about the readability and maintainability of the code. You also have a sanity check on whether the changes relate in any way to the actual change request.

If it confuses the code reviewer it will probably confuse any one who has to amend the code subsequently.

Yes code reviews can pick up other potential bugs, but most of these could be picked up by various source code analysis tools anyway.

The actual major benefit of code reviews is the coders write better code if they know another human is going to read it and ask "why did you stored the sales tax in a field call "bonus"" type questions.

|improve this answer|||||
4

In our team, the style and general coding rules are checked by code analyzers with every build. Therefore when we review the code, mostly the code written by juniors, we try to spot the parts that does not follow the architecture and design patterns because these cannot be checked automatically by tools easily.

We don't run the code because it is mandatory to write tests and have a code coverage above 80%. But we also review the tests as much as the implementation. This is the most beneficial part to discover possible design problems.

If a unit test is not clear and it is bulky, generally the implementation is also not ideal. So we tend to review tests first.

|improve this answer|||||
3

I would argue if there is a piece of code you need to run or debug first to understand what it does, the code has to be improved in terms of readability, and the review which has revealed that was quite effective.

|improve this answer|||||

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