8
\$\begingroup\$

Yesterday, it became apparent that there is a significant difference of opinion in the CR community about what constitutes "Too Broad" for the purposes of closing a question.

In particular, there is disagreement over whether questions containing separate pieces of code (E.G. The first N Project Euler problems, a collection of functions, perhaps 2 different parts of the same overall system) should be closed as "Too broad".

To be clear, this is not a debate about the best way to ask questions, but about when a question is so broad that it should be closed. It should also be noted that "Too Broad" is a close reason entirely separate from "Off-Topic".


What is the purpose of Code Review?

Is it to "Review a piece of code, and advise how it could have achieved its objective better".

Is it to "Review how the OP currently writes code in general, and advise how they could become better at writing good code".

If it is both, does a question have to be constrained by both requirements?

If an OP wants a general review of how they write code in [language], must it always be in the context of a single program?

Does it make a difference if a collection of code is small?

How about if a single program is extraordinarily large?

What is the purpose of "Too Broad"? What kinds of questions do we want to discourage and why? Where do we draw that line?

\$\endgroup\$
12
\$\begingroup\$

I've said this before and I'll say it again:

Code Review is not about what you need. It's about what you get.

Even better, it's not about what you think you need. If there's a major design flaw in one's program let's hope somebody points it out. If the code is riddled with bad practices, point it out.

It's not up to the one asking the question what the answer should focus on. Sure, if there's a known performance problem OP should feel free to point it out and if he gets lucky somebody will focus on that. But it's not a requirement.

Reviewing code is hard. It's hard on the reviewer and hard on the one getting the review. Both parties want the best possible code and get the technical debt as low as possible. However, there can be disagreement about what is 'best'.

The appropriate way to use Code Review is, in my opinion:

  • Write code.
  • Improve the written code.
  • Post it for review to see whether the rest of the community agrees with your code being well written.
  • Get answers.
  • Use the information received from the answers to write better code.
  • Repeat the above, assuming the next post will not be within hours of the first.

What happened here was 5 unrelated pieces of code were thrown into one question. Yes, it's likely the same mistake was made twice. So what could've happened if the above scheme was used?

  • Write 5 pieces of code.
  • Improve the written code.
  • Post the first piece for review to see whether the rest of the community agrees with your code being well written.
  • Get answers.
  • Use the information received from the answers to improve the remaining 4 pieces of code.
  • Repeat for the next 4 pieces.

Don't post your code the moment it's done! Take a look at it first, see if there are obvious improvements you can make yourself, especially if you were new to the language when you started and got expert advice in the meantime.

\$\endgroup\$
  • 6
    \$\begingroup\$ +1 for "Don't post your code the moment it's done!" \$\endgroup\$ – nhgrif Dec 8 '15 at 13:31
  • 1
    \$\begingroup\$ I think there should be a difference here between recommended way and enforced way. I also recommend posting one piece of code, then wait for review, then post another piece, etc. but in this case, only one question was created. And you're saying here that "What happened [in EBrown's question about Project Euler 1-5] was 5 unrelated pieces of code were thrown into one question." without arguing at all about why there were unrelated. All five pieces of code are called from the same "main" part, doesn't that make them related? \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 16:11
  • \$\begingroup\$ I feel like you're more arguing about "Don't post 5 questions directly after each other" than "This question is too broad". \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 16:11
  • \$\begingroup\$ @SimonForsberg It's trying to explain why it's too broad. Not clear enough I guess? \$\endgroup\$ – Mast Dec 8 '15 at 16:36
  • \$\begingroup\$ Nope, not clear enough for me at least. The only explanation of too broad that I can find in your post is "5 unrelated pieces of code were thrown into one question", and that is debatable whether or not they are unrelated. \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 18:00
  • \$\begingroup\$ @SimonForsberg Being in the same main doesn't make them related. All can be executed regardless of the other pieces in this example. It isn't like the first function is the initialisation of the second. I'm not level headed enough at the moment to clear that up without making it worse. \$\endgroup\$ – Mast Dec 8 '15 at 22:13
  • \$\begingroup\$ @Mast Well, apparently the OP thought they were related enough to put them there in the first place. Again, I think it should be an answer in that case, not a close vote. \$\endgroup\$ – Simon Forsberg Dec 9 '15 at 10:48
