26
\$\begingroup\$

Looking back at some of my last few answers (here and especially here), I started wondering if these answers were good enough.

Allow me to expand: in these answers I went through the code almost line by line, and noted everything that jumped at me; by the time I was done, I already had a quite lengthy answer and decided to stop there.

In both cases, re-reading myself I find my answers look like code bashing sessions, shredding the OP's code to pieces. That's certainly symptomatic of my "read-the-code-and-note-everything-I-don't-like" approach, and could make me sound like a harsh dude that comes along and bitches about everything I see in someone else's code.

In this case, I hinted between the lines that a more OOP approach would be much better, and ended up intentionally leaving out an actual alternative approach, 1 because my answer was getting way too long, and 2 because I figured I'd leave room for other answers. I was thanked, upvoted and my answer was even accepted, and when I told the OP that the acceptance might be premature my answer kept its green tick.

Then another [excellent if not epic] answer came, which basically picked up where I left off and, without the slightest hint at the OP's code, gave the alternative approach I had in mind.

I don't mind that my answer was un-accepted and that the other one got the green tick - it's the OP's call and it's all fair game, and that other answer is really absolutely great.

I made the effort of downloading the OP's code, building it, running it, reviewing it, writing a code review to the best of my abilities, ...and yet the OP deems the other answer as most useful, the one that I could have written by merely reading the OP's post - again, not to take anything away from that great answer (which even quotes mine).

So the question is, exactly what is expected from the "best answer" on this site? An actual code review? Or an alternative approach? The best of both worlds, even if it means an answer so long that only the OP will read entirely (if we're lucky)?

\$\endgroup\$
5
  • 5
    \$\begingroup\$ That anonymous upvote is just agreeing with me being a harsh dude that comes along and bitches about everything I see in someone else's code, isn't it? :) \$\endgroup\$ Oct 26, 2013 at 0:27
  • 2
    \$\begingroup\$ Present. :-) That upvote is mainly for the constructiveness of the question... although I also do this with all of my answers. Anyway, I may post an answer here later. \$\endgroup\$
    – Jamal
    Oct 26, 2013 at 0:30
  • \$\begingroup\$ 1. You don't come off as a jerk in the answers you linked. 2. Hinting at something and stating it clearly are different. Stating it clearly is more valuable than hinting, but hinting isn't worthless. In the case you mention, your hinting at something caused a good answer to be written that you didn't have to write yourself. That isn't a bad thing. 3. Whether you put effort into an answer doesn't matter, what matters is whether it is a good answer. If downloading and running code helps you answer well, by all means do it. \$\endgroup\$ Oct 27, 2013 at 21:14
  • 1
    \$\begingroup\$ 4. It sounds like you find it easier to go through all the details first, and then when you're done you realize that there are high-level problems too. There is nothing stopping you from doing this in that order and then putting your analysis of the high-level problems and overall advice at the top of your answer, as if you had thought of it first. 5. Both details and high-level stuff are important. Whether you look at details first or high-level stuff first is not. \$\endgroup\$ Oct 27, 2013 at 21:31
  • \$\begingroup\$ Not sure that this is what you're asking at all, but I think it's worthwhile to mix a little sugar with the vinegar and tell people what they are doing well also... even if it's just "this seems roughly the right approach" or "nice project" or "this compiles without warnings - good job!". It's easy (and can be helpful) to nit-pick on code style, but if an answer comes across as highly negative or "critical" then the poster might feel downhearted... and that is a worse outcome than settling for less than perfect code. \$\endgroup\$ Feb 9, 2015 at 4:58

4 Answers 4

26
\$\begingroup\$

A good answer is whatever the OP can learn the most from. Depending on the skill level of the OP, either a line-by-line critique or a high-level review could be more appropriate.

Line-by-line critiques

A line-by-line critique is often needed for beginners whose code is too confused or buggy. A pitfall, though, is that often beginners produce code where every line needs work. With a review consisting of a dozen items, you would be lucky if the OP could keep just three points in mind when writing his/her next program.

You do have a bit of a responsibility, as a teacher, to make your answer digestible. The more disastrous the original code is, the more challenging and more important it is to keep the review simple. To that end, you could

  • Prioritize

    With Question 31502, I would consider

    If Expr = FalseIf Not Expr

    and

    i = i + 1i += 1

    to be nitpicks, since their badness only affects the line itself. When you have 11 bullet points, less is more.

    Here's an example of an answer with prioritization. Even though I've raised about a dozen issues, I consider only four to be serious.

  • Group issues into themes, and summarize

    Here's an example of one of my reviews that I think was successful. Even though I found lots of lines of bad code, you can get the gist by reading the headings. You can also skip straight to the revised code to see how it should be done, because reading good code is also an effective way to learn. Finally, the summary gives a root-cause assessment of why the original code was so convoluted to begin with.

    Some common categories might be: bugs, design issues, flow-of-control issues, naming suggestions, and nitpicks.

  • Highlight key words, name your ideas

    If your suggestions have names, they are more likely to be memorable. Soundbites and slogans work!

That said, here's one of my line-by-line reviews that didn't follow those simplification principles. I can't honestly judge my own work — do you think it's effective?

High-level reviews

If the code could benefit from a major overhaul, then a line-by-line critique, no matter how thorough, is just missing the forest for the trees. In fact, it gives the OP a false impression that the code is salvageable. So, with the OOP dice issue, it pays to think before you write, and sometimes you need to discard part of your review once you realize that the code needs a major overhaul.

Summary

Writing clear and concise reviews is as hard as writing good code. There are reviews that get the job done (a "core dump" of your brain), and then there are reviews that express key ideas eloquently. How meta!

