6
\$\begingroup\$

This question got sparked by a recent discussion in the 2nd Monitor about this question: Golang Map-Reduce Master - Channels.

The problem is: How much context is needed to actually review the code in the question? How much should the reviewer assume about functions/types/object states which are referred to in the code but not included in the question itself?

In this case, it could be looked up on GitHub, because the code in the question was part of a larger project. Should this be required?

Also, the correctness of the implementation depended heavily on object state being setup correctly (otherwise it would deadlock). However, the setup code isn't included in the question. Without any mention by the OP other than "[it] passes the tests", how far should the reviewer go in assuming that everything got setup correctly? (Especially if the code could be called from anywhere, i. e. setup isn't guaranteed to be done.)

EDIT: After there had been some uncertainty in the comments, I'd like to clarify my problem. I'm mostly concerned about major dependencies.

These might be (heavy) dependencies on (at least for the reviewer) unavailable code, especially if there is some non-obvious communication going on (e.g. setting some globals that then get referenced later on) or some of the actual core logic happens there.

This could also be unstated pre- or post-conditions that would otherwise be hard to infer just from the code.

We as programmers and reviewers rely a lot on having as much context available as possible. And while it isn't required to have the whole context available to review a piece of code, a substantial fraction is. Sometimes some of that context can be inferred or some educated guesses can be made, but where is the line between not enough and just enough available information?

\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

This is one area where Code Review is different from doing reviews on pull requests for your own project - because we're not immersed in the same code-base as the asker, we need a bit more help to know how and why their code is to be used.

If you know the language and any libraries mentioned, but you can't make a reasonable guess as to what the code is doing, then I'd say it doesn't have enough context. (If you don't know the implicit context of the language/libraries so well, and the question seems borderline, then move on without voting).

If there's no statement at all about what the code is intended to achieve, that's definitely lacking context - the canonical expression of this is, "I have this function; can I make it faster?" If we're to nail down a good threshold for what's sufficient context, we probably need to start from there on one side of the line, and from an unarguably complete question on the other side of the line, and see how far we can advance from those until we stop with a much narrower grey area separating them.

I try to be generous where there's doubt, and will mention in the answer if I've had to guess (e.g. "I assume that vector is actually std::vector; remember to #include <vector> for this" in C++ code, or "It's not clear whether you expect the inputs in a specific order on the stack; I'm guessing that the smallest will be at the top" in a dc program). But sometimes the whole review would be guesswork, and would have to be rejected for context.

Remember that putting a question on hold is a good thing, as it gives the asker a chance to improve it before it gets answered. But it's important to give guidance on how to improve the question, as a comment - without that, it can be perceived as unwelcoming (to use a currently topical term). I've seen many incomplete questions resurrected in much better form, so let's take care to guide and encourage their askers!

\$\endgroup\$
3
  • \$\begingroup\$ I tried reducing the grey the other day, it's hard to do. There's so many things you have to think of, and not everyone is likely to agree on which are on and off topic. \$\endgroup\$
    – Peilonrayz Mod
    Aug 15, 2018 at 13:55
  • \$\begingroup\$ Yeah, I think there always will be some subjectivity. This answer is more a dump of my scattered thoughts than any coherent position, I'm afraid. It's really not an easy one; I think we perhaps need some canonical (invented, to save embarrassment) questions that consensus agrees are one or other side of the line, then wing it on the rest... \$\endgroup\$ Aug 15, 2018 at 14:11
  • \$\begingroup\$ Hmm, maybe a poll would be a good idea. \$\endgroup\$
    – Peilonrayz Mod
    Aug 15, 2018 at 14:17
4
\$\begingroup\$

I think that an excerpt from my answer to Thomas Ward's meta question provides a good starting point for nailing down the whole "context" thing.

This is how I feel about context personally.

This may or may not be how Code Review feels as a community


The Last Piece of the puzzle

Context.

This seems to be where most of our contention is when closing questions, we need the Context thing nailed down a bit more.

