20
\$\begingroup\$

So, this site is called "Code Review"...

What we do here is... ask for and write Code Reviews, naturally.

However, I feel that there are a whole bunch of different thoughts floating around about what a Code Review really is.

On Code Review, what is a Code Review?

(And what is it not?)

This question is about Code Reviews with respect to this site, Code Review Stack Exchange, the term 'Code Review' may have other meanings at other places or in other situations

\$\endgroup\$
21
\$\begingroup\$

Ideal situation - asker

Code Review is where you go to get feedback on your code that teaches you at least one new way to do things (better).

  • Premise: you bring your code - and you have it in a state where it represents the best you can do.
  • Expectation: someone helps you see things differently, that you would not otherwise have seen.

Ideal situation - answerer

Code Review is where you go to share your knowledge, insights, and experience with people who are trying (and eager) to improve themselves.

  • Premise: you bring your experience - there's always going to be someone who has less experience than you in something.
  • Expectation: you share your ideas in a way that improves the people, not just the code.

Concept

Code review is as much about the person, as it is about the code.

\$\endgroup\$
14
\$\begingroup\$

What is a code review?

Well, let's take a look at that Wikipedia article you linked to.

Code review is systematic examination (often known as peer review) of computer source code. It is intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers' skills. Reviews are done in various forms such as pair programming, informal walkthroughs, and formal inspections.

I think this is a pretty dang good definition myself. The difference between this definition and what a code review is here on CR is the medium in which the review takes place.

Reviews are done in various forms such as pair programming, informal walkthroughs, and formal inspections.

Of these different kinds of review methodologies, CR is kind of a bastardization of an informal walkthrough and a formal inspection. It's like a formal inspection in the sense that the reviewer takes the code, "goes away", and writes up all of their notes about the code. Then, once the reviewer has all of their notes, they take them back to the developer that wrote the code (i.e. posts their answer). It's also like an informal walkthrough, because we're all just doing this in our spare time for imaginary internet points, so reviews are likely to not be very thorough (although many are very thorough, it can't be expected).


So what does that really mean?

It means that a code review here is still a code review. We just use a different methodology to get it done.

So, from an asker's point of view:

  1. My code works as intended.
  2. I'm done with it. It's as good as I know how to make it.

In other words, I'm ready to check my code in.

And from a reviewer's point of view:

  1. Does this code actually do what it is supposed to do under normal conditions?
  2. Are there edge cases where this code will fail?
  3. Will this code be hard to maintain?
  4. Is it flexible?
  5. Does it meet the style standards for the project?

No. 5 is a bit hard for us to do here, but we do tend to make sure the code is consistently styled and often defer to some external "official" style guide.

Note that the definition on wikipedia mentions that the intent of a code review is not just to improve the quality of the code, but also the developer's skills. This is important. Each and every review we give here is a teaching moment. While it may be sufficient to simply point out the flaws in the code, it is not the soul purpose of a review.

What a code review is not.

  • A code review is not an alternative implementation with no explanation as to what is wrong with the dev's code, or why the alternative is better.

    An alternative implementation is okay here, but only if it explains these things.

  • A code review is not a suggestion to throw out an implementation without a good reason for doing so.

    You shouldn't use PHP. C# is wayyyyyy cooler.

    You're going to have to back up that claim. It may be legitimate to suggest a different technology, but you should have clear and objective reasons for doing so.

\$\endgroup\$
  • 3
    \$\begingroup\$ Yeah, but... you really shouldn't use PHP, tho (sorry, sorry, I just couldn't resist) :) \$\endgroup\$ – Flambino Jun 6 '15 at 11:38
  • 1
    \$\begingroup\$ Well, you really shouldn't @Flambino! =;)- \$\endgroup\$ – RubberDuck Jun 6 '15 at 11:39
  • \$\begingroup\$ I know, I was weak and I apologize. A long time ago, I spent all my time with PHP. There's just some lingering, personal dislike there, but it need not affect anyone else. \$\endgroup\$ – Flambino Jun 6 '15 at 11:53
  • 1
    \$\begingroup\$ LOL. Well, for the record, I'm fine with reviews that say "You've got a hammer, you need a screwdriver." so long as they say why the screwdriver is the better tool for the job. \$\endgroup\$ – RubberDuck Jun 6 '15 at 11:54