11
\$\begingroup\$

First of all, let's do an extreme example:

Imagine that I post a question about a PHP Code Review "shield" and a C# Sudoku solver. Not as two separate questions, but as a single question. Such a question would be on-topic, it's working code, it is real code, and so on. But what does a PHP Code Review shield have to do with a C# Sudoku solver!? Absolutely nothing! This would be too broad. Different languages, completely different purposes - clearly too broad.

Okay, so let's see what we think about posting a question in the same language. Let's take a Poker Hand Evaluation and put that into the same question as a Tool for posting Code Review questions, both questions written in Java. There is still the problem about these two being completely separate things, they have nothing in common besides being written in the same language. They should still be separate questions.

Single Responsibility Principle

Now, let's take a question about Finding the sub-array with the maximum sum and a question about Minimum triangle path. Still, those questions have nothing in common. It's not one program for both, it's two programs. Could I write a program that contains both? Sure, I could. And if I would, it would be an acceptable question as then the programs would have something in common: They would both be accessible from the same class that I would have written.

As I said in chat, it's about whether or not you can say "You are violating Single Responsibility Principle" or not. If I would write a single program for both finding a sub-array with the maximum sum, and a minimum triangle path, then you could tell me that this program is doing too much, and I should put these things in separate programs and not try to tie them together.

Describing your code

It is also about being able to describe what purpose your code has. When writing a Code Review question, it is your responsibility to describe what purpose the code has, if you cannot make a reasonable single description for your code, then it is too broad.

So what do I think about some specific questions?

EBrown's question about project euler 1-5 - includes a wrapper to access all parts of the code, which means that the code has a common thread (all being accessible from one place), and "Project Euler problems #1-5" is a reasonable description. In my opinion, this is okay and not too broad. Saying "You should split these up into five totally different programs instead of trying to tie them together with some small piece of code" is a valid review.

Zak's question about 19 utility methods in VBA - No code provided to tie it all together. No reasonable description provided, but instead 19 different descriptions, each providing information about what one method does. Note that some methods are about arrays and others are about open and closed workbooks. In my opinion this is definitely too broad.


Note: Sorry for mainly using my own questions as examples. Those were the first one I could think of and the ones most easily accessible to me. This is not meant to be "pimping", only as being useful examples.

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't understand your distinction between the two specific questions. If Zak's question instead were "Project Euler 1-19 in VBA", would that then not be too broad? If it still would be, is it just adding the extra wrapper method calling the previous 19 that would make it not too broad? It seems like your answer is building up to solving one problem, about a "reasonable single description" - but then the F# question led with five problem descriptions. \$\endgroup\$ – Barry Dec 8 '15 at 16:18
  • 1
    \$\begingroup\$ @Barry I consider "This is a program for solving project euler 1-19" as a clear description for what the code does, and if there would be code to call these 19 different questions, then no it would not be too broad in my opinion. However, if someone would post such a question, I would likely down-vote it, comment on it, maybe even answer and say something like "Next time, consider doing a program that does less", but I would not close it for being "too broad". Remember that there are more options available, not only "let it be" and "close as too broad". \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 18:04
4
\$\begingroup\$

Is it to "Review how the OP currently writes code in general, and advise how they could become better at writing good code".

The issue with this being the purpose is that it's less re-usable for others and it only serves to reduce clarity. As much as Code Review is focused on the individual's code writing, it is still a resource for others to come back to when they're doing a similar problem with a similar goal. Someone looking to solve Project Euler can read some previously posted questions here but it gets a lot more muddied when there are 4 other Project Euler unrelated questions in there. And it's even worse if the one they're looking for didn't get reviewed at all.

Is it to "Review a piece of code, and advise how it could have achieved its objective better".

