16
\$\begingroup\$

Re: Connect 4 in C++

Situation: The OP, a beginner posted code with several bugs in it. They said they thought the code worked okay, because they hadn't discovered the bugs during their testing, either because of poor testing and/or a lucky memory arrangement by the compiler.

The first comment mentioned the most egregious bug. My answer also mentioned another off-by-one/out-of-order bug. All of a sudden the question has 5 close votes due to being buggy.

I was under the impression that "closed due to non-working code" is only for cases where the code is horribly broken (e.g. a sort that doesn't actually sort arbitrary lists) or where the OP is asking for help fixing the bugs. In cases where the code has hidden bugs, review is still permitted.

Or am I off-base here?

EDIT: For reference, from the "broken code" link in the "put on hold" message:

If your question contains broken code…

Does your question contain something like:

  • ... runs ...but it gives the wrong answer...
  • ...I tried ... but sometimes...
  • ...works great when I do ..., but when I do ...
  • ...it works fine, but I also need it to do ...

If it does, then your code does not work in the way that you expect, or need.

Broken code is code that:

  • cannot possibly compile even if other dependencies are included
  • fails to run (crashes, throws exceptions, core-dumps, segfaults, etc.)
  • produces results that are obviously incorrect

I think that the question being discussed satisfies precisely NONE of the above (albeit, it may segfault depending on the memory layout, but it obviously didn't for the asker)

\$\endgroup\$
  • 6
    \$\begingroup\$ Can you play a game of connect four with their code? \$\endgroup\$ – Peilonrayz Jan 12 '18 at 17:10
  • 2
    \$\begingroup\$ Related question: How broken does code have to be for a question to be closed? \$\endgroup\$ – 200_success Jan 12 '18 at 17:26
  • 3
    \$\begingroup\$ I plead guilty. I have voted to close because there had been 2 close votes already and because of the comment about index out of bounds. I should refuse to vtc questions using languages I don't know that good. \$\endgroup\$ – Heslacher Jan 12 '18 at 17:29
  • \$\begingroup\$ Note that compiling with clang++ or g++, without any special options, gives three warnings about "control reaches end of non-void function". \$\endgroup\$ – 200_success Jan 12 '18 at 17:34
  • 1
    \$\begingroup\$ @Peilonrayz You can indeed, although if player 2 makes an illegal move the game will hang, and depending on the memory layout chosen by the compiler for the game board, it may award victory to someone who doesn't deserve it. \$\endgroup\$ – Snowbody Jan 12 '18 at 18:23
  • \$\begingroup\$ @Snowbody that scenario has all looks of an unexpected edge case that could easily have been missed by a beginning programmer. \$\endgroup\$ – Mathieu Guindon Jan 12 '18 at 18:39
14
\$\begingroup\$

For this particular case, it's a common beginner error, that wouldn't show up during runtime unless the out-of-bounds reads lead to a win because it happened to look like 4 in a row. I've had plenty of errors like this when I was starting out.

By voting to close questions because of errors like these, we're denying starting programmers the benefit of help from us experienced folk. I for one enjoy helping out young folk setting their first steps.

Questions for help fixing broken code: obviously out of scope.

Questions for comments on code the author thinks is good: definitely in scope.

If you only want to comment on code by professional programmers, feel free to skip this type of question. But I'm glad I could help the kid learn about this error, and a few other things.

\$\endgroup\$
13
\$\begingroup\$

If you ask yourself:

Can you play a game of connect four with their code?

And your answer is:

Yes, except in some special case - which someone may be unlikely to test.

Then no the question shouldn't be closed. Because it's likely the code works as far as the OP knows.

\$\endgroup\$
  • 1
    \$\begingroup\$ I wouldn't call it a "special case", but it is a case that a beginning programmer would be unlikely to run into/think about. \$\endgroup\$ – Snowbody Jan 12 '18 at 18:36
  • 1
    \$\begingroup\$ @Snowbody I don't have good terminology, edge case, special case, edge case, call it what you want. I think it's clear what I mean by it. \$\endgroup\$ – Peilonrayz Jan 12 '18 at 18:42
9
\$\begingroup\$

I just voted to Reopen. My reasoning:

As far as the poster knew at the time of posting, the code worked. The code works for most situations. Two people answered, so the poster can't fix the code anymore.

I would feel differently about this without the two answers. If we can see quickly that the code is broken and express how to see that it's broken, then we can close the question until the code is fixed. That way we're only reviewing working code. But the code in this question can no longer be fixed due to the answer invalidation rule. Therefore the question should be reopened so that people can provide more critiques of the original code.

Remember that the primary reason to put a question on hold is so that it can be improved to meet our standards. However, that's no longer possible. So there's no real point in putting the question on hold anymore. It's not fixable.

\$\endgroup\$
7
\$\begingroup\$

It is unreasonable to insist that code posted for review has no bugs.

It is reasonable to require that code posted for review has no known bugs.

The rule exists so that we can concentrate on improving working code, without being distracted by "please look at my code and fix it" questions that would achieve their purpose better if stripped down to an MCVE and asked on Stack Overflow.

The presence of undiscovered bugs is an opportunity for us reviewers to give some advice on how to improve the test code (many questions are completely lacking test code, suggesting that an introductory lesson on automated testing could really improve the askers' coding careers). That's something I wouldn't want to lose from the Code Review site.

However, if it's clear that the code (as posted) cannot have been tested at all (doesn't compile, produces wrong results using inputs provided in the question), then it's appropriate to close it until it's in a reviewable state.

\$\endgroup\$
4
\$\begingroup\$

In my opinion, it is irrelevant whether the code is bug free or not. I'd say that code will almost always have "yet another bug" even if we feel we've caught all of them.

Otherwise, we should define a formal criteria for keeping the quesion open or closing it. Such as "The OP has to provide unit-/integration-/end-to-end tests. The tests should consistently pass (be "green"). The code coverage should not be less than X%." The point is, even if the questions are posted with the tests, it still does not guarantee the code is bug free.

I think, the questions should only be closed when the OP is aware of the bugs and seeks help with debugging. For example, if the code won't compile at all, it is simply impossible for the OP to not know the code is broken. If the code compiles, we -- the readers on Code Review -- should assume that the OP is unaware of defects, even if that may be something "obvious". Otherwise, we're punishing the beginners for making silly errors; same errors each and every of us has made in their early days in programming. That would not only be unfair, but would affect the community negatively.

\$\endgroup\$

You must log in to answer this question.

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