The suggested edit by a new user added a fclose(secretFile);, so that the function in the accepted answer does not leak a file descriptor. The change is not a trivial one, yes, so I actually did go through the trouble to verify that he is right, and went to accept the edit. But the edit had been rejected already with 3 votes stating that "This edit changes too much in the original post; the original meaning or intent of the post would be lost."

The user, having just 1 reputation on SO, then had no other choice to report his findings as to post in a new answer with just one line of code changed from the original. Someone else could have been discouraged by the rejection outright, and this could have been unnoticed already; certainly not a way to commend a new user for a contribution.

What is the rationale of rejecting this code edit? To me it was an exemplary one, and certainly not changing the intent of the post, unless the intent was to leak file descriptors. Furthermore it is not trivial knowledge that PEM_read_RSAPrivateKey would not close the file.

Furthermore I hardly ever can even go through my 20 daily edit quota as the edit queue is almost always flushed, due to hordes of trigger-happy badge hunters. I have always skipped edits to code where I am not a domain expert, but this does not seem to be the case for the most reviewers. Even on Meta, many state that they outright reject any edits that touch any code, good or bad, if it is not for an obvious typo or formatting issue.

The one you link above was accepted by the OP, both reviewers who saw it rejected it.
– OGHazaMay 13 '14 at 15:17

Ah and it seems it was accepted edited, removing the bug in the original.
– Antti HaapalaMay 13 '14 at 15:25

13

The only rationale is, "does this improve the answer"? The problem is, reviewers aren't expected to be experts in the tags they're reviewing, so they might not know what the heck fclose is or why it's necessary, and they might therefore vote to reject the edit. The review process tends to encourage this, because there are so many bad edits. All things considered, I think more reviewers clicking "Reject" is best. Unfortunately, that does mean sometimes good edits fall through the cracks. The only solution is to get enough rep to make those edits without requiring approval.
– Cody Gray♦May 13 '14 at 18:04

I don't personally think that a radical change would be the best reject reason. It would possibly be more appropriate to use invalid edit since a comment would be more appropriate, which may help to explain better why it was rejected. This is because edits should not change the actual code of a question.
– AnonymousMay 14 '14 at 22:33

2

@Anonymous We get into this misunderstanding a lot on Meta when examples are brought up. In this case, both of the examples I see are of edits to answers, not questions. Editing code in answers for correctness is fine, even a good idea. You only avoid editing code in questions because you might obscure the very issue being asked. Code in questions is expected to be wrong.
– Cody Gray♦May 14 '14 at 23:44

@CodyGray The edit may improve the answer, but it just seems better to leave a comment in order to assure that the author of the answer actually agrees. You have a valid point though.
– AnonymousMay 14 '14 at 23:54

2

@Anonymous Why? Should you leave a comment every time you want to fix a grammatical mistake? A spelling mistake? Where does it stop? Why do you need agreement? This site is collaboratively edited for good reason. If you can improve a post, you should do so.
– Cody Gray♦May 14 '14 at 23:55

7

"I have always skipped edits to code where I am not a domain expert" warrants a full blown discussion in my opinion. I find that I actually tend to skip most things in any queue because I don't know what they're talking about. I feel like StackOverflow would benefit from some kind filtering process in the queues so that people are more likely to deal with the things they understand thoroughly. The edit queue never builds up, but the decisions made there are extremely questionable, in my opinion.
– jpmc26May 15 '14 at 0:38

This is a moderately common scenario. I really think it comes from some reviewers rejecting all code edits (as suggested by a couple of answers below). I think that's the wrong approach, and I find it very frustrating. You get exposed to so many bad edits while going through the review queue, many of them getting approved. Then you finally come across one that actually fixes a legitimate, important problem, and it gets rejected.
– Reto KoradiAug 16 '14 at 6:33

4 Answers
4

Given just the portion of the code that you pasted into the question here, I, too, would have questioned the necessity of adding the fclose(). However, seeing the entire function in your link, the fclose() is definitely necessary as the function is absolutely leaking the handle without it. secretFile goes out of scope at the end of the function and it isn't being passed to anything else that could close it later. The edit should have been approved, IMO. The only reason I could potentially see for not approving the edit would be if you instead added fclose() further up in the function than the user who made the edit did.

How so? The function returns after that point, so what would the OP possibly be deliberately leaving the file open for? I don't see the "maybe not" scenario.
– Sam HanleyMay 13 '14 at 18:00

7

Experience has (eventually) taught me that any statement containing the phrase "I don't see" is a statement about my ability to see, and not a statement about the presence or absence of something to see. Be very careful when editing code that does not belong to you. If this were the real world, and you had inherited that code and the original developer was gone, would you have added that fclose? Without unit tests or regression tests or load tests?
– John SaundersMay 13 '14 at 18:03

