4

I know reviewing code edits can really be a grey area, so just chewing the fat here/and learning, and if I'm wrong no probs at all.

I just wondered what others think would have been the right course of action on this specific one.

(The edit has been approved, just some weird rejections in there)

The question:
https://stackoverflow.com/questions/26432438/php-foreach-loop-only-echo-if-more-than-one-item

The edit:
https://stackoverflow.com/review/suggested-edits/6023143

Rejection reason:

Do not modify question code as it can change the nature of the question.

I did not change the nature of the question.
Surely in this case a quick looksie of the question, or even just the side-by-side comparison shows, I just removed the individual PHP open/close tags as we couldn't see the wood for the trees!

The code was 97% PHP, so individual open/close tags were redundant in terms of the question scope.
With it cleaned up, we could potentially help.

If it was "Why does this return an error" or "Why does this code not work", I'd not have edited the code. Would suggest in comment they tidy it up before we could help.

Other rejection reason

It would remove one of the fundamental items that the OP needs to be taught - proper syntax, non-trivial.

We're here to answer questions.
I don't agree that we're here to teach syntax, or anything unless it's within the specifics of the question being asked.
In fact, ironically, a teaching syntax question is likely to be off topic.

Besides, surely how they code their application is up to them!
If they want to tidy it up, they can ask a question "how would I tidy up this code" (on another forum or Stack site, of course..)

To reject my edit based on "make them do it so they learn" surely means we're to make every user edit their own question so they "learn"?

Maybe I've missed something...

0

3 Answers 3

7

Yes, the rejections are valid. Your edit was invalid - you broke the link-generation (by introducing multiple syntax errors):

Old:

<a href="<?php the_permalink(); ?> ? attribute_pa_style=<?php echo $style; ?>">
<?php echo wp_get_attachment_image( $thephoto ); ?> </a>

New:

echo '<a href="' . the_permalink() . ? attribute_pa_style . '=' . $style . '">';
echo wp_get_attachment_image( $thephoto ); </a>

Note the ? and attribute_pa_style which were previously inside the href are now (incorrectly) in the PHP, and the closing a tag is in the PHP instead of in the HTML output.

This is a perfect example of why edits like this are a bad idea. It should not have been approved.

1
  • 3
    As much as it pains me (being a bloke I hate being wrong..) this is correct. My original argument, and input from AstroCB, both have good arguments, but all that is now moot with the error being brought to light.
    – James
    Oct 18, 2014 at 1:44
1

As said, editing code (mainly on a question) is a grey area, with many questions on MSO and MSE with answers contradicting each other, all with plenty of upvotes.

@Andrew Medico
I get what you are saying, and my bad for the poor quick bodge-up on the additional echo. Schoolboy error!

The code was a mess, and the question was poor. By the time I got there, it had two downvotes, and it was a border line flag-able question in the first place - "How do I make this code echo only if X > 1".

So I just quickly edited the code so we could actually read it, with the intention someone could then provide a quick answer to what was a simple question, and we could all move on.

Of course, this idea ended up to be my downfall, and in hindsight I shouldn't have progressed down this avenue.
If you're going to do something, do it right, or don't bother.

I suppose the real issue was it added an error to the code, which is a no no. OP could have copy/pasted their code back into their scripts, or people could have started to comment/answer about the error, even "before you fix your loop, fix your code, it has a fatal error" etc.

Either way it introduced an error, which is enough to reject on.

I'll be more careful in future, and wont be changing code. As much as it might help the OP, really they need to sort out their question code when it is actual code change rather than just code re-alignment.

Thanks for the feedback everyone.

-4

You're right.

As for the first rejection reason, to someone unfamiliar with PHP or web development, what you did looks like vandalism of the code (i.e. you're changing it beyond repair, which is changing the OP's intentions in posting it). A lot of times, things that people like to "fix" and label as formatting are actually the cause of the problem in the first place, so in most cases, it's best to stay away from code unless it's not formatted as code at all.

For the second, the tags you've removed could have been mentioned as redundant in an answer; when you edit a question, the asker may not always look to see what changed if it appears obvious without checking the revision history or suggested edit. By removing them from the question, you're potentially changing one aspect of an answer (although not necessarily an important one) and thus changing the overarching meaning of the question.

Edits are important, but it's important to learn what they're used for as well as when one goes too far. That's part of the reason we have a suggested edits queue. Asking questions here is one way to figure these things out, and by the time you've reached 2k reputation (and received full editing privileges), you should have a fairly good idea of their utility.

6
  • 1
    Fair points. Cheers. Once I get to edit level, I'll just see what others do for some time before reviewing edits myself. Same as I did on the old MSO.
    – James
    Oct 17, 2014 at 21:26
  • 1
    The syntax and/or code conventions of the post were changed, this is a no-no. Regardless of if it changes the behavior, it COULD change the behavior. Citation. Oct 17, 2014 at 21:27
  • 2
    Sorry if that sounds harsh, it wasn't meant to be, just exact (I'll respond rather than rewrite). It is one of those areas where the potential problems outweigh any benefits. Please continue to contribute edits and grow as a contributor. There have been plenty of mistakes that I have made and learned from for sure. :) Oct 17, 2014 at 21:33
  • 1
    @JasonMArcher No sour grapes my end, ever ;) In a generic sense, there could be problems with code edit, however this particular scenario I see no such potential issues. The syntax and code conventions were changed, as that was what needed changing (editing) in order to make the question comprehensible (edit to improve). And same friendliness back - I wasn't pulling you out into public, just I'm like a dog with a bone with things I disagree with or find illogical ;)
    – James
    Oct 17, 2014 at 21:43
  • 1
    I understand, I encourage you to ask. And though it wounds the pride, I should add that you speak up if you don't agree. Oct 17, 2014 at 21:46
  • 1
    This answer contains some good info, but it is missing the fact that the edit did break the code (exactly as one of the reject reasons mentions).
    – nobody
    Oct 17, 2014 at 22:31

You must log in to answer this question.

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