Question: Are style issues legitimate critique in a code review if the code works?

[blu28]

Sometimes when reviewing code one finds issues of coding style. Some style choices are long-standing, such as those from the 1999 Java programming style guide. Some are a question of best practice. Some are purely religious in nature. Some are just alternate ways of attacking problems. Should such things be ignored, mentioned and discussed, or mandated?

Of course, to criticize on style suggests the existence of a style guide. Should the team develop our own unique style, adopt the WPI style, or adopt one of the popular standards, such as Sun or Google?

[blu28]

Not having a style guide of some kind leads to code that is difficult to understand and maintain. Further, we should be encouraging the habit of following guidelines in students new to programming. This is why I included add the the VSCode Java extension "Checkstyle for Java" in a software level 1 lesson. Since that extension comes with a choice of two style guides, (Sun and Google) I choose Sun, but Google may be just as good, it was just an arbitrary choice.

An advantage of following the WPI coding style is that it makes understanding the WPI lib source code easier. The WPI style is the same as Google style, with three exceptions, WPI does not require blank lines between import sections, WPI allows lines up to 80 characters in length and Google allows 100, and WPI allows the use of formatting on/off tags and Google does not. There are some other naming conventions that WPI uses as well, but they don't seem to be well documented.

Since there are so few differences, using Google or WPI doesn't seem to be an issue. Google is supported by most VScode extensions, including the previously mentioned Checkstyle for Java and the WPI recommended Spotless formatter (issue #8 ). The Sun style is supported by Checkstyle but not Spotless.

So, once we decide on a style, is following the style and conventions we wish to adopt something that should be brought up in a code review/PR and if so, to what level? If we adopt a guide obviously following it is fair game, especially if we use Checkstyle and Spotless (and Sonarlint, but the jury is still out on that). But what about coding conventions and style? Should mentors make suggestions about these to students? What about between mentors?

[blu28]

See https://nashua151.slack.com/archives/C02SXFTMV2S/p1644411418319279 for discussion.

[blu28]

Apparently yes, but it will be better if we decide to commit to a style guide (probably Google) before the season starts.