12
\$\begingroup\$

Why is it that Code Review's definition of "working code" is so misaligned with so many people's expectations?

@rolfl

Well, we've had a lot of discussion about broken code on meta, but we don't seem to have a black and white definition of what is "Working Code" is. The help center doesn't define it, but only asks

To the best of my knowledge, does the code work?

I really think we need to define the term "Working Code" and update the help center accordingly. There are some good starting points scattered about. Let's compile them into one answer here.

Relevant Quotes

  1. Are there any doubts expressed in the question itself?

  2. Does it fail to compile?

  3. Does it fail to run, or to produce the expected output?

  4. If it's not obvious from the heuristics above, then the next question I would ask is, would a person having ordinary skill in the art of programming have recognized it as broken code?

@200_success - Can Working code be off topic as Not Working

If it is a matter of a time limit, then the question is valid. This is because we would be reviewing performance, but if the application is giving the wrong results then it is broken code.

@Malachi - OP Says it works, Contest Engine says it doesn't.

Can a bunch of interfaces be 'working code'? Yes. If you have chosen an interface that you feel is right for you, then you can have it reviewed. I do consider an interface itself to be working code, even if it is not technically executable.

Preferably though, you should implement your API before asking here and get the whole thing reviewed together.

...

What is 'working code'?

Code that works the way you want it to. Code that produces the results you want.

@Simon André Forsberg - Code Review vs. Help me solve my problem

Other Relevant Discussions

\$\endgroup\$
6
\$\begingroup\$

Welcome to Code Review! I'm afraid this question does not match what this site is about. Code Review is about improving the cleanliness of existing, working code. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way!

This is what I usually comment on questions that I vote to close with the reason "Questions containing broken code or asking for advice about code not yet written are off-topic (...)"

To me the question about "Is this working code?" all boils down to:

Does the code do what you want it to do?

This can be separated into the following parts: (perhaps there are even more parts)

  • Does the program return the expected results?
  • Does the program have the features you want it to have? (Do not ask "How do I implement feature X?")
  • Does the program do it's job without throwing unexpected exceptions, causing segmentation faults, etc? (Questions about avoiding exceptions and segmentation faults is Stack Overflow territory)

The answers to the above questions has to be Yes for the code to be considered 'working'.

\$\endgroup\$
  • \$\begingroup\$ Pretty good definition. \$\endgroup\$ – RubberDuck Oct 1 '14 at 9:30
  • \$\begingroup\$ But if you pass in empty list and the code breaks, is it offtopic? \$\endgroup\$ – Pimgd Oct 1 '14 at 14:22
  • \$\begingroup\$ @Pimgd If the OP knows that it does not work for an empty list but works for other lists, and the OP wants it to work for an empty list, then it is not working code. \$\endgroup\$ – Simon Forsberg Oct 1 '14 at 14:35
  • 2
    \$\begingroup\$ If OP wants it to work for an empty list. It's perfectly acceptable to call out, "Hey, I know this doesn't work for situation X, but that's okay because Y." \$\endgroup\$ – RubberDuck Oct 1 '14 at 14:47
5
\$\begingroup\$

Code works when it does that which was intended in full and nothing more, not containing errors for the original expected input domain, with the exclusion of issues caused by external factors such as memory limitations.


The explanation:

That which was intended in full and nothing more

Code does X. Code does not do Y. It's not required to validate this, but when an asker states that "code does both X and Y and I wish it only did X instead" then we have broken code. Code doesn't not need to be executable for reviewers; it does need to be executable for the asker, though.

not containing errors for the expected input domain

I put "normal" strings in here and then the code reverses the string.
I put a filled list in here and then the code sorts the list.

If you then find that the code doesn't support unicode, that's a nice point to state in an answer. But the asker's original input domain didn't contain unicode. Just "normal" strings. If the asker had mentioned they did support unicode and it was obvious they didn't then it would be viable for closure (but more likely; we'd edit it so it's no longer making the claim, then review it anyway).

with the exclusion of issues caused by external factors

The code works... but I run out of memory on my machine for any list larger than X.

Here the cause is important.

