9
\$\begingroup\$

Obviously related to this, but I want to know the opposite. How short is too short?

For example, this doesn't seem long enough to be appropriate for a code review:

Highlight input if empty

The entire code sample:

/* highlight input, if it is empty */
$(function() {
    var highlight_if_no_val=function () {
        $(this).toggleClass('highlight-input', !$(this).val());
    }
    $('#id_index-EditForm-gpnr').keyup(highlight_if_no_val).each(highlight_if_no_val);
}
);

Clearly this user has a different understanding of what a code review is than I do. This doesn't seem like enough code or nearly enough of a complete picture to warrant a review. Should questions like this be closed as off-topic?


Should testability be mandatory?

The length of the code sample is an oversimplification of only part of the problem. A bigger problem is the lack of context. We don't know what the markup being "queried" looks like, we don't know what else is going on in the script, etc.

You can't easily test this code by passing it some values. That means you can't refactor it and be sure that it still works as before given some test cases.

Without testability, we have to guess at stuff and say things like "do something like this instead" and leave FIXME comments here and there, etc.

Making at least some level of testability mandatory could really improve the overall quality of the code review requests listed here.

What's the solution?

Reviewing (answering) these requests seems like the wrong move to me. It encourages quantity over quality.

Closing the questions as off-topic might be easiest. Another approach could be moving to Stack Overflow. The justification for that could be that the code sample is short enough and not enough context is given that only one or two things can really be said about it, making it a question suitable for SO.

In the example above, the question is introduced like this:

This snippets works, but I think it can be optimized:

[code]

Is there a simpler solution?

In fact this already sounds like an SO question, and probably has a duplicate on SO, so it can just be moved there, marked as a duplicate, and closed (which sort of seems like a lot of trouble). This user doesn't really know what question he's asking, but that's OK, others do. When he clicks through to the duplicate he'll understand what his question was, and see the answers.

Where should the mark be set?

In contrast to the previous example, it's like this user read my mind. Sure, it's a TGP browser. Sure, he's using Comic Sans MS. But I can drop his code in jsfiddle, test it, edit it, and give him a good code review. He tells us what the program does and gives the script, markup, and CSS all in one code block.

There's plenty of stuff to review, and plenty of code, enough that you don't feel like you have to refactor the whole thing (and at the same time nothing too irrelevant).

If the mark for valid questions that involve multiple technologies (here, as above, JS, CSS, and HTML) is set closer to this example than the example above, I think a lot of noise can be avoided.

\$\endgroup\$
  • 2
    \$\begingroup\$ I would say this perspective is falling into the LOC trap of evaluating complexity. At the risk of sounding like a hand-wavy guru, "It is as long as it needs to be." Of course, if the ground's been covered already and frequently I would just direct those users that way. \$\endgroup\$ – Incognito Feb 10 '12 at 20:10
  • \$\begingroup\$ I think this relates to my question here: meta.codereview.stackexchange.com/questions/383/… \$\endgroup\$ – Winston Ewert Feb 10 '12 at 20:15
  • 1
    \$\begingroup\$ @Incognito it's more the fact that it's incomplete and untestable than the length that bothers me. WinstonEwert: Most of the things you linked seem like SO questions for sure, not code reviews requests. To me, the thing I linked seems more like a request for a code review than an SO question, it just seems like clutter not worthy of a code review. \$\endgroup\$ – Dagg Feb 10 '12 at 22:37
  • \$\begingroup\$ @WinstonEwert I changed my mind, this one sounds like an SO question too. \$\endgroup\$ – Dagg Feb 10 '12 at 23:14
  • 1
    \$\begingroup\$ I see what you're saying, it's not valid code. But that's more about feasibility of doing a review than the length per se. \$\endgroup\$ – Incognito Feb 10 '12 at 23:15
14
\$\begingroup\$

Should questions like this be closed as off-topic?

No, questions like that are reviewable...

I find that some of the shortest code samples contain large amounts of wrong. For this particular example I found that:

  • There is no point in creating an anonymous function here, do function highlight_if_no_val()
  • In fact, use lowerCamelCase; highlight_if_no_val -> highlightIfNoVal
  • In fact, would highlightEmptyValues not be a better name ? Your comment says: /* highlight input, if it is empty */
  • Consider also renaming your CSS class to match your intent
  • Cache ($this), especially if you are going to run this potentially on a number of elements
  • It is custom to not put a newline between } and );

Thus there is enough to review, and the question is not off-topic.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for meta-review! \$\endgroup\$ – Mathieu Guindon Apr 2 '14 at 16:03
7
\$\begingroup\$

The length of the code is actually not important.

A short question can contain reviewable code.

int main()
{
for(int i=0; i<=100000000000; i++); // <----- note that the loop doesn't do anything 
return 0;
}

We know what the author is trying to accomplish with this code. It accomplishes the intended task poorly, and we can say exactly why. That constitutes a good question.

\$\endgroup\$
  • \$\begingroup\$ Well, technically the example would be closed for broken code because the int value overflows. :p \$\endgroup\$ – IEatBagels Jul 20 '15 at 18:21
  • \$\begingroup\$ @TopinFrassi It's only closable for being broken code if it's obviously broken. Latent bugs should be pointed out in answers. \$\endgroup\$ – 200_success Jul 20 '15 at 19:36
-7
\$\begingroup\$

There is a necessary minimum, but it is not strictly related to length.

Rather, the minimum related to whether there is enough context to know whether or not the code is good. For a longer code sample, the related bits give enough context for each other.

As a rough rule of thumb, code should have at least three function calls or control flow statements. A single call is almost never sufficient to know what it's for, and I can't think of any two-call cases either.

The code in this question satisfies that minimum.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why three? What's so magical about three? This answer falls woefully short of complete. Why do short code snippets seeking review require context code but long code snippets seeking review don't? \$\endgroup\$ – nhgrif Jul 16 '15 at 1:38
  • \$\begingroup\$ Three is an approximation based on what I can imagine. A long code snippet contains enough context to review its contents, a single function call does not (and two is unlikely to). \$\endgroup\$ – o11c Jul 16 '15 at 1:41
  • \$\begingroup\$ You don't need context to review contents. The only thing you need context for is if it's unclear how the code to be reviewed might be used (you don't understand what it's for). Context can be provided by way of context code (example usage) or by a plain-English description of how it's used. A question that lacks context should probably be closed as unclear what you're asking. \$\endgroup\$ – nhgrif Jul 16 '15 at 1:44
  • \$\begingroup\$ @nhgrif It's entirely possible to lack context without lacking understanding of the code - this is what happened in the single memmove call that prompted this discussion. The answer is completely different based on whether it is called once per input or multiple times with different positions. \$\endgroup\$ – o11c Jul 16 '15 at 1:52
  • \$\begingroup\$ It's not though. The fact of the matter is, no matter how we intend code to we write to be used, it's going to be used in that way and a million ways it wasn't intended to be used. Write your review to account for that. Why are you trying to review context? \$\endgroup\$ – nhgrif Jul 16 '15 at 3:27

You must log in to answer this question.

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