Is "just reviewing the code" good enough of an answer?

Question

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)?

Answer 1

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 = False → If Not Expr

  and

  i = i + 1 → i += 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!

Answer 2

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. :-)

Answer 3

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.

Answer 4

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.