28
\$\begingroup\$

I have seen several (suggested) reviews lately, where someone edits the code in the original poster's question. Should such edits be allowed?

The edits in question are mostly minor. Some add spaces to the code to increase readability, others fix syntax errors in the posted code.

\$\endgroup\$
37
\$\begingroup\$

No

Personally, I think that they should usually NOT be allowed for questions.

This site is about code reviews. Readability is an important issue in code reviews, as are syntax errors. Editing the post is not constructive. It is much more helpful to leave a comment or answer that points out the shortcomings.

If the code in the question is edited, other reviewers will (normally) not see these issues with the code, and therefore can't comment on them.

Explicitly pointing out issues makes them more apparent to the original poster, and he or she will learn by doing when fixing the issues themselves.

As @200_success points out, replacing tabs with spaces is probably okay, but only to help the present the code as the author originally intended, not to improve it (i.e. it is OK to fix formatting issues that are related to MarkDown, not the actual code).

\$\endgroup\$
  • 11
    \$\begingroup\$ Syntax errors should not even be a part of code review. Code review is for code that appears to work. \$\endgroup\$ – svick Jul 16 '13 at 13:25
  • 2
    \$\begingroup\$ You're right. What I meant was typos that caused a bug but didn't cause an error. \$\endgroup\$ – Lstor Jul 16 '13 at 15:18
  • 1
    \$\begingroup\$ Edits that actually clarify the question though should be allowed. Perhaps approved in some way by the original poster. Or at least a veto power by the original poster. \$\endgroup\$ – vgoff Jul 20 '13 at 9:35
  • \$\begingroup\$ @vgoff The original poster is notified of edits, iirc, and can revert them if he or she doesn't like them. I agree regarding edits that clarify the question, but for CodeReview I find that very rarely includes code edits. \$\endgroup\$ – Lstor Jul 20 '13 at 10:14
  • \$\begingroup\$ Yeah, I agree about the code edits. There was an edit that i made that did involve code, but only after discussion on another site. It got obliterated with "I don't think it is right to modify other persons post to have it edited so that it is OK for review". Without any question at all to the reason for it. The OP was very new. Trying to help, I think he got the wrong impression. Partly because he was advised to post here for the kind of question he was asking on SO was more appropriate on CR. \$\endgroup\$ – vgoff Jul 20 '13 at 17:00
  • \$\begingroup\$ sometimes when someone posts code it doesn't look right because they haven't replaced their tabs with spaces. this kind of Editing is something that doesn't change what the Poster intended to be reviewed, this should be an allowable edit, but it draws a fine line, @200_Success's answer has clearly stated this \$\endgroup\$ – Malachi Apr 28 '14 at 17:13
13
\$\begingroup\$

Unlike Stack Overflow, at Code Review we are interested not only in a solution that works, but a solution that is well expressed as code. Therefore, we review all facets of the code, including whitespace. If you feel that the code in the question is improperly formatted, do not silently edit the question, but rather write an answer that addresses the problem.

An example of such an answer:

That said, I would make a narrow exception: it's OK to fix poor formatting in a novice user's question if there is clear evidence it is obviously unintentional. Usually, these formatting errors result from copying-and-pasting code into the Stack Exchange Markdown editor, and they fall into three types:

  1. The entire code is not indented at all, and therefore does not even appear to be a code block.
  2. The entire code block is indented by too many levels.
  3. The user has mixed Space and Tab characters. Markdown treats both four leading spaces and one leading tab as code block line. Therefore,

    SpaceSpaceSpaceSpaceint main() {

    Tabreturn 0;

    SpaceSpaceSpaceSpace}

    would be rendered as

    int main() {
    return 0;
    }
    

    making the code appear to be unindented or haphazardly indented, even though it looked fine in the author's IDE that had eight-character-wide hard tabstops.

\$\endgroup\$
  • \$\begingroup\$ I suggest that there could occasionally be a fourth kind of clear evidence: If the user has provided an external link (e.g. to GitHub) and the problems do not appear there. \$\endgroup\$ – Josiah May 4 '18 at 7:51
  • 1
    \$\begingroup\$ "The user has mixed Space and Tab characters. Markdown treats both four leading spaces and one leading tab as code block line." Perhaps this is the Python in me, but that actually sounds like something I'd want to bring up in a review. It's certainly something I'd want someone to mention in a review of my code. \$\endgroup\$ – Josiah May 4 '18 at 7:54
-9
\$\begingroup\$

Update

Given the comments I must not be explaining my point clearly.

  1. Good formatting is crucial to the readability of code (and sometimes syntax).
  2. Readable code with correct syntax is significantly easier to review.
  3. Everyone wins when there are more reviews.

So go ahead and critique the formatting in your answer, but please reformat the code in the question to encourage more reviews.

Original Answer

