Take the 2-minute tour ×
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.

Given that the reviewer is not part of the project, but was assigned to review because he has done some coding for the application being updated/enhanced.

Is it the reviewer's job to ensure that the requirement is complete and correct? Or is this the QA and business tester's responsibility?

If the developer and the reviewer misses a requirement which is discovered in UAT, is the reviewer also at fault (probably to a lesser degree)?

share|improve this question
    
What do you mean by "the reviewer is not part of the project", but he has "done some coding for the application being updated/enhanced"? –  Thomas Owens Sep 7 '11 at 13:17
    
I think it depends on the team leader's confidence in the original developer, to be honest. Thus there is going to be no right answer here. –  pdr Sep 7 '11 at 13:21
    
I meant that the reviewer was chosen to conduct review due to his past involvement in the application at hand. He is not actively participating in the project other than code review. –  Carlos Jaime C. De Leon Sep 7 '11 at 13:36
    
@Carlos Jaime C. De Leon -What does one normally do? Every company is different I would say one should do both. Make sure the requirement has been meet and the code meets all accepted standards. We recently had a similar situation where a requirement used the word "all" had the requirements been reviewed during the code review process we might not have gone into testing before fixing it. –  Ramhound Sep 7 '11 at 17:29

6 Answers 6

up vote 11 down vote accepted

It obviously depends on your code review process but personally I'd say the functional completeness of the requirement is more within the remit of the QA / Test team than the code reviewer.

That's not to say that a code reviewer shouldn't be able to pick up on problems with the functionality where they find them (assuming they have the knowledge to identify them), just that it's not their primary aim.

I'd generally see code review as looking to uncover coding errors and inefficiencies, vulnerabilities, memory leaks, that is not checking that what has been implemented is correct against the specification, but that it is "correct" within itself.

If you do want to extend it beyond this to include more of a functional review, you need to accept that that's going to have an impact on the time you need commit to it. If you want the reviewer to be fully familiar with the functional requirements and to be checking them as well, then you obviously need to allow a lot more time for reading and understanding the requirements and checking the code against them.

But it shouldn't be seen as a substitute for proper QA and testing, at most this would be an additional layer.

Broadly speaking:

  • Code review - is the code which has been produced efficient, secure and robust
  • QA - does it do what is outline in the specification
  • UAT - does the finished application actually work within the business
share|improve this answer
    
This is my sentiments exactly, committing more time to code review, which would mean adding more time to the project schedule. In turn the project will be lengthier if code review includes requirements completeness. Is it a normal practice then to not include requirements completeness in code review? –  Carlos Jaime C. De Leon Sep 7 '11 at 13:34
1  
@Carlos - Certainly it's not part of our code review and I wouldn't consider it normal practice to include it. It feels to me a bit like a manager who doesn't understand code review trying to either turn it into something he does understand, or save money in other areas. –  Jon Hopkins Sep 7 '11 at 14:45

If the reviewer spots any problem he should definitely mention them (especially if he is the SME).

But when I receive code that needs to be reviewed I am concentrating at the function level. I see my primary job as reviewer is to make sure the code is readable and maintainable in the future (if I spot bugs I will point them out but I am not going to write unit tests (though if I spot missing unit tests I will point that out)).

Maintainability covers a lot of ground. Some of the things I look for are: style (though anything I point out in style are suggestions and I don't require them to be fixed). Obvious errors or coding styles that can cause problems or that will make the code hard to extend. Algorithms that are very efficient (and there is a simple know better one). Anything that is not trivial self documenting that does not have a description of what the goal of the code is.

It is the job of tests and QA to make sure it does a sufficient job.

share|improve this answer

What you look for in a code review depends what you hope to accomplish it and what type of code review you are conducting. Some code reviews focus on security and looking for vulnerabilities, others look at style and conformance to coding guidelines, while others are just a sanity check for the code. Typically, the reviewer is told (either per guidelines or in the form of a checklist) what kinds of problems the code review is looking for, so their attention can be focused on critical sections.

If you are concerned about high quality, you probably want to introduce reviews at each phase. Requirements reviews can be used to ensure completeness and understandability of requirements. Design reviews are used to ensure that all requirements are addressed by the architecture and design of the system. Code reviews can be used to ensure that the code matches the design and doesn't contain errors before testing, as well as to review the strength of test cases.

When it comes to ensuring that requirements are properly implemented, it's everyone's responsibility, from the requirements engineer to document it to the architect and developers to understand it, design for it, and implement it to the testers who write test cases and test procedures. If anyone identifies an ambiguity, conflicting requirements, or some aspect of the system that doesn't meet a specified requirement, that individual needs to document it and somehow report it (usually by reporting a defect in your bug tracker).

share|improve this answer

Is it the reviewer's job to ensure that the requirement is complete and correct? Or is this the QA and business tester's responsibility?

It is the QA (Software quality assurance) responsibility to ensure that the requirement is complete and correct. Anyone assigned to do that wears tester's hat no matter how large is his developer's badge.


http://dilbert.com/dyn/str_strip/000000000/00000000/0000000/000000/80000/3000/800/83826/83826.strip.sunday.gif

share|improve this answer

It all comes down to what YOU want him to do. But generally speaking, I think of code review as a very specific function. He is there to check for the code. Make sure the code is up to standards and optimally written. If you make your code reviewer do the same thing your Q&A people will do, then you'll be doing double the work for double the time, and the same results. Which is often unnecessary.

This becomes more critical when you have several projects. You can't have 1 reviewer spend too much time on any one project, because Q&A is waiting to test it and you delay the entire process.

Which is why functions need to be clearly defined, as to not duplicate work.

share|improve this answer
    
I agree with the redundancy being introduced! –  Carlos Jaime C. De Leon Sep 7 '11 at 13:47

I'll agree with Jon Hopkins answer that a code review should be mostly about the code, not the requirements.

However, if somehow the basic architecture, design or algorithms of the code under review simply cannot meet the core requirements or the spec, or are highly non-optimal for such, one might need to point this out at the beginning of the review so that much time isn't wasted going down a dead-end path.

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.