11
\$\begingroup\$

I've noticed that sometimes I end up reviewing code despite the question's violation of our policy that it must contain working code. I bring this issue up because it is related to Changing wording of the code-to-be-written and working-code off-topic reasons, but I don't want to sidetrack that discussion.

Specific examples of such questions include:

  1. https://codereview.stackexchange.com/q/18663/9357

    The question was about understanding a compiler error. The question was not closed at the time, and in fact attracted several answers before I weighed in with a more thorough code review. (Incidentally, this was the first answer I wrote on this site, a year ago, when I was unfamiliar with the rules.)

  2. https://codereview.stackexchange.com/q/36178/9357

    This question was blatantly asking for code to be written, and in fact ended up attracting downvotes. Nevertheless, there were obvious improvements to be made, so I wrote a partial review that suggested a better data structure and indicated a correct algorithm to use. The answer was accepted.

  3. Applying Dijkastra's algorithm on a graph of five nodes

    The code was acknowledged to be not working. However, the author clearly needed major hints to get close to a solution, which I provided. The author subsequently incorporated those remarks into his code.

In all three examples above, I was able to write a decent review despite the questions' rule violations. The answers were clearly helpful to the authors who posed the question. However, I'd like to ask:

  • Is it harmful to this site to bend the asking-for-code-to-be-written and must-have-working-code rules? It might help the individuals, but allowing the poor examples could damage the quality of the site in the long term.
  • Does code really have to work to be reviewed? Should we recognize sensible exceptions or nuances to the rules, or be more specific about what code deficiencies are tolerable in a question? For example, could reviewers agree to provide guidance rather than a complete solution if the question contains incomplete code?
  • For questions that are marginal (not blatantly asking for strangers to do all the work), what is the appropriate action? Is closure too punitive and unhelpful? Should we just downvote instead? Flag the question for migration? (Would Stack Overflow or Programmers SE necessarily be better, though?)

I'll also point out that "working code" is somewhat subjective. A posted program could produce correct output, but it might be based on such an inefficient algorithm that we would advise discarding all the code — in which case, it doesn't really matter that the original code "worked", except to demonstrate that the poster had put some effort into solving the problem. The author could even suspect that a bug exists. If you wanted to game the system, you could just keep your mouth shut about known bugs when you post the code.

No doubt, the on-topic rules serve a very useful role in maintaining a threshold for the quality of the questions. I'd just like to re-evaluate how strictly they should be applied.

