35

This is related to Is it okay to code-review and improve OP's code when posting an answer?, but I'm not sure if in certain cases I must change other parts of the code.

When the user is asking a question with some code that doesn't work, and I give a modified version of their code that works, should the code be refactored and errors (more like anti-patterns, the well known wrong way of doing something) that they did not ask about, be solved?

I ask this after noticing some users posting a very modified version of the OP's code, and they also comment on my answer about why I didn't change X to Y or something, which wasn't asked about in the question.

Using this pseudo example:

Hi, my function code_with_error_user_talked_about is not working, I tried this and that, and I have the following code:

some_random_code()
anti_pattern_code_that_can_cause_unexpected_behavior()
code_with_error_user_talked_about()

Do I need to address the other issues? Can my answer be directed towards code_with_error_user_talked_about which is what the user is talking about? My view is that they could have plans for their other code, and their own design decisions aren't my concern. I couldn't find something talking about this.

11
  • 22
    Why wouldn't you want to point out anti-patterns or issues that can cause unexpected behaviors?
    – charlietfl
    Commented Aug 31, 2021 at 0:52
  • 59
    "Do I need to address the other issues?" - No, you don't need to, but there's nothing to stop you if you'd like to. Potentially worth separating that into a footnote on your answer though, as the actual answer to the question asked is more important. Commented Aug 31, 2021 at 1:00
  • 1
    @charlietfl Would be better to at least point it out? Or the code I send be also using them? I don't mind changing the code as such. Sometimes it involves a substantial change, I have been taking a look on questions related to a specific framework, and sometimes if the user decided to do x(which is anti pattern) instead of y, changing it back will return a complete different code for them, comparing to just their issue
    – iunfixit
    Commented Aug 31, 2021 at 1:03
  • 1
    @Nick if it would be good I will start adding notes when I spot something odd, I didn't want to turn my answers off topic so I kept them focused on the user question
    – iunfixit
    Commented Aug 31, 2021 at 1:06
  • 16
    Keeping in mind that your answer could well be visited in future and straight copy and pasted out as an example of how to do X, yes I would say you are best off improving any bad practices. Viewers (including potentially OP) are going to see your answer and assume its all good code. Therefore definitely best to make sure it is.
    – Dale K
    Commented Aug 31, 2021 at 2:39
  • 5
    Some relatively minor things, I'd personally might leave out. But serious problems, I would tend to mention as "Here is how to do X. However, that is problematic, it might be better if you did Y". A very serious problem I'd address upfront "Doing this is a bad idea for X, Y, and Z reasons. Use this instead. If you really need to do it your way, then you can do that.".
    – VLAZ
    Commented Aug 31, 2021 at 5:45
  • 4
    Maybe the user that asked that question did not really provide a minimal example. If only minimal examples were provided, any problems with the code are likely part of the problem and must be fixed anyway. Commented Aug 31, 2021 at 6:48
  • 9
    I think it depends. Personally, I find that if I notice other problems in the OP's code I will likely add notes to my answer stating they are problems; and perhaps suggest they ask a new question about about that problem (or link to a duplicate question about it). If it's a major problem, such as SQL injection which I see often in the tags I follow, I will address it in my answer as such things (in my opinion) are a plague that should have died out years ago (I also personally tend to downvote answers that completely omit even a warning about such dangerous code and leave it in situ).
    – Thom A
    Commented Aug 31, 2021 at 8:57
  • 6
    At the end of the day it's what can differentiate an answer, from a good answer, to a great answer (and possibly a bad answer), in my opinion.
    – Thom A
    Commented Aug 31, 2021 at 8:57
  • It depends on how much context they give you. Sometimes it's impossible to present a valid refactor without knowing the full context. If you have everything you need, then it's probably a good idea to always promote good coding practices. If your answer ever appears on Google, I'm sure many will appreciate the input. Commented Sep 1, 2021 at 13:04
  • 1
    If your refactor introduces new bugs or is above the skill level of OP then is it useful? If you don't meticulously comment your refactor so that OP understands WHY you wrote the code which you wrote then it's about as good as a link-only answer. You should strive to answer the question directly and then supply a re-factor at the end of your answer. Clearly distinguish it using a horizontal line.
    – MonkeyZeus
    Commented Sep 1, 2021 at 15:00

