16
\$\begingroup\$

Based on this question: WPF Calculator Code

The question's code... works, according to the author. However, I quickly spotted multiple bugs. I know that Code Review is not there for fixing bugs. A single, minor bug shouldn't halt review process, however. But what do you do if there's a consistent misuse of a language feature?

In the linked question, the asker consistently resets arrays and strings by nulling them. However, he forgets to reinitialize them. This leads to a crash when he reuses the nulled variable. This leads to roughly 9 bugs (since there are 9 locations where this pattern is evident).

Do you ...

  • Leave a comment stating the bugs and move on? No, says meta. Comments should be disposable, and a comment describing bugs is not disposable.
  • Leave an answer only pointing out the bugs? Maybe, but we're not here to fix bugs, we're here to review code for other aspects.
  • Do both, pointing out the bugs and reviewing the code? Earlier questions from meta seem to indicate that having to spend more time on a question discourages people from answering. So enforcing this means people just let buggy questions alone... except for the people who didn't notice the bug. This effectively means the bug will go unspotted.
  • Vote to close, even although most of the code works, and it's just a minor error causing the bug?

I'm in a bit of a state of conflict regarding what to do. I picked option 3 because I believe helping people is the best course of action... but is that the right option for the site as a whole?

\$\endgroup\$
2

3 Answers 3

Reset to default
17
\$\begingroup\$

Fixing non-obvious bugs is part of code review. What this means is that if the OP ran their code and it seemed to work, then such question is on-topic and shouldn't be closed. If you find a bug in the code, post it in your answer along with other things you want to mention. (But don't forget that you don't have to review everything.)

One problem with this is that you can't read OP's mind to know whether they actually tried running their code. If you think that they couldn't have (e.g. the code won't even compile or it always fails), closing is the right approach.

\$\endgroup\$
3
  • \$\begingroup\$ If their code compiles, but works only once (crashes on second button press, for instance, but works for first), would you consider it as valid? \$\endgroup\$
    – Pimgd
    Commented Jul 23, 2014 at 18:26
  • 2
    \$\begingroup\$ @Pimgd I probably would, but I understand if others disagree and I think I wouldn't vote to reopen such question, if it got closed. \$\endgroup\$
    – svick
    Commented Jul 23, 2014 at 18:32
  • \$\begingroup\$ Code is on-topic if it works to the best knowledge of the asker. If it works only once but crashes the second time a button is pressed, I'd argue that the asker should probably know that... \$\endgroup\$
    – nhgrif
    Commented Jul 28, 2014 at 22:00
6
\$\begingroup\$

This is a subjective call; use your best judgement. But in general:

  1. Make sure the OP didn't already specify that this code is incomplete or anything like that.

  2. Analyze the skill/experience level of the OP and determine whether or not you believe this is a bug that they could miss after a reasonable amount of testing (IE running through the main/correct/expected branches). Keep in mind that perhaps the OP is making assumptions that they should or should not be making.

    After this examination, if you feel the bug is obvious, then vote to close the question. Optionally (recommended) you should also post a comment pointing out the bug.

    Otherwise, include the bug in your answer.

\$\endgroup\$
1
\$\begingroup\$

If you want to play strictly by the rules it would be :

  • VTC the question as question containing broken code
  • Wait for the asker to fix the code
  • Vote to reopen the question when everything is fixed.
  • Review the now working code.

What I would do :

  • VTC as I don't think reviewing broken code is a good thing for the site.
  • Leave a comment about the bugs and probably how it could be fixed.
  • Wait for the code to be fixed in the question, and act accordingly (remove VTC or Vote to reopen)
\$\endgroup\$
4
  • 2
    \$\begingroup\$ The rules say (emphasis mine): “To the best of my knowledge, does the code work?” So, if the OP didn't see the bug, the question shouldn't be closed. \$\endgroup\$
    – svick
    Commented Jul 23, 2014 at 17:25
  • \$\begingroup\$ @svick Well the Op did not test his code properly since the bug is easily identifiable by using the application (using the AC button then doing another action). "To the best of your knowledge" I'm pretty sure he could have find it if he would have tested his code a bit more. I would consider the best of your knowledge to at least try your program as a regular user. \$\endgroup\$
    – Marc-Andre
    Commented Jul 23, 2014 at 17:33
  • 3
    \$\begingroup\$ Try the program as a regular user? Yes. Do everything a regular user could? I think that's asking too much. \$\endgroup\$
    – svick
    Commented Jul 23, 2014 at 17:53
  • \$\begingroup\$ In this specific case doing two actions back to back is not much IMO. I understand your point though, and I would not close a question just because it has a bug like multithreading bugs, race condition and complicated behavior bugs, those are not worth of off-topicness. \$\endgroup\$
    – Marc-Andre
    Commented Jul 23, 2014 at 17:59

You must log in to answer this question.

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