\$\endgroup\$
  • \$\begingroup\$ Should that first question have been closed? \$\endgroup\$ – Jamal Nov 27 '13 at 21:49
  • 1
    \$\begingroup\$ @Jamal The first question should have been closed. (It's not too late!) However, it illustrates that some good could come out of leaving such questions open. (Clear local benefit, but possible global harm?) \$\endgroup\$ – 200_success Nov 27 '13 at 21:53
  • 1
    \$\begingroup\$ I think closing it now is more or less pointless - perhaps a lock (with the kind of message that says the question is not a good fit for this site, blablabla historical reasons)? \$\endgroup\$ – Mathieu Guindon Nov 27 '13 at 21:55
  • 2
    \$\begingroup\$ @retailcoder: I'll study this myself before making that decision. Otherwise, I may just close it. \$\endgroup\$ – Jamal Nov 27 '13 at 21:57
  • 1
    \$\begingroup\$ Due to it's somewhat low number of views at this time, and the fact that it was never debated on its on/off topic nature, nor has it been closed or deleted at any point, a historical lock doesn't seem valid. I'll close it, then. \$\endgroup\$ – Jamal Nov 27 '13 at 22:04
  • 4
    \$\begingroup\$ I read the link you provided, I agree with closing. But this is an excellent discussion that we need to have here. Code that doesn't compile because it's missing a curly brace is still reviewable imho, and keeping it shut about known bugs is certainly happening in more places than we think. (keeping it shut about whether it's your code is also the same kind of issue that should be discussed here as well). \$\endgroup\$ – Mathieu Guindon Nov 27 '13 at 22:09
  • 2
    \$\begingroup\$ @retailcoder: In such cases, it would seem better to edit in that simple fix, then explain it. Sending such a simple problem to SO may be considered sending crap. \$\endgroup\$ – Jamal Nov 27 '13 at 22:14
  • \$\begingroup\$ @Jamal I think edit in that simple fix should only be for users with edit privileges; the consensus is that we don't suggest code edits - right? Hmmm actually I still think the best would be to comment and let the OP fix it themselves. Of course if it's an old question then yeah ok. \$\endgroup\$ – Mathieu Guindon Nov 27 '13 at 22:20
  • 1
    \$\begingroup\$ @retailcoder: I agree with that. Though, I feel uneasy with the OP posting code that hasn't been put through a compiler. \$\endgroup\$ – Jamal Nov 27 '13 at 22:23
  • \$\begingroup\$ Just as a side note: Questions which just happen to be typos are closed on SO (missing curly braces count as such questions). \$\endgroup\$ – ChrisWue Nov 28 '13 at 9:53
  • 1
    \$\begingroup\$ Closely related to Off-topic questions that are still reviewable. This question asks about slightly broken code; the other question asks about working code where the author asks for help with an enhancement. \$\endgroup\$ – 200_success May 9 '14 at 16:33
5
\$\begingroup\$

I think we're over-thinking it.

Questions should include working code.

That's because we don't want CR to overlap with SO. We want to review working code because we're not here to solve OP's problems. Thus, explicitly asking for help fixing an issue is off-topic, and should be redirected to StackOverflow.

I find asking the OP to prove they are submitting working code can hurt us. Of course if the OP's code obviously doesn't do what it's supposed to be doing, it's a low-quality question and IMHO deserves to be stabbed with downvotes, especially if the OP doesn't even explain what the thing is supposed to be doing. Basically these questions hurt our overall content quality, and should be treated as such.

If the rule is stiffer than that, we're inviting gamers. Basically if the OP is asking for their code to be reviewed, it's on-topic. If they're asking us to fix their issues, it's off-topic. All clear? Agreed?

Questions should include your own code.

That's because we don't want to get the OP in trouble (or the author of the code). I think it's useless to try and enforce a stiff rule here, but at the same time we have to have it - I can't help thinking of that employer that submitted an employee's code to get reviewed and decide whether the employee would remain employed based on our reviews... The thing is, we need to trust the OP on that. Besides, is it the OP's code if they posted a question on SO and got an answer and fixed their code with the code that was supplied in the answer they accepted?

Any volunteers for scraping the Internet for identical code that wouldn't be CC-WIKI-compliant?

I think it's ok to close questions that explicitly mention they would like a review of some piece of code they didn't write. If the OP wants to game it, it's their problem.

\$\endgroup\$
  • 2
    \$\begingroup\$ Reviewing doesn't mean solving the OP's problems, though. Many (most?) reviews don't include The Solution. The question in Example 3 may have been about correctness, but the underlying problem was actually style and strategy — things that we care deeply about in Code Review. \$\endgroup\$ – 200_success Nov 28 '13 at 19:59
  • \$\begingroup\$ Would this be the right way of reviewing those? \$\endgroup\$ – Mathieu Guindon Nov 28 '13 at 20:54
  • \$\begingroup\$ Maybe. Your review is similar to what I did with Examples 2 and 3, except that those questions had more salient flaws. \$\endgroup\$ – 200_success Nov 28 '13 at 21:11
5
\$\begingroup\$

"If you wanted to game the system, you could just keep your mouth shut about known bugs when you post the code."

YES, I've seen that many times. I've seen that the code which consists a text at last like "my code works well but it doesn't work for XXX input" becomes victim for most of the times.

Example : Beginner Java Question. Infinite loop? The code was perfectly OK, but OP didn't know how to stop the console to read from standard input. So the question was on-topic since the code was a working code. But as OP mention something like "my code is going to infinite loop..." the question got closed, even though someone pointed out in the comments that the code worked. Eventually, the question was reopened.

But there are also some people who knows the game and intentionally(?) don't provide the test-cases which will break the code. But reviewer points that out in their answer/comment and then it is too late to close the question.

So we can do two things

  1. If it is a small code snippet ask the OP to provide some using/background and some test cases with corner cases also. It would be great if they provide some online testing link like Ideone. Until then don't answer or vote. It may decrease answer ratio of CR but quality >>> quantity.

  2. If someone says "my code doesn't work for XXX but works perfectly else" don't abruptly vote it for closing. See them yourself, it may happen they don't know the technique to handle the special cases. If it is easy to solve give them some hint, not the full answer. And if it was hard to solve then you get a good time solving that... share that with everyone.

At last come on man! there has to be some bugs. No one can write perfect code1, rather it's a good thing that programmer knows the cases for which their code will break and want to solve that.

1 except Jon Skeet

\$\endgroup\$
  • 1
    \$\begingroup\$ I very much agree with the closing statements. I tend to focus on code that has compiler errors, which obviously indicates that it doesn't work. I do think there can be a workaround with these test cases, as long as the OP doesn't outright ask us to look for bugs (that could mean any kind of bug). \$\endgroup\$ – Jamal Nov 27 '13 at 22:26
  • 2
    \$\begingroup\$ @Jamal OP can ask for bug-finding iff he/she came up with some test cases and could not think of any cases which might break the logic. \$\endgroup\$ – Anirban Nag 'tintinmj' Nov 27 '13 at 22:35
  • \$\begingroup\$ Right. I'm referring to requests only about this that only include code. \$\endgroup\$ – Jamal Nov 27 '13 at 22:37
3
\$\begingroup\$

I would not be too dogmatic. If the code posted is interesting enough for someone to post a review then I'd leave it open for other comments, even if the OP (or the reviewer) note that it doesn't 'work'. I would draw the line at the code compiling, preferably cleanly.

On a philosophical note regarding reviews in the wild, I think that not reviewing code until it has been thoroughly tested is leaving things much too late. The cost of rectifying code at that stage is much greater than for errors found when code is reviewed early. Significant issues reported in a late review are likely to be noted but not fixed. Sadly late reviews are the approach taken by companies at which I have worked (those that performed reviews). The review was part of the 'process' and was really just about ticking a box.

\$\endgroup\$

You must log in to answer this question.

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