9
\$\begingroup\$

Code Reviews tend to be a bit of a negative thing. We spend a lot of time pointing out problems in other people's code (and rightfully so). However, a good code review also points out what was done well. Should we put more effort into telling people what they've done right too? What are the benefits of it?

\$\endgroup\$
8
\$\begingroup\$

We are all here to write good code. We should be told when we have succeeded in that!

One day I would really like to write something like:

You have made a nice frobnicator here, I like how you used the fizzler to buzz it. Additionally, your naming is awesome and your method interfaces are cleanly written. Now go out there and write some more awesome code for me to review!


There is just something special about hearing things such as:


Even if all the rest of what you are saying is negative (whether it's very negative or just nitpicks) then you can always say that they got it to work, which is an important achievement itself

Sometimes when there are truly no positive things to say then you can say that you would like them to come back, because you would like to write a more positive review next time.


One of my all time favorite quotes is:

Remember compliments you receive, forget the insults. If you succeed in doing this, tell me how
- Baz Luhrmann "Everybody's Free (To Wear Sunscreen)"

If we give more compliments, it will be easier to do that.

\$\endgroup\$
2
\$\begingroup\$

When someone asks about a very specific part of their code, I think one of three options are appropriate:

  1. Don't address it at all because you're not sure about that specific part.
  2. Address it and tell them how it could be better (because it needs work).
  3. Address it and relieve their concerns by telling them they've done it in the most appropriate way possible.

However, we should be careful about the third option.

By using option 1 and not addressing it, we effectively tell the asker "I can't see a better way to do that," and at least assure him that someone else agrees that's an all right approach. However, it doesn't confidently lock him into that view point.

If I know two approaches, approach A and approach B, and the asker has used approach A, but I know approach B is objectively better (easier to read, runs faster, uses less memory, best practice, more common way for the language, etc), then I can offer approach B as an answer.

In fact, I can offer approach B as a definite improvement over the current approach without claiming approach B to be the best approach available (there may be an unknown-to-me approach C).

However, if the ask has used approach B, and again, I know approach A and approach B, and approach B is objectively better than approach A, what should I do?

I could comment and say "Well, that's better than approach A"... but how constructive is that?

Variables with actual names are better than variables with single-letter names. Is it useful for me to point this out to every questioner who managed to use more than one character for his variable names?

And here's the problem. What if there's an even better way of naming those variables, but I, with my limited experience with the language, or with what he's doing, cannot see it. To me, he's used the best possible variable names. To another, his variable names are still just as bad because they see these other variable name options out there.

....

I'm getting rambly...

If you take nothing else from this answer, the point is, given Approach A and Approach B, it's easy to know for sure that Approach B is better than Approach A, but it's very difficult to know whether there's an unknown Approach C out there that's better than either, and that makes telling a user that what they've done is good very complicated.


And if you're ever reading one of my answers (or answer sets), you should take omission of commentary on particular aspects of the question-code to mean "I know no better way."

\$\endgroup\$
  • 2
    \$\begingroup\$ you should take omission of commentary on particular aspects of the question-code to mean "I know no better way." - How can we take it as such if you omit it? We might just as well take it as 'I did not bother looking into that part'. \$\endgroup\$ – Simon Forsberg Aug 28 '14 at 11:52
  • \$\begingroup\$ Agreed with @SimonAndréForsberg. An omission is simply an omission and no assumptions should (or can) be made. \$\endgroup\$ – RubberDuck Aug 28 '14 at 15:09
  • \$\begingroup\$ Also, after reading this a second time, I'm not sure it addresses the question. \$\endgroup\$ – RubberDuck Aug 28 '14 at 16:54
  • 3
    \$\begingroup\$ The gist of the question is, should we tell askers that something they've done is good, right? The answer addresses that by warning against doing this because there may be a better way that the answered doesn't know about. \$\endgroup\$ – nhgrif Aug 28 '14 at 17:44
  • 1
    \$\begingroup\$ @nhgrif: I can only second that. Each time I write something like: you have done this well I have the nagging thought who are you to give this "absolute" evaluation? This can be reduced a bit by qualifying it with "to the best of my knowledge" but then the question which becomes even harder (see Dunning-Kruger-Effect). So I fear that on CR the saying becomes "If you don't have any critique, don't say anything at all". \$\endgroup\$ – Nobody Sep 7 '14 at 9:01

You must log in to answer this question.

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