When asking a question on code-review, the goal is clear: To get a second opinion on your code. By peers, or by people with more experience ("experts", some might say). If you post a ton of code, asking something like "assess my OOP skills" or "Is there a better way to do this", you're essentially asking for refactoring tips, or to be put at ease.
IMO, good Code review is tough love. You can only improve the code by being brutally honest. A proper review of your code often leaves you feeling bad, and that's good. Because your code is making you feel bad, you're less inclined to try and make it work. The delete and backspace keys suddenly don't seem dangerous or scary.
Sadly, sites such as these aren't very welcoming to harsh answers. You're asked to be friendly, and to keep in mind that some users are amateurs/hobbyists. This means there's a trade-off: either we prefer honesty or we are more welcoming to new members.
SE chooses the latter. For this reason I feel forced to write more verbose, overly polite and caring answers, with the occasional applogy and reassurance like "It's not all bad".
Here's an example of an assess-my skill-question. There's nothing wrong with it. If anything, it's the best of the questions I'll link to in this post. The OP kindly provided 3 focus-points. So I dealt with them one by one. I put everything as politely as possible, but I could've linked to the SOLID wiki page, saying "Read it, learn it, and refactor".
Instead, I brought it up several times, because putting it bluntly wasn't going to make the answer very palatable for the OP, or any of the other CR users.
Much like jslint warns you that "Using jslint will hurt your feelings", inform everyone that code-review can be nasty
warn new users that CR has to be tough, to be good.
CR questions should specify the context, or at least an intended use-case. If there isn't any (the code is a technical/hypothetical exercise), the question should express what the OP is hoping to get out of it.
I don't like guessing, and much prefer being complete in my answers. Of course, typing up an answer that would qualify as a mid-sized blog post/article takes time and effort. These long-winded answers, I've noticed, tend not to be read by everybody. They are "put off" by the prospect of reading too much. That's a bit silly, you might as well be put off programming and learning to program all together.
This question, for example, isn't that uncommon here. I can only guess at what the OP is hoping to get from this, so I try to be complete, and touch on everything, but nothing in depth. I briefly discussed the code itself, too. It got upvoted quite a bit, and is the accepted answer: I'm not complaining. However, it's the comments the OP left that really bug me:
Brilliant, just for some background info this was just me messing about with PHP trying to learn OOP, I would never bother reading something as long as your answer normally but from the beginning it was so insightful, I read it all, and thank you for that!
Initially, I was flattered, but come to thing of it: Should I edit, and focus some more on the code? The OP says he's trying to learn OOP, after all. Should I leave it at that, and wait for the next question, and focus on the code more then?
I eventually decided to let it rest, because even if I edited, the OP would probably not read it, and with every paragraph, the answer would become less likely to be read by other users anyway.
People should realize, before posting here, that tearing code down is easy, but explaining why you don't like a piece of code takes time... and words. Words they must be willing to read.
Bottom line: I'm not improving my answer, because that might mean my efforts will go by unnoticed/unrewarded. If I'd have known what the OP was after, my answer wouldn't have taken me that much time and effort to write, and I wouldn't care if somebody didn't read it.
CR questions should specify the context or motivation. users must be willing to read
Another issue with CR I have is: presumption of knowledge. Unlike SO, which is meant to deal with specific problems/situations, you can use the provided snippet to see how the OP attempted to tackle the issue being discussed. Even if it's terrible, it's a starting point.
Here, because the focus is much broader, you can't. Not really.
CR tag to indicate skill level?
This question is a good example: The code didn't make my eyes water, it's not that bad. There's always room for improvement. But what knowledge can you safely assume the OP to have?
I set about explaining what his code does on a low level (simplified of course) with an ASCII diagram.
Then I set about explaining what vars were redundant and why, to go on and simplified the main loop a bit. Once I'd explained that, I suggested using implode
rather than a loop to echo
every element in the array.
Only then, was I satisfied that I had presented the OP with a decent clean version of his code. But sticking to code review, I added some more things the OP ought to know, not in a dry and lecture-y kind of way, but using examples. I tried to illustrate that, in programming TMTOWTDI is king. I also showed that it's all about using the right tools for the job (regex not being the right tool) and that writing clever code can be stupid ('Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.' to quote Brian Kernighan).
Again, this answer was met with gratitude, and was rewarded with up-votes and acceptance, so once again: I've nothing to complain about.