Is it always ok to suggest best formatting practices on code that doesn't follow popular conventions?

Question

In an effort to make progress on the oldest JavaScript zombies, I've been offering as much of a review as I'm capable of, based on the content, or the quality of the code, or sometimes, the amount of time that I have available. Even if I can say more, I often leave that to another (more capable) reviewer and move onto another review: "Share the Rep. Team up on a review."

Well, when the code looks great, sometimes there's nearly nothing to suggest.

In this case, I thought the code was written very well, but it didn't seem to follow a few of the conventions that I read and observed in Douglas Crockford's Article on JavaScript formatting conventions. So I walked the user through a few of the changes suggested and demonstrated in Crockford's article.

On my answer, a user comments that I should not suggest formatting changes unless they damage readability or cause confusion.

As long as everyone involved in the maintenance of this code agrees that this is how they want it formatted, then there is no mistake. Nitpick about formatting when its inconsistent or significantly impacts readability, not because its in a style that differs from your own.

Is it always ok to suggest best formatting practices on code that doesn't follow popular conventions? Or should I not submit these formatting improvement suggestions?

Answer 1

Is it always ok to suggest best formatting practices on code that doesn't follow popular conventions?

YES

on Code Review, you can review any aspect of the code you want (Do I want feedback about any or all facets of the code?). If you feel the best formatting practices are not being followed, then feel free to comment on it, and suggest improvements.

---

I have read your answer, and, I assume your assertions about both the HTML (Your html is flawless. Good job.) and CSS (Also completely flawless.) are reasonably accurate, I would not know, I am not familiar with best practice there...

But, the rest of your answer (the bulk of it), reads quite well to me.

I often include reviews about common practice and style. I cant think of many times where I tell Java people to put the { at the end of the line, etc. I know the Python folk often talk about PEP-8 or whatever.

So, as your answer goes, it got a +1 from me. It reviews the code, it provides suggestions, and suggests the rest of the code is Great.

Now, if there is a local convention, or code standard that is different to general practices, and the OP does not tell you up front, then you are not expected to be able to guess this. If they tell you that these local rules exist, after you have reviewed, then add a comment saying: Your local rules deviate from common practices, and you should know, and it is valid on code review to include feedback on your style.

Obligatory note though, you should always be aware that code style is code style, and not code functionality. The difference is that people can sometimes be offended easily when you challenge their style. Do it tactfully, and sensitively. After your edits, your answer looks fine to me.

Answer 2

I agree with the complaint: please don't write a review that consists of formatting nitpicks, unless the formatting is atrocious and unreadable. Many programmer already have a misconception that code review is about unproductive arguments on tabs-vs.-spaces, indentation width, brace styles, etc., and I'd rather not have Code Review fall into that reputation. Sure, your points could be worth mentioning, but only if you have something substantive to say as well. In this case, there were many real problems with the code. (I've posted an answer; I'm sure that there's more that could be added by others.)

I think that no answer at all would have been preferable to one consisting solely of superficial opinions.

Answer 3

I think that some formatting nitpicks are okay, but be careful about what you're nitpicking about.

For some languages, 99.999% of the programmers will be developing using the same IDE. For some, a massive percentage will even all be on the exact same version of the exact same IDE. But in still other languages, there will be a massively wide array of IDEs.

Moreover, some formatting standards will vary widely through various work environments. One of the worst things we could do would be to encourage someone to pick a fight at their work over a particular formatting convention that isn't even consistently preferred by professionals of that language.

But in some cases, the nitpick may seem small but can have a huge impact.

For example, when writing code in Objective-C or Swift, I know that virtually everyone will be using Xcode. And in Xcode, the Egyptian-style curly braces are vastly preferred. For other programming languages, I might only comment on the brace style if it's inconsistent. But for these two languages, I will often times make a remark about the brace style. In Xcode, anything that you can wrap curly braces around can be collapsed, and when the opening brace is on it's own line, the collapsed look is displeasing, arguably confusing, and takes up way too much space.

I consider this a nitpick, because a lot of programmers may never even collapse any bits of their code, so this may never matter to the asker or anyone who will ever maintain any of the asker's code. But if you were going to collapse it, it does make a noticeable difference in the collapsed readability.

---

I must say though that I cannot agree more with the spirit of 200_success's answer. Answers posted that review formatting issues only should introduce themselves as answers that do just that and not mislead the asker into thinking that everything else in the code is fine to at least one set of eyes.

Although I think this can apply to all answers--not just answers that address formatting. We could get into the habit of announcing the scope of our answer as an introduction to the answer itself. Often times I'll decide I want to comment on a single method out of an entire class. When I do this, I should try to be clear that the scope of my answer only covers this method and that the lack of comments on the rest of his code should not be taken as an indication that I feel the rest of the code is fine.

Answer 4

I feel that it is OK and valid. Good formatting is one of the best ways to make your code make sense to Mr. Maintainer, along with comments and such. Some marketing people make a living formatting information in a way that's instinctual to the user. Why should programming be any different?

Sometimes even a very talented programmer can make his/her code cryptic by improper formatting, and I think all efforts towards sticking by a standard formatting are well worth it.

Answer 5

As a small addendum to what was already said in all answers:

Make clear to op that your advice does not make him a bad coder. As a Programmer, your code is your baby. Your heart is often in code and you are emotionally attached.

Usually I prefix my style-recommendations to already nicely styled answers with something along the lines of: "Personally I prefer to [...]"
And add a note at the end, along the lines "whether you incorporate the style-advice is yours to decide".

OTOH, especially when reviewing Java / C# / Python, where an official style-convention exists, and OP didn't add a notice in his question, I will be more blunt and refer to the official docs when suggesting stylistic changes.

Up to now this has worked well ;)

Answer 6

NO, because:

Sometimes there are things happening to code-style, when you program for a company. As you specifically asked about, if it's always okay to suggest style-changes, I will focus on the always you use.

Our help center defines: "Do I want feedback about any or all aspects of the code?" as one of the major points to ask a question, but sometimes there's things you aren't allowed to change.

Let's for example take the Apps Hungarian notation, that is used internally in the Microsoft Office Programs. This is a special naming-style that is used, in terms of corporate usage, solely in the Office APIs. (AFAIK that is)

Assuming someone from Microsoft came here, and asked a question about some code he wrote for Microsoft, then there's a few things he can't change about the code. That is because the development team has decided on a special convention (Apps Hungarian) to do things.

Of course we could now come and say "hungarian notation is bad, use [suggestion] instead, because [reasoning]". Important here is especially the part [reasoning]. We can only apply reasoning in the limits of information given to us. It is highly probable that the person posting his code here has more information on why a specific style was chosen.

It is not our job to criticise that decision, but instead to show inconsistencies in the used style in these cases.

Requirements

For that it is necessary, that OP mentions in his post, that he is tied to using this convention.

Whether that convention is now about the placement of braces, the use of tabs over spaces or the naming schema is irrelevant. That convention in the development team overrides industry convention!

A review focusing on reviewing development team conventions against industry conventions against better knowledge is not constructive and should be subject to downvoting, or even deletion.

If OP does not mention this in his question, then he's not in a position to criticise the answer, but if he did, then the answer is (at least in that aspect) meaningless.

But:

It's just some corner cases where this applies. These cases are seldom, and for everything else the answer is a definite YES!