19

@JohnSaunders If you look at the complete function, the handle is definitely not being used after that point. The snippet posted in the question here is not the entire code block from the question he was asking about. With the entire context, it's clear that it needed the fclose() and, without it, will leak a file handle. If it hadn't been for the entire function being provided, I would agree with your answer, but having seen the entire function, the edit was definitely correct. secretFile goes out of scope at the end of the function, so the handle is definitely leaked.
– reirabMay 13 '14 at 18:07

3

Clearly the reason that I said "I don't see the" and not "there isn't a" is that I'm leaving it open to debate -- but for the sake of argument, yes, if a file that should have been closed was left open at the point where it went out of scope, I would close it. I'd love an example of a reason why this would be bad, beyond just the suggestion that it could be.
– Sam HanleyMay 13 '14 at 18:07

8

Don't edit code unless you're more certain than that. Instead, ask the OP. Most likely you're right and the OP will say, "Thanks", but he may also say "Thanks for telling me - I forgot to add the call that makes it so I need that handle to stay open"
– John SaundersMay 13 '14 at 18:10

12

In this case the person suggesting the edit was a newbie user who could not have asked OP anyway.
– Antti HaapalaMay 13 '14 at 19:50

9

@JohnSaunders This is an answer, not a question. There's no risk that you'll accidentally spoil the question. It's clear that the missing fclose is an accidental omission, since there would be no reason to keep the file open and no way to close it later. This edit should have been accepted.
– Gilles 'SO- stop being evil'May 14 '14 at 21:32

It doesn't strike me as particularly necessary. It's common knowledge that files opened by fopen need to be closed with fclose and it would be very unusual for a function that reads from a file handle to take ownership of that handle and close it for you. In addition, someone using code from an answer ought to be reading the docs for the functions involved to be sure they understand what's going on, not just blindly copying and pasting.

Actually, PEM_read_RSAPrivateKey eventually calls BIO_set_fp that may or may not assume the ownership of the file, depending on its arguments. In this case it is called with BIO_NOCLOSE flag.
– Antti HaapalaMay 13 '14 at 12:36

13

I disagree: If a post starts with "I finally found the solution. Here is a method that will ...." then I believe strongly that the code should work correctly as written. If he had said "Do something like..." then I can (begrudgingly) see room for allowing broken code to exist on our site. (related meta post here)
– Richard Le MesurierMay 13 '14 at 17:46

2

I see that the actual answer is a very large block of code, not just the three lines quoted here. The answer would definitely be better with the fclose call added, but nevertheless the community consensus is to use a comment to ask the original answerer to edit their own post.
– nobodyMay 13 '14 at 17:56

Wow, this gets asked a lot. None of you have linked the questions where I last remember discussing this. I feel strongly that improving code snippets in answers is a valid edit and should be encouraged. But I realize it's pragmatically difficult when you're dealing with the review queue. So I'm okay with just telling competent reviewers to get the damn 2k already.
– Cody Gray♦May 13 '14 at 18:23

All code edits should be rejected with no exceptions. Reject them as "invalid edit" or "too radical", either is fine.

If you find an error in code posted as a question, the error might be very answer that the OP is looking for. If you edit it to correct the error, the question doesn't make any sense, nor does any of the answers. So in that case, your "helpful" edit made the whole post nonsense.

In other cases, errors in code of a question contain useful information to the readers, such as "this guy has not even compiled or tested this code".

And in all cases, making changes to someone else's code is too radical a change. In the worst case, an edit might even introduce bugs in the code.

Furthermore, silently correcting things in someone's posts means they will not learn anything from their mistake, nor will they know that the error was there.

And finally, editing posts to change the coding style to your own personal preference is not okay either. Coding style is subjective and there is no obvious right or wrong, so such changes are always too radical a change.

The only kind of code changes that are fine are changes that fixes indention or severe formatting issues, without changing the meaning of the code or the coding style.

silently correcting things in someone's posts means they will not learn anything from their mistake - except they get a notification that their answer was edited which should prompt them to take a look why.
– AD7sixMay 14 '14 at 16:43

5

While your point of view is perfectly valid and is, imho, the way to go about question code edits, you may have skipped an important part of the question. This was about answer code edits, where I completely agree with reirab and the OP. This edit was more than acceptable, it was nessesary.
– Silver QuettierMay 15 '14 at 8:44

@SilverQuettier The above addresses answer code edits as well. The only remark that's exclusive to question code edits is the one about potentially fixing bugs that were causing the problem to begin with.
– LundinMay 15 '14 at 8:46

3

Fixing a bug in an answer is not too radical a change. It is barely radical enough to restore some value to the answer. If you can't verify whether I introduced a bug, you lack subject matter knowledge and should skip the edit.
– JustinOct 15 '14 at 20:00