Indeed, the acceptable scope should be based on the code. The code should have a unified purpose, and separate blocks should have some relation to the rest of the code. This makes it far clearer and easier to address whether or not the code achieves its particular goal because the reviewer only needs to talk about the one goal.

\$\endgroup\$
  • 2
    \$\begingroup\$ I think Barry puts it really nicely here: "The fact that you can invent an arbitrary umbrella that includes all 5 questions doesn't make them a coherent unit." \$\endgroup\$ – Vogel612 Dec 8 '15 at 10:54
  • \$\begingroup\$ Maybe, but at what point does an Umbrella stop being arbitrary? \$\endgroup\$ – Kaz Dec 8 '15 at 10:57
4
\$\begingroup\$

I mostly agree with Simon's assessment. It's about the Single Responsibility Principle. Does the question solve a single problem? Or can it be broken up into coherent, independent, unrelated subproblems?

Put another way, if you can split a question up into multiple on-topic Code Review questions, then I think it's too broad and should be split up into multiple questions. If those sub-units end up being too "stubby", then it's not too broad.

So this:

def euler1():
    ...

def euler2():
    ...

if __name__ == '__main__':
    euler1()
    euler2()

is too broad. I can split this up into two on-topic questions about the two, coherent, independent, unrelated problems.

On the flip side, a question about implementing the C++ std::map might be several hundred lines of code, but be one single problem for which there is no on-topic coherent subproblem, and so I would not consider that to be too broad.

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think the question is if you can split them up. There's a lot of code that can be split up. But just because it can be done, doesn't mean that it has to be done. Saying "You should split this up" can sometimes be better said in an answer than in a close vote. \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 16:04
  • \$\begingroup\$ @SimonForsberg You mean a comment? \$\endgroup\$ – Barry Dec 8 '15 at 16:09
  • \$\begingroup\$ No, I mean an answer. As I said, "You're violating SRP" is a valid review. "This program does too much" is also a valid review. \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 16:12
  • \$\begingroup\$ @SimonForsberg But we're talking about whether the question is too broad, not whether the program is. I don't see anything wrong with writing a program to solve every euler problem - but that's too broad for a review. \$\endgroup\$ – Barry Dec 8 '15 at 16:16
  • \$\begingroup\$ That wouldn't even fit in the char limits ;) But if the question is too broad because the program is too broad.... then who decides if the question is too broad? The reviewers, right? \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 17:57
3
\$\begingroup\$

If I may attempt to summarize a guideline into a soundbite:

We want to review code, not the programmer who wrote it, nor general practices/principles/concepts. If you cannot summarize the task accomplished by the code you posted in the title, then it is probably too broad or hypothetical.

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

Personally, I have always seen the purpose of Code Review as "helping people to write better code". This encompasses a wide range of advice, from specific questions to broad ones.


We specify that OP's must be open to advice on any and all aspects of their code.

As such, I tend to consider "Too Broad" in the context of what the OP wants out of their question:

If they want to optimise code along specific criteria (speed, memory etc.) then I think splitting questions into specific programs is beneficial.

If they want a general review of how they write code in a specific language, then I think having a broad sample of code is beneficial because it allows you to pick up wider stylistic tendencies that you couldn't pick out from a 5-line FizzBuzz. Whether it's technically all part of the same program is not something I care about.


As an asker and an answerer, as long as the question is clear and there is a common thread that's appropriate to what the OP is seeking, I don't mind if the code is part of a single program, or a collection of separate programs.

If someone posts 40 lines of code that solve the first 5 project Euler problems and asks for general feedback on how idiomatic their code is, I don't see the problem. If someone posts a series of small utility functions and asks for general feedback on their ability to write small utility functions, I consider it a perfectly appropriate and useful question.

I think if an On-Topic question is clearly articulated, and has received useful answers, then closing it is a net detriment to the community.

\$\endgroup\$
  • \$\begingroup\$ So you mean I could post a Sudoku Solver and a Maze Generation algorithm in the same question, as long as my main focus for both is "How can I make this faster"? \$\endgroup\$ – Simon Forsberg Dec 8 '15 at 16:13

You must log in to answer this question.

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