10
\$\begingroup\$

So there has been a lot of discussion about what purpose a code review will have in this site. Whether or not users should be able to bring a "Help me my code is broken" to this site.

So, what does a code review mean to you? what are the pre-conditions for a code review (input) and what kind of outcomes (output) can a user expect?

Add your thoughts below, 1 idea/theme per answer please.

(this should be a community wiki, I know, but no option at the moment)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Fixing broken code is for Stack Overflow; optimizing and offering feedback on working code would be more suited for Code Review. That's how I see it, at least. \$\endgroup\$
    – Andrew
    Commented Jan 20, 2011 at 5:52

3 Answers 3

Reset to default
14
\$\begingroup\$

Not sure these are the way, just a way that looks good right now. So it's a list of desired features.

Input:

  • Programmer believes that high level code/algorithm works correctly.

  • Has spent some time making it easier to read (adding comments is a plus).

  • If in a sane language, has passed done lint/check-list cleanup on code.

  • Should excerpt if and as much as possible, while offering enough code to make it understandable.

  • Is free to contain FIXME, TODO or XXX, as long as OP don't expect peers to fix those bits for them.

  • Context on goals, alternatives, etc. is welcome.

Output:

  • Respects design decisions, may politely suggest/cite alternatives.

  • As pedantic as possible in any dimension the OP asks for it, politely pedantic on others (preferably not on personal style choices).

  • Clear, concise suggestions, optionally backed by extended arguments and examples.

  • General comments about problematic sections or anti-patterns are OK, if constructive and polite.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ +1 for "as long as OP don't expect peers to fix those bits for them." \$\endgroup\$
    – cbrulak
    Commented Jan 20, 2011 at 2:16
4
\$\begingroup\$

What i'd like to see is: "Help me my design is broken"

What i woudn't like to see: "There is a parser error that i can't figure out". I think those question are better suited for SO as they don't really require an in depth code review and most people that ask those question just want their problem fixed and not talk about complexity or readabilty.

I think TryPyPy provided a very solid list (+1)

\$\endgroup\$
1
  • 2
    \$\begingroup\$ This is an acceptable attitude. Compiler errors (or similar) should be resolved before the code gets here, but help on program structure doesn't seem like it should be out of bounds. \$\endgroup\$
    – Inaimathi
    Commented Jan 20, 2011 at 15:03
3
\$\begingroup\$

At my company (where I work, not my own company) we focus code reviews on detecting defects as a number 1 priority (but we also assume code in review will compile and pass tests).

We also ensure that code meets coding guidelines (spaces vs tabs, placement of curly braces,etc).

And, most importantly, that any long term maintenance issues are covered.

\$\endgroup\$

You must log in to answer this question.

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