0
\$\begingroup\$

Before asking my last question, I've double checked what counts as off-topic and on-topic in this SE. According to my findings:

We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review.

I find this poorly phrased compared to what is actually happening. From Gareth Rees comment I take it that any question that is bug related is immediately off-topic.

This is not what is reflected by this phrase. In fact this meta post seems to suggest:

[Purpose of a code review:] Are there edge cases that may cause the code to fail

I would assume this to be on-topic on a code review platform? I am confused because this doesn't seem to be the case.

As such I want to suggest to phrase the off-topic reason more explicitly to reflect current policies. My ideas are:

We require that the code be working correctly before proceeding with a review

or

We require that the code be working correctly in all cases and to the best of the author's knowledge before proceeding with a review

But there are probably better ways. My goal is to make it more clear that anything that is about unexpected behavior is off-topic; including corner cases.

\$\endgroup\$
8
\$\begingroup\$

Unfortunately I agree with the close reasons. To the best of your knowledge, your code fails the google test cases, therefore it does not work. Just because you don't know what the problem is, does not mean you don't know there is a problem. Further, just because you don't know what the failing test case is it does not mean that the code works.

On code review we have a specific exception for failing test cases in "competitive" challenges for - questions that get the right result, but take too long - performance problems. You can't even tell us if that's the failing test case.