5 Answers 5

26

My two cents on how a good contributor should think:

  • Step #1: Make sure the question is complete and that you understand the OP's task. If you can't boil the question down to a single error or single technique then it probably needs to be edited or closed as Needing More Focus instead of being answered.

  • Step #2: If the question was posted after 2015, then it is probably a 5x-duplicate. Find the duplicate that provides the best advice and hammer it shut. If you can provide valuable, unique advice on the dupe target, go there and post an answer before moving on.

  • Step #3: If the question can be closed for any other reason, close it.

  • Step #4: Make sure you aren't getting sucked down some silly XY Problem rabbit hole. When you understand the input and the expected output, ask yourself how you'd solve it -- professionally. If this is a complete deviation from the OP's code, then deviate and explain why your recommendation is any combination of more reliable, secure, efficient, readable, maintainable, etc.

  • Step #5: Don't write dodgy code and append cautionary advice against the coded technique (I'm sure I did this in some of my earlier answers). Don't tell people that your code shouldn't be 100% followed. If you don't want to go to the trouble to code up whole chunks of anti-pattern, then write some non-code text that says what should go there instead of the anti-pattern.

    If only code_with_error_user_talked_about() is broken, only mention that in your answer. Give the rest "the silent treatment". Others can comment under the question about that.


As for the related topic of whether the OP's posted question should be altered/refactored to demonstrate better coding practice -- I'd say "No".

I can't think of the last time that I tried to polish a question's code/logic. Maybe this is because I also spend time on Code Review, but I prefer to keep all code in questions "as-is". We want people to see anti-patterns occurring in the wild. We want researchers to see that when they post questions with anti-patterns on SO that they will get peppered with comments that explains that their, say, "your SQL is vulnerable to injection attacks, use prepared statements" and "HTML should be parsed with a DOM parser instead of regex". It is healthy to have continuous reminders that bad code can be dangerous -- let's not hide bad/dangerous code, it represents an opportunity to educate.

