12
\$\begingroup\$

Obj-C wrapper for OpenLDAP

The user originally posted the top half of the code there. An answer was posted.

After the answer had upvotes and a comment from the questioner (noting his appreciation for the answer as a good answer), the question was edited to "add more context".

No where does anyone hint that more context might be needed, nor does the questioner imply that the posted answer might be misunderstanding something based on a lack on context. Yet the extra code was added (making the existing answer less complete).

I rolled back the edit. The questioner undid the rollback. And I pointed a mod at the question. The response was this comment:

Modifying the code in your question in response to a review ("fixing things") is strongly discouraged on Code Review. I see that your actual edits do not change the code, but add more context to the code. This has not invalidated the current answer, but does make the current answer less complete. For future reference see what you may and may not do after receiving answers

Again, I must emphasize that no one involved in the question made any suggestion that additional context would be helpful or was necessary.

So with that in mind, does the failure to rollback this edit open the door for chameleon questions on Code Review?

What's a chameleon question? A chameleon question is a question that changes over time. It takes a lot of back and forth with the questioner to get him to finish the question and mark an answer as accepted because he wants a single question to answer all of his questions when the question should instead be multiple questions. For example:

Q: I'm trying to do X, but I ran into problem Y.

A: Try solution Z.

Q: Okay, solution Z solved problem Y, so now I'm successfully doing X, but when I try to do A now, I run into problem B.

... and it goes on.

Should we allow chameleon questions? Is this question a chameleon question?

\$\endgroup\$
1
  • \$\begingroup\$ There is some ongoing conversation about this in chat. \$\endgroup\$
    – RubberDuck
    Dec 11, 2014 at 13:52

6 Answers 6

7
\$\begingroup\$

I think the mods did the right thing here in this specific case, but I understand your concern. This could be a slippery slope, but all in all, I see nothing wrong with adding code without invalidating answers. After all, the reason we don't allow edits to the code is to prevent answer invalidation. The edit doesn't invalidate your answer. It just makes it less complete, and answers are under no obligation to be "complete" reviews. In fact, a complete review is likely to be nearly impossible to begin with.

I feel OP's actions are to be discouraged, but do not violate any of our community's policies. You are under no obligation to answer a question to begin with, so you are certainly not obligated to update your answer either. Simply stop helping when a question becomes a chameleon.

\$\endgroup\$
7
\$\begingroup\$

I don't think that answer invalidation actually occurred here. Invalidation would be: "You're right, I hadn't thought of that idea. Let me incorporate it sneakily and hope nobody notices."

You made a valid observation that accepting NSString would probably make the method more usable.

The author therefore felt compelled to show a use case demonstrating how it wouldn't actually make a difference. (Is it possible that the use case was contrived just to disprove your advice? Possible, but in accordance with the Assume Good Intentions principle, I would assume that the use case is genuine.)

Therefore, it looks like this is a case of miscommunication and clarification, rather than a deliberate effort to misappropriate your advice. Miscommunication happens — that's life. Clarifying miscommunication is a good thing. "Oops, I didn't phrase the question as clearly as I should have" happens all the time on all Stack Exchange sites, and it's usually not something to get upset about.

I wouldn't say that we are establishing a new rule to allow chameleon questions by allowing the clarification to stand. Rather, it just means that we are carefully thinking about the nuances of each edit rather than blindly enforcing a broad no-code-changes rule.

I would also add that answers do not need to address all of the code in a question, so there is no such thing as invalidating the completeness of your answer.

\$\endgroup\$
5
\$\begingroup\$

Does this open the door for chameleon questions?

No, it does not.

This is a question where the code to be reviewed is perhaps easier to review if the code is put in to a bigger context.

This is not an iterative review, the code to be reviewed was not changed. The context code is the same as what it was before it was posted.

The core question is unchanged, and there's no chameleon here. The context has not changed either, the only thing is that now the context is available for reviewers to see.

