12
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ Be aware that the Crockford conventions often differ wildly from the node conventions. One person's good style can be another person's WTF. That said, the things you pointed out were fairly basic issues and I don't think any style guide would disagree with them. Also, declaring variables without var is not a style thing, it's just plain wrong. It won't work in strict mode or ES6 and there is absolutely never a reason to do it, outside of extreme edge cases like code that's going to get combined with some other code at build time where the variable is declared properly. \$\endgroup\$ – Dagg Jul 28 '14 at 15:55
  • \$\begingroup\$ Also, the HTML wasn't completely flawless. Assuming it is HTML and not XHTML (he calls it HTML so I think it's safe to assume that), those trailing slashes in the self-terminating tags don't belong there. Even if it is XHTML, there is a trailing slash with no preceding space in one of the image tags; it definitely does not belong there unless this is actually plain XML (and not XHTML, and certainly not HTML). There may be more problems but I didn't bother looking more closely. \$\endgroup\$ – Dagg Jul 28 '14 at 16:06
  • \$\begingroup\$ @Dagg My [Looks good so let's compliment the OP for it] practice was a bit exaggerated here. I agree there there were some small changes, now that I look back. I do still say the code looks pretty good! I'll repair my answer a bit. \$\endgroup\$ – Viziionary Jul 28 '14 at 16:20
12
\$\begingroup\$

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.

\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ People don't come here just to be warned that their code is atrocious and unreadable. They come to improve it beyond the beginner level, to learn to write better than average code. Formatting is one of the key aspects of that goal. \$\endgroup\$ – Viziionary Jul 27 '14 at 0:35
  • 1
    \$\begingroup\$ Many programmer already have a misconception that code review is about unproductive arguments on tabs-vs.-spaces, indentation width, brace styles, etc. - Unproductive? If a programmer (like myself) moves forward to submit code for a job without paying attention to each and every one of those issues, he'll be laughed at. Attention to detail is a vital aspect of programming, and I would personally avoid working with a programmer that thought conventional formatting practices were unproductive. \$\endgroup\$ – Viziionary Jul 27 '14 at 0:38
  • 2
    \$\begingroup\$ As I said in my comment, inconsistencies in style are worth pointing out (part of that attention to detail you mention). If the code in question consistently uses tabs for indentation throughout the entire project, suggesting switching to spaces is absolutely unproductive. \$\endgroup\$ – cimmanon Jul 27 '14 at 1:00
  • \$\begingroup\$ Nor would I suggest such a thing. I simply suggested very popular best formatting practices to a user wishing to improve his JavaScript programming. @cimmanon \$\endgroup\$ – Viziionary Jul 27 '14 at 2:12
  • \$\begingroup\$ There are situations where mentioning code formatting issues could be beneficial, but I feel that that answer was poor. My disapproval stems from a confluence of several considerations: 1) the formatting issues were all minor; 2) there was nothing else in the answer; 3) you stated that the HTML was fine. It's a matter of misplaced priorities and providing false confidence. \$\endgroup\$ – 200_success Jul 27 '14 at 2:27
  • 2
    \$\begingroup\$ I happen to disagree on a couple of points with @200_success ... but that is the nature of things, sometimes. Specifically, the first two paragraphs are real reviews (the HTML and CSS are flawless), whether they are accurate, or not. That should be a subject of upvotes or down votes, not related to the review of the style. \$\endgroup\$ – rolfl Jul 27 '14 at 2:30
  • 4
    \$\begingroup\$ @rolfl "HTML and CSS are flawless" are extraordinary claims, which should require extraordinary proof. I don't consider the first two paragraphs real reviews for that reason. \$\endgroup\$ – 200_success Jul 27 '14 at 6:19
  • \$\begingroup\$ @jt0dd You're missing the point here. This isn't just about you and what you would or wouldn't have done, this is about criticism of any formatting (including choice of indentation!). Mr. Crockford condemns the use of tabs for indentation (with no real justification behind such a suggestion, just like everyone else who condemns the use of tabs), if you're going to say follow this standard, why not go the whole hog? \$\endgroup\$ – cimmanon Jul 27 '14 at 11:07
  • \$\begingroup\$ @cimmanon this opinion seems to be a slight minority. \$\endgroup\$ – Viziionary Jul 27 '14 at 17:13
  • \$\begingroup\$ @200_success That was not rolfl's point at all - You 100% missed his point - His point was that if there were to be disagreement about the post, it should be about that, and not about the formatting side of the review. \$\endgroup\$ – Viziionary Jul 27 '14 at 17:15
  • 2
    \$\begingroup\$ @jt0dd You're missing my point, which is that the confluence of factors 1, 2, and 3 makes your answer an misleading assessment of the code. If you had omitted "HTML and CSS are flawless" and instead prefaced your remarks with "I'm focusing on your code formatting", then at least it would have been an honest answer. If you didn't actually make an effort to find problems in the HTML+CSS, declaring that it's flawless is irresponsible. \$\endgroup\$ – 200_success Jul 27 '14 at 17:36
  • \$\begingroup\$ @200_success You missing the entire topic of this question. It's not an argument. It's a question of content, and the popular consensus seems to be that it's ok to submit a review based on formatting conventions for the language in question. My answer's various weaknesses or mistakes are not part of this discussion; if you'd like to bring those issues up, please open a new meta discussion on the subject. \$\endgroup\$ – Viziionary Jul 27 '14 at 19:22
  • 2
    \$\begingroup\$ +1 because in principle this is probably usually right. Considering the actual post in question, though, I think style was an appropriate thing to go after, since it was egregiously bad by any standard. Also, hidden in that "style" section was a paragraph addressing improper variable declarations, and that definitely needed addressing. It was not a style issue. \$\endgroup\$ – Dagg Jul 28 '14 at 16:00
7
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I agree. People didn't bother to write formatting standards for their health. \$\endgroup\$ – RubberDuck Jul 27 '14 at 0:52
6
\$\begingroup\$

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 ;)

\$\endgroup\$
5
\$\begingroup\$

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!

\$\endgroup\$
  • 2
    \$\begingroup\$ Good answer and good point about OP disclaiming it. I tend to point out breaks with style best practices anyway. Always with my own disclaimer, "... but you were consistent and that's more important." \$\endgroup\$ – RubberDuck Jul 30 '14 at 11:40

You must log in to answer this question.

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