10
\$\begingroup\$

Occasionally, when I am writing a review, I'll notice that the OP's code has a small typo in it that renders that part of their code broken.

Should I stop, delete my review, and leave them a comment saying that their code is broken (and possibly a line number and error message, too)? Or, should I continue the review?

I'm not quite sure where I read this, but one time I read on meta that it was okay if a bug is found in your own post, because originally the code was working to the best of your knowledge.

However, I have no way of knowing if the OP is lying just to get a review.

Here is an example where I have seen someone continue their review.

\$\endgroup\$
3
  • \$\begingroup\$ This is a duplicate of something. This has been discussed on meta before (I'm just not sure where right now...) \$\endgroup\$
    – nhgrif
    Commented Aug 3, 2015 at 19:28
  • \$\begingroup\$ @nhgrif I figured that it was a duplicate, too, but I couldn't find it either. I was hoping someone could find it, and maybe point me in the right direction. \$\endgroup\$
    – SirPython
    Commented Aug 3, 2015 at 19:29
  • \$\begingroup\$ It's only partially duplicate. This question specifically asks what should be done while part/most of the review is already written. Should one post it anyway or delete it? \$\endgroup\$
    – Mast Mod
    Commented Aug 3, 2015 at 19:31

1 Answer 1

Reset to default
15
\$\begingroup\$

If you've already written part of the review, then don't delete it. You've worked on it, and your effort should not be wasted.

If you haven't written anything yet, then:

  • If it looks like a simple typo, then:

    • If you're 100% sure of the fix, then just fix it, for everyone's sake
      • I do mean 100%. See the comment by @200_success: exercise extreme caution here
    • If you're not 100% sure of the fix, then leave a comment, and maybe somebody else can fix

    If the code looks legitimate, you can go ahead and review.

  • If the bug is blatantly obvious, then the question doesn't look legitimate, and it's best to vote to close, and probably downvote too

  • As @200_success suggested in his comment: "If the intended code is not 100% obvious, then nobody should edit in the fix; you should write a comment instead."

Keep in mind that having bugs is "perfectly normal" in software construction. And that's one of the big reasons to perform code reviews in software development in real life: several studies show that even single-line changes have a surprisingly high error rate.

Whenever you commit something to version control, you're probably pretty sure the code is bug-free. (Right?) But in reality, it's very often not. It's not surprising that askers miss some bugs too.

The example question you posted had an obvious typo. But legitimate posts with fairly complex hidden bugs are not uncommon. So it comes as no surprise that at the end of last year we even had a Best of Code Review 2014 - Exterminator category:

Answer that points out the most interesting obscure bug in the original code.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I recommend extreme caution when "fixing simple typos", especially ones that would certainly cause a compiler error. They are likely indicators that the code is hypothetical or that the author has not tested the code at all. Also, if the intended code is not 100.0% obvious, then nobody should edit in the fix; you should write a comment instead. \$\endgroup\$
    – 200_success Mod
    Commented Aug 4, 2015 at 7:51
  • \$\begingroup\$ Thanks 200, I incorporated your comment in the answer. Also feel free to further improve. \$\endgroup\$
    – janos
    Commented Aug 4, 2015 at 8:44

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