Is this situation ideal? No. The context should have been provided before any reviews were added. But, it would also be wrong to deny other people the context that would perhaps be helpful for their reviews. Asking it as a new question would be a duplicate (there's no code change).

So, the bottom line is that now people who choose to review the code have a bit more context than the early review. That gives others an advantage (or not).

As much as that may be seen as 'unfair' to the early reviewer, it's also unfair to prevent the asker from clarifying the context of his code (whether he was asked to clarify the context is not significant).

So, in this case, two wrongs do not make a right: it was wrong to exclude the context, and it would also be wrong to deny the asker the opportunity to provide it.

There is no perfect (or even good) solution here, and my assertion is that allowing the added context results in the best quality for the site.

What you choose to do with that additional context is up to you. Providing a second answer, extending yours, or doing nothing all seem like reasonable options.

Note that many questions on Code Review provide code to contextualize the core code that is there for review. For example, I often include a 'main method' to run the code, and I don't want that main method reviewed. Other people offer test harnesses to run the code. Adding code as a context does not mean the code needs to be reviewed.

I have satisfied myself that the current situation is the best resolution, and that no, it does not open the door to 'chameleon' questions.

I will unlock the question, and move on.

\$\endgroup\$
13
  • \$\begingroup\$ I think the big problem here is that the context is provided in direct response to the answer. If my answer doesn't mention anything about NSString versus C-String, does the added context even get added? \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 15:42
  • 2
    \$\begingroup\$ While that may be the case, I really don't see it as a 'big problem'. It's a bit messy, but this is far from being the portent of impending doom that you appear to think it is. \$\endgroup\$
    – rolfl
    Dec 6, 2014 at 15:44
  • \$\begingroup\$ It's leaving a slippery slope. And now we've got this vague gray area where it's up to a moderator's personal opinion on whether or not harm has been done instead of what could easily be a more clear cut rule. Couldn't the policy simply be that context added AFTER answers should be provided as a link to an external site? It's trivial to post stuff on pastebin... \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 15:45
  • \$\begingroup\$ No, definitely not. Links to external sites should not be required. \$\endgroup\$
    – rolfl
    Dec 6, 2014 at 15:46
  • \$\begingroup\$ But in this case, the added context is 100% unnecessary. In this specific case, it actually adds nothing. All it adds is an open door for another review and making my answer look incomplete. \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 15:46
  • 1
    \$\begingroup\$ So then what's your problem with the context being added? \$\endgroup\$
    – rolfl
    Dec 6, 2014 at 15:46
  • \$\begingroup\$ The damage it does to my existing answer. \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 15:47
  • 1
    \$\begingroup\$ Whatever perceived damage inflicted on your answer happened because the context was not in the question to start with, not because it was added later. The context for the code has never changed, all that's changed is that you're now aware of it. \$\endgroup\$
    – rolfl
    Dec 6, 2014 at 15:50
  • \$\begingroup\$ Why do we disallow people to edit code that's already been reviewed? Even if it isn't to take into account an exist answer, we disallow it. We disallow it because it invalidates answers. \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 15:53
  • 1
    \$\begingroup\$ No, we don't and he didn't. \$\endgroup\$
    – rolfl
    Dec 6, 2014 at 15:56
  • \$\begingroup\$ We don't disallow people editing code after reviews have been posted? \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 15:59
  • \$\begingroup\$ We do 'disallow edits', but he did not edit his code, he added additional (context) code. Pedanticness for the win, it seems. \$\endgroup\$
    – rolfl
    Dec 6, 2014 at 16:17
  • \$\begingroup\$ But the reason we do it for the sake of preventing answer invalidation. \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 16:18
2
\$\begingroup\$

In my view, any question that has any upvoted answer should be considered frozen unless comments have asked for additional context. That's in keeping with this meta-post

\$\endgroup\$
4
  • 1
    \$\begingroup\$ I agree. While the edit didn't add revised code, it does add code that muddles the question. What happens if a new review is posted reviewing the code added for context? And then another level of context is necessary... And then that context level is reviewed, etc. the context code could've been provided on an external site such as pastebin. \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 13:29
  • \$\begingroup\$ I disagree with the "any question" part. Why should a lack of comments (which are freely removable/not important) and the availability of answers restrict me from showing you what some of my data objects look like? \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 12:55
  • \$\begingroup\$ Comments aren't "not important". I've posted plenty of answers that review comments, including my highest upvoted answer which discusses ONLY the comments. \$\endgroup\$
    – nhgrif
    Dec 11, 2014 at 13:24
  • \$\begingroup\$ After a lengthy discussion I still feel this answer is wrong, but only the basis of it not allowing textual context clarifications. \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:53
1
\$\begingroup\$

Chameleon questions should not be allowed. It's why we don't allow editing of the code.

Should we allow chameleon questions?

No.

Is this question a chameleon question?

Well, it looks like one, but I don't think it was intended as such.


Adding context is fine.

However, context should be in text, not in code. By making the context in code, you put it up for review, and that's not what we want to achieve.

Thus if you have only code-context, please transcribe its meaning and significance to English text (a set of passed test cases becomes "I have tested my code for x y and z and it passes the tests."). If you want to add the code to the question, please transcribe as above, then post a LINK to the code in the question.

If I have received an answer that states "you should use more comments, I can barely tell what these functions do", and I see that I do use comments, but I only put them in the header files, why should I be restricted from editing my question to add that "each of these functions has a description with its definition in the header"? With a link to the header files on github.

The header files existed before I posted the question, I just didn't think of them as significant for review. Now they do seem to be relevant.

What about invalidating answers by adding context?

How? How can you invalidate an answer by adding context? I know how. If that answer made assumptions.

Those assumptions should be allowed to be broken. "You don't deal with unicode" -> "Sure, but it doesn't have to support unicode. Our database doesn't support unicode either, and that's where the input comes from."

Had someone dispelled the assumption before the answer was there "This code doesn't have to support unicode" then the answer "You don't deal with unicode" would both be obvious and irrelevant. Yes, I know it doesn't support unicode, but as per the problem description, it doesn't have to. To make a point like that would be to say that "finding the smallest item in an array" doesn't print a list of primes. Yes it's true, but that's not a flaw of the code. It doesn't have to do that.

Adding context cannot invalidate an answer without breaking assumptions that the answerer made. Those assumptions are made because there's a lack of context, not because of the code under review.

\$\endgroup\$
13
  • \$\begingroup\$ So we shouldn't waste our time posting answers that cover things that the posted code is missing, because that assumes that it's actually missing rather than unposted, and it'd be perfectly valid for someone to add in code that 100% invalidates an answer that comments only on what's missing (regardless of whether or not the code was written before or after the answer, before or after the question). \$\endgroup\$
    – nhgrif
    Dec 11, 2014 at 13:17
  • \$\begingroup\$ @nhgrif It's perfectly valid to state that the code doesn't support unicode, but be aware you might get the response "sure, but it doesn't have to". \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:18
  • \$\begingroup\$ What if I've written a Fraction class that didn't include a subtract method (despite having the other 3 operations). If a reviewer posts an answer stating that my class should have a subtract method, would it be okay for me to then edit my answer to include the subtract method? After all, the reviewer ASSUMED that I hadn't written it... but for all anyone knows, I did write it but forgot to include it. \$\endgroup\$
    – nhgrif
    Dec 11, 2014 at 13:18
  • \$\begingroup\$ @nhgrif sure you can add it in at the bottom of your question. Don't edit the block of code you've already posted, put your context before or after the actual code up for review. \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:20
  • \$\begingroup\$ @nhgrif The important difference here is that the actual goal of the review doesn't change. To add a subtract method and say "review this too" is not allowed. That'd be moving the goalposts; a chameleon question. To say "Oh, I forgot that, here it is" is perfectly allowed. \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:22
  • \$\begingroup\$ But ANY AND ALL code in a question is to be reviewed. That's a long standing tenant of Code Review. But the point remains that if my review says "This very important piece of the puzzle is missing and I can't see why you wouldn't include it in this class. He's how you can implement it and here's all the reasons why it should be implemented." and then after seeing the answer, the questioner can than just edit that into the question and that's okay despite completely invalidating the answer that talks about how bad it is that it is missing? \$\endgroup\$
    – nhgrif
    Dec 11, 2014 at 13:24
  • \$\begingroup\$ @nhgrif additionally, just because something is ON-topic does not mean it's good. When you say "please review my public api" and someone responds with "you can only set things, but not get them? WTF?" and you say "ah, I forgot the getters, here they are", then you're basically being stupid. Downvote the question. \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:24
  • \$\begingroup\$ @nhgrif would you not allow someone to add "wrapper main method, for running this code snippet standalone" to his question just because there's already some answers? Would you not allow someone to post his testcases to show that he has tested his code? Why not? \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:25
  • \$\begingroup\$ I'm trying to point out that what this meta answer states is 1) it's perfectly acceptable to invalidate answers as long as it's done under this weird guise of "adding context" that only becomes necessary after you see a posted answer and 2) we should never post answers that point out what someone's code is missing because of point 1 which makes your answer perfectly invalidatable, which used to be something we tried to prevent around here. \$\endgroup\$
    – nhgrif
    Dec 11, 2014 at 13:26
  • \$\begingroup\$ To the text of his question, after it already has answers? If there is something about the code that requires more context, then it should not be answerable, should have no answers, and should be put on hold. We don't review non-working code... so why should a question whose workingness is in question have any answers? \$\endgroup\$
    – nhgrif
    Dec 11, 2014 at 13:28
  • \$\begingroup\$ @nhgrif just for clarification's sake, I'm willing to concede that 1) This "adding context" is not ideal. 2) Repeatedly adding context is a sign of a bad question. 3) Perhaps "context code" should not be reviewable at all. 4) Questions should be closed if they lack context, but this takes time to do and people will post answers to off-topic questions if they're not closed fast enough. The system isn't perfect. \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:32
  • \$\begingroup\$ Your comment still leaves problems. If point 3 is true, then context code must not be added to a question after it has answers. Again, we have a long standing rule that all code is reviewable. If the context code is in more desperate need of a review than the other code, then what does someone think who looks at my answer 2 months later and it's not clear that my answer was posted before the context code? \$\endgroup\$
    – nhgrif
    Dec 11, 2014 at 13:34
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$
    – Pimgd
    Dec 11, 2014 at 13:35