\$\endgroup\$
4
  • 3
    \$\begingroup\$ Wow. Just.. Wow. If only I could give you a meta-bounty for that one! [...] then a line-by-line critique, no matter how thorough, is just missing the forest for the trees this is it. \$\endgroup\$ Oct 27, 2013 at 2:42
  • \$\begingroup\$ I gotta give credit for this answer, too. I suppose I should keep answering more questions to really understand this. \$\endgroup\$
    – Jamal
    Oct 27, 2013 at 3:09
  • 1
    \$\begingroup\$ @200_success I've just put your teachings into application, check this out - I actually started going line-by-line, and then scrapped the whole thing! And I don't regret anything! \$\endgroup\$ Oct 30, 2013 at 0:33
  • \$\begingroup\$ I think it's a good idea to do a high-level review in addition to the line-by-line review. Line-by-line does tend to miss the forest. It might be harder to review at a high level, because you have to comprehend the whole program and consider whether there are better ways to do it. \$\endgroup\$ Feb 9, 2015 at 5:05
9
\$\begingroup\$

We need to make sure that we review the code and not the person. Based on experience reviewing code for my co-workers, code review comments tend to work better when written about the code. E.g., the following comments come across--especially in a textual medium--as constructive.

  • This function should be refactored into two smaller functions, each with a single purpose and a descriptive name.
  • We're missing a test case here when i is a multiple of PostsPerPage.
  • Consider renaming this variable to diagLog dl is a bit terse.

Comments like "You forgot the check for nullptr" are often (subconsciously) reinterpreted as "retailcoder is calling me stupid. I'm not stupid. Now I have to argue to prove that I'm not stupid."

Remember, there's a human being on the other end of a code review. This person has placed themselves in an inherently vulnerable position: they've asked for feedback. Whoah. That's a scary place to be in, especially on the Internet.

I haven't reviewed any code here yet. I'm still getting a feel for what the norms are. As I write this, the norms still being developed by the community. :-)

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Welcome aboard! "That's a scary place to be in, especially on the internet" - this would be true on many forums, fortunately CR (SE in general) isn't one. The community is quite active at moderating the site, ...and we're a friendly bunch in general.. when we've had our coffee :) \$\endgroup\$ Nov 15, 2013 at 11:06
  • \$\begingroup\$ @retailcoder: And unlike SO, we don't hate fun (as much). :-) \$\endgroup\$
    – Jamal
    Nov 15, 2013 at 20:31
  • \$\begingroup\$ We're missing… (&we're a friendly bunch…(Mat's Mug)) being co-opted unasked rubs me all the wrong way - my 1st impulse is to ask "Who we?" (I might exempt persons addressing me thus if I see reason to assume "we" spent 99+ hours cooperating - I might give my brother a frown.) \$\endgroup\$
    – greybeard
    Jan 25, 2018 at 9:12
5
\$\begingroup\$

The way I see it, there's no right or wrong answer. An answer is an answer, regardless of how it's approached. Personally, I think having your own "style" helps you personalize your answers a bit, while there's nothing wrong with changing it up for the better.

I'll offer my thoughts on both forms, and you can decide for yourself.

First form:

Depending on how you look at this one, it may look a bit hard to read. I'd say that's because there's no code written for separation, but it doesn't mean you need to force out written code. Overall, this is like, "Here are the main points. Follow (and/or check off each one), and you're good." Although it's not organized (depending on how you've written it), it just gets right to the point. The "harshness" factor shouldn't matter; if it does, then perhaps the OP is not courageous enough to take any needed criticism.

Second form:

This is probably how most code reviews will be approached, at least that's what I've seen. This also looks a bit more "formal" as well. You could also say that it's not "spewing" out the points, as with the first form. This one does take longer to read, but it may very well be easier to read. Despite the essay-like form, it could be a better pleasure to read when one is willing to read through it all.

So, which is better? Well, as I see it, they both have their pros and cons. In my opinion, I think the answerer should first go with what feels more comfortable. If the OP and/or anyone else finds any issues with said format, then it could be changed. The answerer could also adapt to the OP's code depending on the desired review and/or severity of the issues. In other words, not every answer has to follow the same form. If the contents of the answer are spot-on, then chances are that anyone interested will read through the answer regardless of the form.

\$\endgroup\$
2
  • \$\begingroup\$ What about an answer like the object-oriented dice, which doesn't actually review code but rather teaches a much better approach that mootinizes most of the OP's code? \$\endgroup\$ Oct 26, 2013 at 21:01
  • \$\begingroup\$ Then I think such an answer is necessary. If the entire code needs to be redone, then the bullet points won't be sufficient. But they can still be used to emphasize the important points. \$\endgroup\$
    – Jamal
    Oct 26, 2013 at 21:04
3
\$\begingroup\$

As an observation on the topic in question, under the section "Actual Code Review" for the question "Win Forms design pattern", you write

You're using a SplitContainer, and your form is resizable. That's good. Now I realize this isn't WPF, but in terms of layout, I'm sure you could do better

My (humble) pointers are as follows:

1 because my answer was getting way too long

Agreed

 I find my answers look like code bashing sessions, shredding the OP's code to pieces

Not just the code

Stay on topic. e.g.

Is “just reviewing the code” good enough of an answer?

leads you to write:

I was thanked, upvoted and my answer was even accepted, and when I told the OP that the acceptance might be premature my answer kept its green tick.

Stay objective/unattached. SE / SO are not social networking sites.

Many people end up here through Google, and do not have the motivation to read lengthy treatises, when alternatives are available.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your input! Side note, the "premature acceptance" thing was mostly due to reading this meta post. And technically, layout is somewhat part of the code :) \$\endgroup\$ Nov 2, 2013 at 22:19

You must log in to answer this question.

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