8
\$\begingroup\$

Basically it's an assessment of code. Good assessments that make statements support their statements with arguments and explanations. Bad assessments just make statements and don't support them in any form.

One might say that their assessment is not a Code Review if it doesn't cover all of the code, and only a small section... or if it only makes a single statement, where more should have been said. But that just makes it a small or targetted, or partial Code Review. It's still a Code Review.

To assess, for me, is to observe and then make statements about. "This code is long", is a valid assessment... but maybe not the best code review. "This code snippet is long for a function, considering the average length of functions in my program is a third of that." is a bit better, but I wouldn't really call it a good code review still.

(If you want to get technical: a statement about code assigns or describes attributes to that code - e.g. that code is long, that code is ugly, that code has bad formatting)

For me, good code reviews provide actionable advice. So not only is it a set of statements, it's also advice for what you should do about those aspects of the code.

A good code review takes (part of) the code, makes 1 or more statements about the code, supports these statements with arguments and explanations, and then provides actionable advice for how to deal with the statement.

If an answer does not make any statements, or these statements are not about the code, then you can't call it a code review. An example of this would be "I rewrote your code for you, *code*." It might help the asker a lot ("wow, you can do that in 3 lines"), but it's merely providing an alternative (if that), rather than reviewing the code.

\$\endgroup\$
  • \$\begingroup\$ Hmm... in this answer you have added the concepts of 'good' Code Reviews and 'bad' Code Reviews. Discussing what is good and what is bad was not my intention of this meta post, there are other meta questions already about what good and bad CR questions and answers are. This question is more about how we use the term 'Code Review' here and what we mean by it. \$\endgroup\$ – Simon Forsberg Jun 1 '15 at 12:36
  • 5
    \$\begingroup\$ @Simon I am trying to cover my ass against statements to the tune of "but surely you can't be suggesting that 'your code sucks' is a valid code review"? Well - it is a code review, just not a very good one. It needs more explanation and arguments as to why/how it sucks and what you could do about it. \$\endgroup\$ – Pimgd Jun 1 '15 at 12:40
4
\$\begingroup\$

How I understand a code review on Code Review

It is a code review, if it

  • shows racing conditions, edge cases and will spot not noticed bugs
  • explains why something should be done in another way
  • explains why something shouldn't be done at all
  • shows logical errors
  • provides an alternative solution with an explanation why the alternative solution is better
  • provides links to relevant websites with a quoted excerpt of the relevant parts
  • has code in it, which should ideally be tested and produce the same results and which improves the former code by making it more readable and maintainable.
  • is simply *"Your code as it stands is interesting, well readable and I wish I would be Bob the maintainer of this code."**

It isn't a code review, if

  • the answer contains only code without explanation
  • the answer is only a link with a comment stating "Check out xxx, it is doing what you want"
  • the answer should be a comment
  • it talks about something totally different
\$\endgroup\$
2
\$\begingroup\$

What a code review is not:

Your code seems fine.

That on itself should be an invalid review. Although there are exceptions (a follow-up on a follow-up will reach near-perfection), on most questions this is an unwanted answer which isn't bringing anything new to the Q&A.

It's fine if it's formulated as this:

Your code seems fine, but you might want to implement X with pattern Y instead of pattern Z to improve readability/maintainability/expandability in case you'll ever do A/B/C with it.

In which case you actually made it a review instead of just saying it's fine.

It would be a valid comment. Many things which are not valid as answers are still valid as comments.

Note there doesn't seem to be agreement about whether this is or is not a valid review.

\$\endgroup\$
  • 1
    \$\begingroup\$ I disagree; and the selected answer for the discussion linked in your answer provides a far more detailed explanation for my disagreement than I can dream of mustering. Essentially what your answer implies is perfect code would never get a single review; as only flaws can be reviewed. I've never seen perfect code; but maybe one day... \$\endgroup\$ – motoku Jun 14 '15 at 18:28

You must log in to answer this question.

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