20
  • 3
    The step by step helps to understand the process, the current how to answer introduction is a bit abstract
    – iunfixit
    Commented Aug 31, 2021 at 11:06
  • 1
    I very much agree with you @iunfixit meta.stackoverflow.com/q/406984/2943403 Commented Aug 31, 2021 at 11:17
  • 7
    "close" + "close" + "close" x3 in the first 3 Steps sounds to me more like "a zealous Curator" than "a good Contributor"..., and probably requires 10k or 20k Rep anyway... (@OP "only" has 600+ Rep...) - A good Contributor will help, answer and promote Quality + Good Practice...
    – chivracq
    Commented Aug 31, 2021 at 18:30
  • 4
    @chi if we are name calling and stereotyping, then I have to assume that you are in the "never close, always reopen, never delete, help desk, infinite content redundancy is good" camp. After more than a decade of feverish Q&A engaging hundreds of thousands of brilliant professionals to generate MILLIONS of answers, it is statistically very, very difficult (I want to say nearly impossible) to ask a new, basic, narrow, on-topic question on SO. Closing is not a punishment, it is apt resolution that respects the system first. Commented Aug 31, 2021 at 20:56
  • I see the "close" as a downvote and move on, and flag if appropriate, as a new user without rep, and it seems more solid to me, a while ago I answered a question with a typo, without really knowing about this kind of questions, and then from now on I ignore them after getting to know these answers are bad, but still see users doing the same(sometimes without getting downvoted), some rules feel very arbitrary, close/downvote situations draws a line easier to understand
    – iunfixit
    Commented Aug 31, 2021 at 21:11
  • 3
    The decision to close and the decision to downvote should be completely independent. There are non-closable questions that absolutely deserve to be downvoted. There are closable questions that do not deserve to be downvoted. Questions that are closed and downvoted as usually purged by the Roomba -- which is a critical feature to keep this site clean. Questions which are answered and closed as duplicates cannot be removed by the Roomba -- so humans must do it manually when necessary. This is why we must not answer duplicate questions. Commented Aug 31, 2021 at 21:42
  • 2
    @iun If you are a member for 9 days and have over 600 rep, there's a pretty fair chance that you have been answering duplicate or otherwise closable questions. I know I answered lots of closable questions when I started -- because the Stack Overflow doesn't have a clear help page that states the importance of considering closure BEFORE considering answering. Commented Aug 31, 2021 at 21:44
  • I apologize if I did on the first days, just lately I got to understand the "future readers" aspect of the website, don't wanna get off topic but I also got access to the review queues, and I notice some answers are duplicate of others, which there is no action for, but as I'm new to the website I may just ignore queues until I understand everything better
    – iunfixit
    Commented Aug 31, 2021 at 22:04
  • 2
    @iunfixit there is no substitute for experience with a lot of thecworkflows, etiquettes, and culture in the Stack Exchange. Never stop yourself from doing something because you are inexperienced -- then you'd never do anything new. Ask others to explain what doesn't make sense to you (as you've done here) be strong enough to disagree when you don't agree with a policy/behavior. The world doesn't need more sheep, it needs more shepherds. Commented Aug 31, 2021 at 22:14
  • 1
    Oh, and when you see an answer that is at least an hour older than another answer on the same page and the newer answer adds ZERO new value to the page, downvote it and create a custom flag on the redundant answer and explain to mods (who may not understand programming language) that the answer is identical to the specific earlier answer. When you give clear, actionable flags to mods, then can remove the unnecessary answer regardless of the upvote tally. @iun Commented Aug 31, 2021 at 22:17
  • @mickmackusa because I don't think you should consider closing if you have an answer to contribute. A lot of questions get closed because they're "kind of" duplicates, even though the user is asking in a different way or has a slightly different requirement. A good answer can be instructive in that situation, and can obviously link to existing good answers to do the bulk of the work. In my opinion, having 3 similar answers to 3 similar questions over the course of 9 years, that all cover slightly different aspects and solutions, is better than a single canonical question that is stuck in 2014. Commented Sep 1, 2021 at 0:34
  • 2
    @shadowtalker No one should ever be silenced by closing a question as a duplicate. If you find a question that you would like to answer (AND your insights are new to Stack Overflow) but the question itself is already resolved by an earlier page, then close the redundant page and add your advice to the dupe target page. This is EXACTLY what SO needs -- better eggs in fewer baskets (not just more baskets). If a page from 2014 doesn't have the best advice -- GREAT -- you found a great place to contribute a new valuable answer! Questions should be treated as timeless. Commented Sep 1, 2021 at 3:03
  • 3
    @mickmackusa What does that have to do with the question posed here? It's clearly talking about answering, not edits. And how does that square with giving the rest of the code the "silent treatment"?
    – jpmc26
    Commented Sep 1, 2021 at 8:18
  • 1
    Is the misinterpretation here "don't alter the code in the question" is being read as "your answer shouldn't change the code in the question"? That's not what I got from what mickmackusa said, but the responses seem to point that way IMO
    – Kevin B
    Commented Sep 1, 2021 at 20:04
  • 1
    @mickmackusa Sorry, I thought we were all on the same page that we're not talking about editing OP's question but rather providing an answer with refactored code.
    – MonkeyZeus
    Commented Sep 1, 2021 at 20:07
33

Up to you, frankly. Depends on how much of your time you are willing to spend educating a user. If you know what the OP is using is an anti-pattern, feel free to rewrite it if you feel up to it and point it out in your answer.

But whatever you do, please do not forget that we are not here to help the OP per se.

We are here primarily to help potential future visitors that will come after the OP by spreading the collective knowledge in easily digestible chunks (also known as answers), all the help the OP gets is a side-effect of that effort.

By taking the above as your modus operandi, you will soon notice that a lot of questions like the one you asked stop becoming existential dilemmas while the quality of your posts increases further.


To address some specific points from your question:

Do I need to address the other issues?

No, you are certainly not required to address any issues beyond what the question is asking, even if this is an error that will cause a failure at compile/run time. But addressing this, even as a remark, is part of "being a good citizen" and will definitely be looked at positively by other readers.

2
  • 4
    Agree when it comes to anti-patterns, but when you notice potential security vulnerabilities, always mention those. Commented Aug 31, 2021 at 23:52
  • Eh, for what it’s worth I strongly believe that this is wrong, except in the trivial and irrelevant sense that we can’t force anybody to do anything here. But ignoring obvious anti-patterns in OP’s code makes the answer objectively worse. There’s no way around this. — I also disagree with the priorities, although I know that this view is fashionable. I very much make it my priority to help the OP in a given situation (but that doesn’t make it more acceptable to ignore bad code when providing help). Commented Sep 2, 2021 at 20:16
18

You can do however you want, but don't give the impression that the anti-pattern is good

You don't need to, but it's completely ok to address the issue.

However, if you post a fixed example of op's code, then I would suggest either fixing it with commentary or leaving it out. If your answer looks like this:

some_random_code()
anti_pattern_code_that_can_cause_unexpected_behavior()
fix_that_removes_the_bug()

then you will give the impression to future visitors that the anti-pattern is good code. Depending on the situation, this can be worth a downvote. In this case, you can use only the fix_that_removes_the_bug() line. If you include the anti-pattern, at least comment on it.

Editing the question

There is one option you can choose, but it requires much caution. You can edit the problematic part in OP's question. But if you do, you have to be really, REALLY sure on some things:

  • OP is not in the process of editing the question
  • The anti-pattern does not have anything to do with the problem in the question
  • Editing out the anti-pattern does not invalidate any existing answers
  • The anti-pattern really is bad code and not just something that is a matter of taste
  • The anti-pattern is bad in this particular situation, and not just in general

I'd advise extreme caution when doing this. Don't do it unless you're completely sure about what you're doing. And don't even bother unless you have more than 2k rep. The chances of getting the edit approved are extremely low.

Also, before editing a question like this, I would wait at least a day. Do this after OP is satisfied with the answers. And you definitely should avoid editing posts while the author is doing the same. It's extremely annoying when you get an edit collision while fixing your post.

After that, the focus should be on the future value of the question. I would not do this for any question, but only those that I think will have some potential of becoming a canonical question. In most cases, a more suitable solution is to find a duplicate.

9
  • 4
    I am on board with omitting antipatterns from answers -- 100%. I don't even like commentary within the answer body that say things like "but of course you should use a prepared statement instead" or "but professionally you should use a DOM parser instead of regex". I mean we should ONLY post content in answers that represents the best of our understanding. If you don't want to refactor the code, then don't, but don't put any garbage code in your answer either. I prefer not to touch questions with anti-patterns -- researchers should never copy code from questions if they like quality. Commented Aug 31, 2021 at 8:15
  • 2
    @mickmackusa Of course one should not blindly copy paste. But any code in an answer should be good code unless it's explained why it isn't.
    – klutt
    Commented Aug 31, 2021 at 8:18
  • I think I will also comment on answers that use anti-pattern when I'm sure it is wrong if that's the case, so not showing future readers bad code on the answers section is the way to go
    – iunfixit
    Commented Aug 31, 2021 at 21:16
  • @iunfixit That sounds good to me. You don't have to fix that anti-pattern stuff, but it's not good to replicate it in your answer code, especially if it contains a security vulnerability.
    – PM 2Ring
    Commented Sep 2, 2021 at 7:49
  • 2
    @PM2Ring Me and many others disagree. You should RARELY edit others code, and be extremely cautious when you do it. But "never" is a word that's too strong. As soon as OP has gotten their answer, we should focus on the value for future readers.
    – klutt
    Commented Sep 2, 2021 at 7:54
  • My philosophy is that if the MCVE / MRE needs editing then we should use comments to help the OP to fix it before we start posting answers.
    – PM 2Ring
    Commented Sep 2, 2021 at 8:05
  • @PM2Ring Thanks for the feedback. I have clarified it now.
    – klutt
    Commented Sep 2, 2021 at 11:37
  • 1
    For bugs in code in questions unrelated to what they're asking about (common with GNU C inline asm questions), I sometimes just fix it, but more often just leave a // unsafe: XYZ reason in the code block, so anyone looking at that code block as a potential example to learn from will be alerted. If I post an answer to the question, I'll mention it there, too, and talk about what to do instead. (GNU C inline asm is hard enough to learn without even more bad examples floating around; it's very easy to write code that only happens to work, and could break with other surrounding code.) Commented Sep 2, 2021 at 21:16
  • 2
    Also note that this meta question wasn't even suggesting editing code in questions, only whether to leave bad or silly stuff unchanged when you copy/paste a code block into your answer to start editing to address the question asked. (It's not 100% clear from your answer that you're introducing this idea, rather than talking about an idea proposed by the question). I think we should only edit questions for subtly-dangerous things (my personal pet peeve being unsafe inline asm because it's widely misunderstood; many people just hope it works the way they want, when it very much doesn't.) Commented Sep 2, 2021 at 21:24
16

This should really be a comment but I think it's important enough to call out as a full answer (and elaborating on @oleg-valter ) ...

As @oleg-valter notes,

We are here primarily to help potential future (sp) visitors that will come after the OP by spreading the collective knowledge in easily digestible chunks (also known as answers), all the help the OP gets is a side-effect of that effort.

I wish S/O community still maintained that purity of vision and purpose. Yet, we know too many people just (google) search StackOverflow for any answer that works and use that, often completely out of context, with implications. This is well documented by Stack itself :

Copying code from Stack Overflow? You might paste security Vulnerabilities, too

and has been widely reported (not that those same people would find the reports):

and backed by scientific study: An Empirical Study of C++ Vulnerabilities in Crowd-Sourced Code Examples

Worse, the problem is now being perpetuated by our future AI overlords (and acknowledged by their own hype machine!):

So,I would ask, why are you contributing to StackOverflow? Do you want to feed the OP a fish or are you going to teach them how to fish? I regularly ask in comments to provide more clarity or point out issues, even when I don't have the answer, in the expectation, better questions will get better answers and to reduce the spread of bad information (sounds like a social problem now).

On the flip side, posters of questions and answers should be open to accept "critiques" as well as answers, which can be tough, but they are asking for help, so ... ? I often see posts, "What is your question, please explain WHY the answer works" as well as many elaborations in comments. At the same time, you're not getting paid to help, teach or feed the world, so no one should be obliged to to do more than they feel comfortable.

It's fine line, but the cost benefits must be weighed, to your time, their knowledge and to the greater community. If you see issues and don't call them out, that's a problem. If you refactor w/o explaining why, that's a problem. If you can ultimately answer their question (How do I / What's wrong here) with an answer that does it all - "OP asked X, I understand what your trying to do to get X and here's how I'd do that (the refactoring) and why, that benefits everyone.

3
  • 7
    We should all acknowledge the trickle down effect of "giving a crap". If volunteers start caring more about quality instead of posting speed, everyone wins. Every time you care more than normal, you help to raise the bar for other contributors. This is why it is so painful to see code-only answers -- they only lower the bar. Commented Aug 31, 2021 at 8:19
  • 4
    on an off-note - you know what would be even better? The company starting to care about volunteers :)
    – 0Valt
    Commented Aug 31, 2021 at 8:20
  • 3
    And yet, we all still somehow continue to contribute. Are we fools to do so, too good in our nature and gernous in our time, addicted to the "Gamification of the Q&A" ? I haven't been 'directly affected" by the well vocalized and legitimate complaints of the mods, volunteers and community, so won't delve into that and stick to the issue of editing posting questions and refactored solutions in this thread.
    – Ian W
    Commented Aug 31, 2021 at 8:35
6

When I'm answering a question that looks like:

some_random_code()
anti_pattern_code_that_can_cause_unexpected_behavior()
fix_that_removes_the_bug()

I regularly fix it. My answer will look something like:

I removed most of your initialization code; it doesn't really affect what you are asking at all. After my refactoring, I reduced it to:

SomeInitCode();
MaybeALittleMore();          //dropping 40 lines to 4

Your code that deals with the XYZ Stuff can cause issues. You should read up on SQL Injection and the Justinian plague to see what I'm talking about. Instead of what you have, I did the following (note that I removed the dependency on a database and your table schemas, replacing it with...).

ReplaceThatAntiPattern();

Finally, this is where your issue is. You are trying to remove the engine from the car without opening the hood. You need to first:

DoTheFirstSteps();

Now, with that initialized, everything should just work:

ShinyNewCode();
MoreOfThatShineyCode();

Finally, when I run the code from a CMD window, above I get:

Hello World
Bonjour le monde
Etc.

I try to answer the OP's question and leave code that I'm proud of behind.

You must log in to answer this question.

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