I feel editing to fix formatting is a net win for the community. Doing so makes it easier for other reviewers and provides a good example for the asker. While it may benefit the asker to do it themselves, they'll have plenty of opportunities later. Having a good example gives them something to model.

Additionally, I'm more likely to give a review if I don't have to paste it into my IDE to format it. And others reading the question won't be distracted by hard-to-read code. It also allows lurkers to help clean up the site without feeling pressured into a more detailed post.

Finally, I'm not too fond of any review that consists solely of a rebuke to format the code. Feel free to edit the question and include this in your answer, but don't make formatting your entire answer.

While consistent and clear formatting is very important, it's also quite trivial to pick up and point out. Let's not make this the primary focus of this site.

\$\endgroup\$
  • 11
    \$\begingroup\$ Formatting should not be a focus of a code review? I don't get this. If the question is a javascript function written on one line 100+ characters thats the first thing I will be pointing out to them. And probably the last because it is hard to review something I cannot properly read. \$\endgroup\$ – James Khoury Feb 4 '14 at 5:43
  • \$\begingroup\$ Formatting is important in several languages (VB, Python), so not to mention formatting and indentation in a review is irresponsible. on stackoverflow.com I am guessing that a majority of the time, there is a missing Semicolon or bad formatting causing someone to miss a missing semicolon or something like that \$\endgroup\$ – Malachi Feb 4 '14 at 15:57
  • \$\begingroup\$ @JamesKhoury While formatting is very important for readability and in some cases syntax, it should not be the primary focus. It sounds like we agree when you say, "it is hard to review something I cannot properly read." \$\endgroup\$ – David Harkness Feb 4 '14 at 20:33
  • \$\begingroup\$ @Malachi I said, "consistent and clear formatting is very important" and "include this [formatting] in your answer." It sounds like we agree. \$\endgroup\$ – David Harkness Feb 4 '14 at 20:39
  • \$\begingroup\$ there is good practice and bad practice. mostly we do point out the bad formatting, even when it is consistent, because it is bad formatting, if I come into code given to me by a predecessor I want to be able to read it, it should be consistent with formatting standards. \$\endgroup\$ – Malachi Feb 4 '14 at 20:47
  • \$\begingroup\$ @Malachi I agree wholeheartedly. So much so that I will often skip doing a review if the code is formatted too poorly and I don't have time to reformat it. For example, I often review code on my phone which is next to impossible if it's not formatted well in the question. \$\endgroup\$ – David Harkness Feb 4 '14 at 21:00
  • 4
    \$\begingroup\$ The fact that the formatting needs to be addressed is something that is part of the review, so it should not be edited in the question. The asker needs to see the difference. It makes for better coders. If the indentation is messed up because of copy paste neglect, then yes edit the question's code to fix it, otherwise review it and explain it to the poster. I feel for you reviewing on your phone but the op needs to know the fundamentals of coding. \$\endgroup\$ – Malachi Feb 4 '14 at 21:28
  • \$\begingroup\$ @Malachi The OP can see the diff in the edit (unless that requires higher rep?). Absolutely, add a link to code formatting guidelines in your answer and/or point out how you fixed it. I just want to see more reviews, and I think others are skipping posts with bad formatting. It can't be just me given how many coders I've met that are proud of their laziness. :) \$\endgroup\$ – David Harkness Feb 4 '14 at 21:54
  • \$\begingroup\$ people who are not members of our site cannot see the edits to the posts. or don't know about the edits. it makes sense that everyone should be striving for good formatting, we don't want the rest of the world to think that coders are slobs. \$\endgroup\$ – Malachi Feb 4 '14 at 22:43
  • 6
    \$\begingroup\$ The only circumstance in which I would agree with silently fixing whitespace in a question is when the poster obviously botched the copy-and-paste job due to unfamiliarity with the site's Markdown editor. In all other circumstances, the whitespace and formatting is fair game for reviewers to comment on, and therefore must not be changed. \$\endgroup\$ – 200_success Feb 5 '14 at 7:26
  • \$\begingroup\$ I agree that submitted code should not be edited to 'improve' formatting. What is 'improved' for the editor can be totally against the style of the poster. Rants about formatting can lead to 'holy wars'. For instance, I have never been impressed by the formatting 'standard' introduced by SUN for Java and I developed my own which I find far more readable. But, it's 'my' style and I don't try to force it on anyone else. If you want to 'suggest' a formatting style, fine, but don't edit the code to meet your style. Lets show a little respect. \$\endgroup\$ – Mark Ross Jul 5 '14 at 17:29
  • \$\begingroup\$ @MarkRoss I'm not talking about personal style like cuddling else or braces on a new line. Either form is perfectly legible. I'm referring to unformatted or broken-looking code like in this closed question (which isn't nearly as bad as it gets; I only went back a few pages). \$\endgroup\$ – David Harkness Jul 5 '14 at 19:33

You must log in to answer this question.

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