Tell me more ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.
up vote 6 down vote favorite
2

How much should a reviewer trust the coder who submits the code for review?

I always have that dilemma of whether I should go test of the changes proposed or should I trust the submitter that he would be knowing what he is doing? Mainly because at the end of the day its reviewers responsibility also.

Code-reviews are not fun always.

share|improve this question
No unit test results? Why not? – S.Lott Jul 12 '11 at 18:44
@S.Lott: Good hint. But thats again pretty tricky. I (being a reviewer) might not know what all test-cases are needed to cover all possible corner-cases. – hari Jul 12 '11 at 18:49
1  
It's not "tricky". Unless you're God, you can never know if all test-cases are needed to cover all possible corner-cases. Since you can never know this, it's not "tricky". – S.Lott Jul 12 '11 at 18:51
If the reviewer asks for his code to be reviewed, by all means, test it! – rlb.usa Jul 12 '11 at 19:08
Remain impartial. You wouldn't want testers not to test just because of some developers rep to always get it right and then miss something important that blows up in your most important client's face. So test, but don't take on the testing responsibilities of the programmer nor those of the testing team. When doing reviews I run a couple of cases to ensure that the testers/QA do not have to send it straight back to development, but the programmer remains responsible for being confident that the testers/QA won't find anything. – Marjan Venema Jul 13 '11 at 7:14

5 Answers

up vote 5 down vote accepted

No, you don't need to trust them. You were asked to review the code do so, it is not the same as testing it:

  • Is the code clear and easy to understand? (This is my primary concern.)
  • Does it comply with site standards?
  • Does it comply with best practices?
  • Does it do what it needs to do, and only that?
  • Are there edge cases which are easily caught by code inspection? (Off by one, incorrect limits, missing default case, unintended or undocumented case fall-through, etc.)
  • Are there ways it could have been better written?
    • Can you enlighten the developer on better approaches to use on future work?
    • If the code works, it may not be appropriate to apply the changes to the current code?
  • Did they do anything that caught your eye as interesting?
  • Anything else covered by your review standards?
  • Do note things they did well in addition to problems?
share|improve this answer
1  
Excellent answer. Put another way, the purpose of code reviews is to catch the things that testing can't catch. – Karl Bielefeldt Jul 12 '11 at 21:38
@BillThor: Thanks. I strongly disagree to the point that reviewer should thoroughly test the code. It's not and should not be the job of a reviewer. At the best, I can ask the submitter to run a few test (which I find useful and yet not covered) and attach the results. – hari Jul 12 '11 at 22:45
@hari, no the reviewer should not thoroughly test the code, that is the job of the programmer. However I do agree that the reviewer should at least run a couple of test cases, common scenario's, to ensure that when the code is passed to QA for testing/verification, it doesn't come straight back... – Marjan Venema Jul 13 '11 at 7:08
If by "thoroughly test" you mean write additional tests, then no, the reviewer ought to fail the code instead, suggesting additional test cases. If by "thoroughly test" you mean the reviewer ought to run all the tests, and be reasonably sure that there's decent coverage, then yes, the reviewer should "thoroughly test" things. – Frank Shearar Jul 13 '11 at 8:00
up vote 2 down vote

If you don't trust the person to test their code, what really can you trust them with? If you can't trust them to do that, do you honestly think that you can trust them to do any post-code review follow ups?

Sure, one can say that just because they tested it, does not mean that they tested it well. But that leads to a different question--do you trust them to test their code well?

If you have concerns about how well it was tested, think up all the failure paths you can think of, and all the corner cases and keep an eye out for those in the code review, or during your pre-code review inspection (make notes). If you have concerns about the cases you have thought up, ask about them. If they can not be answered to satisfaction at the session, then the developer must follow up on them; make sure to be copied on the results of the investigation.

share|improve this answer
up vote 1 down vote

Code review should be conducted against a set of measurable criteria. I don't think trust comes into it. The source code and accompanying documentation should be open. The questions to the coder should be frank as well.

If the attitude of the code review and the participant is not open, frank and without egos, then it is not a true code review.

share|improve this answer
+1 for the straight forward statement in bold. – jwernerny Feb 24 '12 at 13:43
up vote 0 down vote

It depends wholly on your organization's procedures and what process control measures you have in place. If, for example, you employ unit testing, then you could check the total code coverage as a measure of how much to "trust" the code.

Truly, there are far too many factors to weigh, many of which are highly subjective, for there to be any hard and fast rule for this. I definitely take into account who is checking in code - I'm much less nitpicky with code checked into by a veteran than I am with a fresh college kid.

share|improve this answer
up vote 0 down vote

Depends on the size of the code changes to my mind:

Extreme 1: Small changes - If there is a bug being fixed by changing a few lines of code, then I'd generally like to show a before and after execution of the code just as a standard part of the review. This could be taken as paranoia on my end, but it just makes sense to do this usually.

Extreme 2: Big changes - If there are dozens of new classes being added, then it may be a few hours just to go over the design so that demonstrating all the functionality isn't a realistic thing to do. In this case there is some trust in getting the finer details right as well as some trust that the higher level details can be communicated in a somewhat concise manner as I don't want to have to go read every line that was added.

In between is where 99% of code changes are made and there is a bit of a judgment call as to where the line is. Generally I like to see some working cases with a few boundary cases if it is realistic to do a basic smoke test of the new functionality.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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