My thoughts are that if there is enough, for someone who knows the language and the libraries that are being used, to understand what the OP is trying to accomplish and the post fits all the other on-topic reasons, then let's review it.

\$\endgroup\$
23
  • \$\begingroup\$ Isn't that still quite open to interpretation? \$\endgroup\$
    – Mast Mod
    Aug 14, 2018 at 13:26
  • 3
    \$\begingroup\$ @Mast is that bad? \$\endgroup\$
    – Vogel612
    Aug 14, 2018 at 13:30
  • 1
    \$\begingroup\$ @Vogel612 I think so in the same way that 'broken code' was bad, it sets a precedent which isn't true for the OP and they get needlessly annoyed. \$\endgroup\$
    – Peilonrayz Mod
    Aug 14, 2018 at 13:39
  • \$\begingroup\$ what precedent? I am not following \$\endgroup\$
    – Malachi Mod
    Aug 14, 2018 at 13:40
  • \$\begingroup\$ @Malachi When a user thinks something along the lines of: "I know the language, and I understand the libraries being used. And I've included enough to understand my code. This is not off-topic". \$\endgroup\$
    – Peilonrayz Mod
    Aug 14, 2018 at 13:42
  • 2
    \$\begingroup\$ "What is your code doing for your application" that is the context of the code, the rest of the close reason speaks to how much code that you need. this close reason is a mesh of several close reasons because we can only have so many custom close reasons. \$\endgroup\$
    – Malachi Mod
    Aug 14, 2018 at 13:48
  • 3
    \$\begingroup\$ My problem is: How can you tell that the code in question does what it is intended to do if the core logic is either in external parts or depending heavily on them? (Especially if those external parts are not a publicly available library, i.e. just another part of the same project). \$\endgroup\$
    – hoffmale
    Aug 14, 2018 at 13:54
  • 1
    \$\begingroup\$ that is not a context issue, that is one of the other 4 reasons inside that close reason, I would say that there is code missing that is needed, I may call that "stub" code or something similar, @hoffmale \$\endgroup\$
    – Malachi Mod
    Aug 14, 2018 at 13:58
  • \$\begingroup\$ I think we should draw the line at putting dots in your code to show that there is code not being included, or when you include a non-standard library / module that may behave differently than expected. Both show lack of context that would be undoubtably wrong. \$\endgroup\$ Aug 14, 2018 at 14:00
  • \$\begingroup\$ that is not lack of Context, @FreezePhoenix. please see --> codereview.meta.stackexchange.com/a/3652/18427 under stub code \$\endgroup\$
    – Malachi Mod
    Aug 14, 2018 at 14:04
  • 3
    \$\begingroup\$ I would catagorize stub code as the lack of context, because it doesn't show all that is being used or done / directed. But, I see what you mean, and realize that your 'lack of context' means little to no explanation, and/or not telling what the code does, and/or not telling how the code would be used/ when it would be used, and/or why it should be used. \$\endgroup\$ Aug 14, 2018 at 14:07
  • 1
    \$\begingroup\$ @Malachi Where did I say code only? You are saying only text can say what, where code does that too. \$\endgroup\$
    – Peilonrayz Mod
    Aug 14, 2018 at 14:15
  • 1
    \$\begingroup\$ I have unfrozen Discuss Close Reasons chat room for this conversation. \$\endgroup\$
    – Malachi Mod
    Aug 14, 2018 at 14:18
  • 1
    \$\begingroup\$ @Malachi UCWYA is for text only, and the lacks concrete context close reason is about the code. You're trying to bundle them into the same, where we're just talking code. \$\endgroup\$
    – Peilonrayz Mod
    Aug 14, 2018 at 14:19
  • 1
    \$\begingroup\$ A lot of code posted isn't actually production code @Malachi \$\endgroup\$
    – Mast Mod
    Aug 14, 2018 at 14:35

You must log in to answer this question.

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