8
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ You've got too many different suggestions going on in this post. If you'd like to push for your suggestions, please post them as seperate questions on meta so they can each be judged on their merit. \$\endgroup\$ – Winston Ewert Sep 7 '13 at 22:26
3
\$\begingroup\$

Codereview requests should specify the context, or at least an intended use-case of the code to be reviewed. If there isn't any (the code is a technical/hypothetical exercise), the question should express what the asker is hoping to get out of it.

I just rephrased a paragraph you wrote. This should be added to What topics can I ask about here?. It would help if people read this before posting code for reviews.

About users being willing to read, this might be a little hard to enforce. A solution might be to review a part of the code and see how the OP replies to that. Placing a sentence in middle/end that there is more if the OP is interested can be helpful to avoid huge reviews if the OP isn't interested to read the whole thing.

But the argument against this can be that even if the OP isn't interested other people may be interested in the reviews. I read C and Python reviews even if it is big. It helps me to learn. I guess the choice would be on the reviewer whether he is interested in writing comprehensive reviews even without knowing if the OP is interested or not.

About the tags, the tag is already present. I edited the tag wiki just now and it should be suitable. I think a medium and expert tag could be added. Here I think making adding one of the 3 tags should be made mandatory. That can be enforced and could be helpful. If you have particular tags in mind then add them to the question details. Others can chip in.

An upvote to the question for good suggestions. These can be helpful.

\$\endgroup\$
  • \$\begingroup\$ "But the argument against this can be that even if the OP isn't interested other people may be interested in the reviews..." Spot-on. This is why I love these sites: no matter the OP's circumstances, someone is likely to benefit from your contribution (as long as it's technically-sound, of course). Had this not been a concern of mine, I would've had fewer answers posted. I am always glad to contribute what I can. \$\endgroup\$ – Jamal Aug 22 '13 at 15:31
  • 2
    \$\begingroup\$ Also, I'm not sure those extra tags are needed. By default, I think most people answer the question with the assumption that the OP has adequate experience in the language. However, I still like the beginner tag because it can alert answerers to "tone-down" the message so that the OP is not too overwhelmed. Eventually, the OP will become more experienced and will no longer need to use that tag. \$\endgroup\$ – Jamal Aug 22 '13 at 15:34
  • \$\begingroup\$ @Jamal The medium and expert tag that I suggested may be redundant but the beginner tag is almost never used. That is the only reason I suggested adding those tags so one of those tags need to be added. Just like on meta sites where there are some tags one of which is necessary. \$\endgroup\$ – Aseem Bansal Aug 22 '13 at 15:39
  • \$\begingroup\$ If you feel that those tags may be beneficial, then give them a try. If you'll go with medium as well, I'd recommend at least naming it "intermediate" or "advanced." \$\endgroup\$ – Jamal Aug 22 '13 at 17:38
  • \$\begingroup\$ @Jamal Only creating those tags wouldn't help. It can be beneficial only if their use is enforced by the website. The main purpose is the use of beginner tag as you correctly said it can alert answerers to "tone-down" the message \$\endgroup\$ – Aseem Bansal Aug 22 '13 at 17:54
  • 1
    \$\begingroup\$ Right. I just wanted to point that out in case you were going to go through with it. Plus, an expert tag would be really unnecessary since they don't even need to ask questions here (but even if they do, it would just be to answer the question purely to contribute information). As for the beginner tag, I'd also be tempted to use it in case the OP puts the "tag" in the title (obvious noise). \$\endgroup\$ – Jamal Aug 22 '13 at 17:59
  • \$\begingroup\$ @Jamal Please do add beginner tag when you see fit. But if the users start adding it themselves then it would be great. \$\endgroup\$ – Aseem Bansal Aug 22 '13 at 18:02
  • \$\begingroup\$ I will. I do recall a few C++ questions from one user with that tag in the title, but I never thought to make that change. I don't think it matters now. \$\endgroup\$ – Jamal Aug 22 '13 at 18:05
  • 3
    \$\begingroup\$ +1, although I'm not all too keen on the expert tag. What makes someone an expert? Who gouverns these tags, what makes a question/post eligible for the expert tag? it's all a bit merky, and might lead to a bunch of self-proclaimed experts, which would render the tag as meaningful as a w3schools certificate \$\endgroup\$ – Elias Van Ootegem Aug 26 '13 at 8:42
  • \$\begingroup\$ I just that for complementing the beginner tag. But if the beginner tag is used without this then there is absolutely no need for this. \$\endgroup\$ – Aseem Bansal Aug 26 '13 at 9:01

You must log in to answer this question.

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