In other words, you've brought us a question with code that works in your test cases, but your test cases are incomplete, and there are other test cases but you can't tell us what they are, which ones are failing, and what the failing inputs are. You are specifically asking us to determine why those tests (that you can't provide) fail.

Now, if you could give us the failing tests cases, then the question would still be off-topic here, because you should be taking it to StackOverflow instead - with a simplified minimal code example illustrating the problem.

Code Review is for reviewing and improving working code. Sometimes that means discovering/mentioning issues where edge-cases may cause problems (for example, unexpected input may cause SQLInjection problems, or integer overflows, etc.). Those are problems the question asker was unaware of when they asked the question.

Put differently, Code Review is for code that in the real world is "ready for release in to production" - code that you are confident will work, but just want an objective opinion on any improvements that can be made.

Edit: the close-reason contains a link to the "Broken code" answer on the question: My question was closed as being off-topic. What are my options?. I have edited that answer here to include failing test cases as a close reason. This does not help with confusion before asking a question, but it may help with confusion after a question is closed.

\$\endgroup\$
  • \$\begingroup\$ Sure, I can agree with that. Please don't view this as yet another "I'm unhappy about my question being closed" post. I am unhappy, but not because it was closed; rather because I put in effort to double-check if it is okay to post, found information that suggested it was okay and then still got flagged. THAT is what actually makes me unhappy and what I would like to see clarified :) \$\endgroup\$ – FirefoxMetzger Apr 8 '18 at 13:50
  • \$\begingroup\$ Oh, and I think the "ignorance about failing cases makes it okay to post" is a very odd policy. I could rephrase my own question as a "This code works, but will not scale to large lists (time-limit-exceeded)" which makes it okay to post and then wait for someone to point out the missing "#" \$\endgroup\$ – FirefoxMetzger Apr 8 '18 at 13:54
7
\$\begingroup\$

The phrasing in the close reason is a long-fought-over compromise.

The Scylla and Charybdis that we are trying to avoid are:

  1. We don't want to turn into a debugging service where people post broken code and ask us to fix it. That's a thankless job for Stack Overflow.

  2. But we also don't want to have to reject essentially working programs that fail in some corner case.

The close reason is phrased so as to tersely steer a course between these rocks. If you have a suggestion for better phrasing, go for it.

In the particular case in hand, we know that debugging code for programming challenges can be frustrating because the challenge site does not tell you which test case failed, or why. But if you can't figure it out for yourself, then you need Stack Overflow, not Code Review.

\$\endgroup\$
  • \$\begingroup\$ That makes it sound like even more of a double standard. Don't we put questions on-hold to give people the chance to rephrase them and make them a better fit? Why should we make any difference between the severity of a bug; compile error or a minor corner case? Once that minor bug is fixed, we can reopen good questions again. Also if we'd rather keep the current phrasing, perhaps there is a clear metric to guide people (like me)? \$\endgroup\$ – FirefoxMetzger Apr 8 '18 at 14:16
  • \$\begingroup\$ I would be in favour of putting questions on hold until corner cases are fixed, but that's not the majority opinion here. As I said, the current position is a compromise. \$\endgroup\$ – Gareth Rees Apr 8 '18 at 14:20
  • 2
    \$\begingroup\$ @FirefoxMetzger Why should we make any difference between the severity of a bug; compile error or a minor corner case? - because if you have a compile error, you know it's broken and you're clearly asking on the wrong site. OTOH if you're happy with your code and it's as good as you're going to make it and you're ready to have it peer reviewed and a reviewer comes up and finds a bug, suddenly your question becomes off-topic? Answers/reviews don't make a question off-topic, what the OP is asking for does. \$\endgroup\$ – Mathieu Guindon Apr 8 '18 at 17:00
4
\$\begingroup\$

Phrasing things is difficult. I want to make it clearer what is currently on-topic and off-topic.

  • I have a bug in my code, please help me fix it. (Off-topic)
  • Is there any bug in my code? (On-topic)
  • Is there any edge-case that I haven't thought about? (On-topic)
  • My tests work fine, but the tests that the online judge has fail because of wrong result (Off-topic)

In your question you wrote:

Since my local tests all pass, the remote tests fail and Google doesn't seem to reveal them, can somebody please look at my code and tell me where I am wrong?

Because of Google being the online judge and "the remote tests fail" then this means that the code is not working correctly to the best of your knowledge. You know that your code is not producing the correct results, you just don't know why.

\$\endgroup\$
  • \$\begingroup\$ With this being said, it's possible that the current close reason phrasing can be improved - I just don't know how. \$\endgroup\$ – Simon Forsberg Apr 8 '18 at 13:02
  • \$\begingroup\$ What bothers me about this policy is that it makes things be on-topic if you are just ignorant enough about it. I can say "with the naive O(n^2) implementation it doesn't scale well to large lists" and ignore my other problem. Suddenly its on-topic and I can get my actual problem solved. I think this is a loophole; not sure if I managed to convey it. \$\endgroup\$ – FirefoxMetzger Apr 8 '18 at 13:58
  • \$\begingroup\$ Yes, it is a loophole. We can amend the guidelines in a way that addresses it and/or rely on participants (in all roles) to understand and uphold the intent. Lacking a concrete suggestion on how to change the wording in a way that better clarifies the intent, it seems to me that we must rely on the latter. \$\endgroup\$ – Edward Apr 10 '18 at 16:54
-3
\$\begingroup\$

I want to present an entirely different solution, which just occurred to me, and let the community vote on it.

The idea is that we require questions to supply working code anyway. We could get a lot more value out of this by taking a test driven approach to this and slightly extend requirements to include a "working/broken" "test case" that fits the aim of this SE:

Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability.

If I have an issue with performance or scalability, I can probably create an example to demonstrate the issue. I did have a performance example of this recently: here I ended up providing such a test case to create a framework for comparability. There was a bug in my initial code; however, it was unrelated to the test case so I guess that's what makes it okay?

I never used this SE for security questions, but I can imagine that something like "is it possible to exploit X to achieve Y" can be turned into a test bed that can be provided by an OP and would make it easier to answer concisely.

Maintainability is a rather subjective goal imho and (for me) it is always challenging to figure out if it is opinion based or not. I generally refrain from asking these questions, maybe someone else can help here.

An advantage of such an approach would be that it would clearly say that questions like this are okay, but this isn't (in lack of being able to provide a test of what is broken).

Further, we can clearly say things like: "reviewing to pass this kind of test is not the aim of this SE; the question doesn't test X on performance but on a random corner case. Please post it on stackoverflow"

Another advantage is that the answers can focus on improving said performance metric / test and, even if a small corner case bug exists, that can be ignored without wronging the question or answer. (Providing invariance to some smaller or bigger bugs which seems to be wanted here)


I have edited that answer here to include failing test cases as a close reason.

As per @rolfl 's edit: This is kind of contradictory to my idea :D Maybe it can still start a discussion and result in something useful.

\$\endgroup\$
  • 3
    \$\begingroup\$ I am under the impression that you're understanding the help center the other way around. Ideally you shouldn't come at code review with a problem with your code (scalability, security, etc.) and ask us to fix it; rather you should present your code aaaaand that's it. Reviewers will pick any subject (maintainability, style, best practices...) that they see fit to answer. You can always indicate some issues such as scalability or performances, but it's in no way a guarantee that you'll get answers addressing it. That's what is meant by "any and all aspect of the code is fair game for review". \$\endgroup\$ – 409_Conflict Apr 9 '18 at 8:49
  • \$\begingroup\$ @MathiasEttinger Then why can you "accept" an answer on this site? Why do you give a title and have to make sure your question is not a duplicate? Your view on this opens up so many new questions to what strikes me as inconsistent that I almost don't dare to think this way :D \$\endgroup\$ – FirefoxMetzger Apr 9 '18 at 12:09
  • 2
    \$\begingroup\$ As stated in How to Ask, we expect titles to state the task accomplished by the code, rather than the author's concern about the code. (Yes, we get tons of "My linked list" questions with similar titles; that's just how it is.) Accepting an answer is optional, and acts as a way to indicate that one answer was particularly helpful and deserves another +15 points. \$\endgroup\$ – 200_success Apr 9 '18 at 18:47

You must log in to answer this question.

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