4
\$\begingroup\$

I was wondering what should be the focus of review. I thought the focus would have been more on:

  • correctness (no syntax or logic error)
  • readability (clarity, respect of coding standard ...)
  • robustness (ability to handle edge cases)
  • performance

I've noticed, instead, that the discussion sometimes shifts on the fact that there might be other solutions that might be "better" without pointing to specific flaws of the code under review.

What do you think?

\$\endgroup\$
4
\$\begingroup\$

I think a good question will point out the flaws in the original code. Its okay to point out alternate and better ways, but you should really point out what was wrong the original.

\$\endgroup\$
6
\$\begingroup\$

The review has to be driven by the situation of the poster and the shape and quality of the posted code. Sometimes the poster's approach is basically fine and the appropriate things to review are documentation, variable naming, layout, and so on. But often the overall strategy is poor and the review would be failing the poster if it didn't reflect that.

It's important to keep an eye on the big picture. Code is only a means to the end of solving somebody's problem, and if the posted code is not solving that problem very well, there's no point in getting down into the details. The biggest issues have to be tackled first. For example:

The need to rethink the algorithm often arises for optimization questions. Big speedups mostly come from finding better algorithms: replacing an \$Ω(n^2)\$ algorithm with one that's \$O(n)\$ usually beats any amount of loop unrolling and strength reduction and other piecewise optimizations. Some examples where merely discussing the poster's code would have been a gross disservice:

\$\endgroup\$
1
\$\begingroup\$

First, the focus of code review should not be correctness, at least not on the surface level. The code should should be real, working code, which means it has to compile and there should be no obvious logic errors. (Reviewing for correctness in edge cases or the like is okay though.)

But if you want the reviewers to focus on a specific aspect of your code, you can ask for that. In fact, I think you should do that, posting just your code with no explanation of what kind of review you're expecting is less than ideal.

\$\endgroup\$
  • \$\begingroup\$ When I say "correctness" I mean that the review should point out that the code, which is believed to be correct from the author, actually is not. What can be "edge" or "obvious" depends on the level of the programmer/reviewer. \$\endgroup\$ – Remo.D Apr 19 '13 at 7:19
  • \$\begingroup\$ @Remo.D Yes, but a syntax error (which you mentioned) should be always on the “obvious” side. \$\endgroup\$ – svick Apr 19 '13 at 7:47

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .