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?
-
2The 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.– idobyMar 20, 2015 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 BrownMar 20, 2015 at 10:12
-
4This is know as a Fagan inspection. Studies have been done as to is effectiveness that are findable once you know it's technical name.– user40980Mar 20, 2015 at 11:31
Add a comment
|
5 Answers
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.
-
5and 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.– jwentingMar 20, 2015 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.– IxrecMar 20, 2015 at 15:54
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.
answered Mar 20, 2015 at 8:16
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.
-
"you will be able to debug without software/coding environment" -- how's that?– gnatMar 20, 2015 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. Mar 20, 2015 at 7:15
-
-
@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. Mar 20, 2015 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. Mar 20, 2015 at 9:42
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.
answered Mar 20, 2015 at 9:03
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.
answered Mar 20, 2015 at 12:40
