Terasology Code Style Conventions

[mkalb]

It is easy to remove the check "NeedBraces".
And we can define, that is should not check IF and ELSE.
http://checkstyle.sourceforge.net/config_blocks.html#NeedBraces

 

[Kai Kratz]

If we develop with 10 or 20 people (and different IDEs) we need some rules and styles (format, naming, coding, ...).
We can discuss about the rules and decide which one we want to use, but we need rules and styles.

Well code should look tidy, but i don't see a need for this sort of strict rules.

 

[Cervator]

I think Checkstyle being off about something is more a call to just tweak a rule than not using it

And even if zero violations != perfect code the presence of the stats get some people to do something about it, including thinking more closely about the exact thing we're discussing - which I think is kind of cool.

With very expressive rules you can support more people helping with style and have core people focus more on architectury stuff

(all IMHO, of course)

 

[x3ro]

Well code should look tidy, but i don't see a need for this sort of strict rules.

The problem is that code won't look tidy without strict rules, because "tidy" is completely subjective. What I am trying to say is that the rules define what "tidy" means in the first place, because otherwise every programmer will argue that _his_ way of writing code is the right ("tidy") one.

 

[mkalb]

Without rules we will have many edit wars and merge conflicts in the future. Both cost a lot of time.

We also need to define the line size/length. The checkstyle rule LeftCurly has a default from 80 and the Eclipse formatter too. But I think 80 is to short. What is the default from IDEA?

 

[Immortius]

IDEA's default is 120, which is probably still too short.

 

[Cervator]

Okay, compromise time, fixing checkstyle warnings just to fix them do cause a lot more potential conflicts than I had thought about at first, my fault there since I don't actually get into any substantial coding these days. We need to group pure fixes at quiet times and just fix any warnings as part of normal functional fixes if that area isn't being worked by anybody else.

On top of that as we learned pretty quick there are some tricky spots without a clear answer. We should be sorting out those issues and tweak rules as needed. And add more fun over time like findbugs and a full Sonar install

In the meantime I'm going to close the outstanding checkstyle pull requests - sorry about the lost effort Perdemot although we did learn some valuable stuff. We'll likely make it part of standard pull requests to run a check on the affected files if they're otherwise quiet and fix warnings then.

 

[Cervator]

Added a wiki page for Checkstyle - mkalb feel free to add anything extra if you like

Mainly wanted to capture the plugin instructions for IntelliJ from the last pull request

 

[mkalb]

Added a wiki page for Checkstyle - mkalb feel free to add anything extra if you like

I added a new section for eclipse and changed the gradle a little bit.

 

[mkalb]

What should we do with the line size? Should checkstyle and eclipse use the default (120) from IDEA?

 

[Cervator]

Thanks!

How many warnings would 120 trigger? It does seem like it might be a little narrow with the crazy monitors of today. How many do 130, 140, or 150?

 

[Immortius]

Make it 250. We don't need to be pedantic about this one.

 

[x3ro]

IMHO anything beyond 90 characters is not readable. I'm not saying that it can't be displayed, monitor resolution obviously allow for far more than 90 characters (except on my laptop), but I just hate reading long lines and then jumping back to short lines. Imagine a book where 20% of the lines are three times as wide as the other 80%. Would be horrible to read.

(Just my two cents, I'm not saying we should set it to 90 chars, I'm just saying that this isn't about what monitors can display but about whether or not it is readable )

 

[Skaldarnar]

For me 120 characters sounds good. So, I vote for anything between 80 and 120

 

[mkalb]

I think 80 is to short and I vote for 120 because it is the default from IDEA.

 

[Cervator]

FYI - some quick stats for how many warnings the different line length settings would actually provoke:

length = warnings
80 = 3,751
100 = 1,595
120 = 720
140 = 361
160 = 178

120 fits comfortably in my IntelliJ with both sidebars open (but narrow). Any shorter than that would be unrealistic to maintain IMHO - for comparison we currently have about 1,500 checkstyle warnings total. Even trying to fix 720 would be substantial, but maybe over the long-term going with the IntelliJ default would beat a slight bump to 140 just to halve the warnings?

On the other hand 120 might waste a little space, perhaps 140 is the better choice. Realistically lines will be several characters shorter, even 10-20, if there's a need to drop down to a second line.

One of those would be my vote.

 

[mkalb]

Some chechkstle warnings can be easily and fast corrected by the IDE.
Tab, Indentation, Line size, ...
And this should be easy to merge without many conflicts.

 

[Cervator]

Bump - I'm playing with Javadoc and Checkstyle some while actually doing a bit of code.

Noticing that IntelliJ is provoking different checkstyle warnings than we have in Jenkins, which means I'm mostly fixing warnings in files I touch that won't even show in Jenkins.

IntelliJ checkstyle config looks to be set to use the /config/checkstyle/sun_checks.xml file - which I guess is a different set of rules than what we're using for Jenkins (in /config/checkstyle/checkstyle.xml). Yet actually making a change in the sun file (changing from default line-length of 80 to match IntelliJ 120) didn't seem to change the warnings and wouldn't matter in Jenkins. Changing in the checkstyle.xml didn't seem to matter for IntelliJ either, but would for Jenkins. My understanding might be off - any ideas?

Also found a discussion on putting blocks like these on interface-derived method implementations where JavaDoc is written in the interface (or similar inheritance):

    /**
    * {@inheritDoc}
    */

Which I personally kind of like as it helps point out that there is JavaDoc written for the method. It also seems to make Checkstyle happy. However, technically that's the behavior anyway, so under some conditions it might get flagged as unnecessary (in particular Eclipse is listed to have had issues, but years ago, unsure of current state). Thoughts?

 

[Immortius]

You have to manually set IntelliJ to use config/checkstyle/checkstyle.xml.
By default IntelliJ uses an internal copy of the sun_checks.xml.

 

[Cervator]

Ahhhhhh! Sneaky sneaky IntelliJ ...

Do we even use the sun_checks.xml under /config then?