You run out of memory because the code goes into an infinite loop: Broken code.
You run out of memory because you have a heapsize of 128 MB and you're trying to store a 3D array of 1000x1000x1000 4-byte pointers: Not broken code. You just forgot to handle OutOfMemoryException for large allocations.

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

The premise of this question is wrong. This is a misleading question.

To put it bluntly, code is not "on" or "off" topic based on whether the code works or not. Not-working code can be on-topic. The critical aspect is actually clear in the help-center:

- To the best of my knowledge, does the code work?

This is not about working code, it is about what the asker of the question knows (or reasonably should know).

So, how do I explain this... If I have the following (Java) code:

public static void main(String[] args) {
    System.out.println(args[1]);
}

Is that working code?

Well, that code will work if you supply two (or more) arguments to the program. It will throw an IndexOutOfBoundsException if you have one, or no arguments.

So, that is code that may work. Now, why is this important? It's important because the significant feature of the question, when it comes to being on topic, is what the asker knows. Let's run through some scenarios for a question containing this code:

On Topic

I have this code that prints "World", and this is how I call it. Is it a good program?:

java MyClass Hello World

This is on topic because, despite being horrible code, the asker possibly never noticed that there is a bug.... and, since the code works for the intended use case, it is, "to the best of their knowlege", working.

Off Topic - known broken

I have this code that sometimes throws an IllegalArgumentException, and sometimes does not. This for example, when I run:

java MyClass Hello World

it works, and prints "World". How can I improve it?

This is off topic, because the asker knows there is a problem, and not only that, it is the top priority to get it fixed. That reason would actually make the question On-Topic on Stack Overflow, and Off-Topic here.

Off Topic - should have known

I have this code which prints "World" when I call

java MyClass World

Code that is presented for review that is obviously broken, and would not ever work, compile, or has some other fatal flaw, even in the intended use case, is code that the asker should have known was broken, even if there is no claim that it is broken, or even a 'lie' saying it works.

Off Topic - missing feature

I have this code that prints "World" when I run it like:

java MyClass Hello World of Code Review

but I want to extend it to print "World of Code Review".

This is code that needs to be modified to accomplish a task that is not there yet. The code currently runs, without error, but does not produce the desired result.

Summary

Code Review is one of the last steps in a process for creating quality code. It is expected that the code does its intended task before the code is ready to present for review.

If the code is not working as intended, and you know it is not working as intended, then the code is not ready for a review. If it fails, or occasionally fails, and you need help getting the code in to a reviewable state, there is Stack Overflow that can help you. If it works in a limited way, but does not accomplish the full desired feature set, then it is also not ready for review, and, depending on the problem, Stack Overflow, or perhaps Programmers may be the right place to go to for help.

If you think your code is ready for review, then you must also think that the code does what it needs to do, and it does it satisfactorily. Even if your thinking is wrong, the code, in your mind, was ready for review, and thus is on topic.

Bringing code to a review which is not ready for a review, and you know it is not ready for a review, is a horrible, mean thing to do, and it wastes time, and there is a special close and downvote button for those folk.

Bringing code to a review which you believe (in good faith) is ready for review, even if you are wrong, is forgiveable... with maybe a downvote, and a polite answer that points out why, would be nice.

Lying about your broken code, and claiming it works, when it does not work, is more than just rude .... hmmm. Not sure what the right punishment is for that.

\$\endgroup\$
2
\$\begingroup\$

One problem with a "working code" is that often we don't know that the code is broken unless the OP says so. As @Pimgd noted,

Code doesn't not need to be executable for reviewers

Compare Delimiter matching using stack vs Array list implementation. Both posts contain the broken code. Both posts have enough meat to be reviewed. One admits that there is a problem, and gets punished. Another doesn't and gets awarded. I don't think it is fair.

In my opinion, if the code contains a material for a review it is not off-topic.

\$\endgroup\$
  • \$\begingroup\$ While I find this to be an interesting point of view (and one I agree with to, at least, some extent) it doesn't answer the question. \$\endgroup\$ – RubberDuck Oct 3 '14 at 2:52
  • \$\begingroup\$ Correct. I feel my post is "Rock is the answer. What was the question?" kind of response. I'll try to expand. \$\endgroup\$ – vnp Oct 3 '14 at 3:44

You must log in to answer this question.

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