0
\$\begingroup\$

Even if this isn't a chameleon question, the code added in the edit is at least an attempt to invalidate the existing answer.

In this specific case, the added "context" code is only added in direct response to the existing answers comment on NSString objects versus C-Strings. If that answer doesn't include that commentary, does the context get added? Almost certainly not.

But the commentary was included, and the questioner disagrees that assessment. Rather than simply not implementing that suggestion (no questioner is even remotely close to being under any obligation to implement anything suggested in any answer ever posted), or questioning my suggestion in a comment, or inviting me to chat, or any other question, the questioner posted "additional context" in what can be perceived as nothing more than an effort to make every comment I made on NSString versus C-String completely invalid.

Now, as someone who donated my time to helping some random stranger on the Internet write better code, I'm left with a few options:

  1. Do nothing to my answer. If the questioner was right and that the additional context invalidates some of my comments, then I look like an idiot in a month because it is very unclear to anyone reading this even a week later that my answer was written without this additional context.
  2. Update my answer (or add a new one) to demonstrate that even with the additional context, my original point still stands.

If scenario 1 is the case, then I'm going to be highly discouraged from posting answers, because I'd rather not open myself up to the opportunity to look like an idiot because someone changed the context in which something I posted.

If scenario 2 is the case, then why don't we allow people to edit a revision of their code into questions? Why do we force them to open new questions? It's because Stack Exchange isn't a forum. We don't want a massive back and forth of questions and answers. We want clear questions and clear answers.

Allowing users to add context (especially in this specific case where it actually adds literally nothing) means that we're adding reviewable code to the question. And when a new review gets posted about the added context, why won't the questioner post even MORE context to clarify the context that was supposed to clarify the original... and so on and so forth.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You missed option 3. Delete your answer and forget it. =;)- \$\endgroup\$
    – RubberDuck
    Dec 6, 2014 at 16:13
  • \$\begingroup\$ I guess that's a third option, but then I, as the volunteer answerer, am still punished one. \$\endgroup\$
    – nhgrif
    Dec 6, 2014 at 16:18
  • \$\begingroup\$ Yes, but it also punishes OP by removing their ability to reference your voluntary help. (Assuming OP can't view deleted posts. I do hope that anyone with that kind of rep knows better though. ) \$\endgroup\$
    – RubberDuck
    Dec 6, 2014 at 17:26

You must log in to answer this question.

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