Terasology Code Style Conventions

Kai Kratz

Witch Doctor
Contributor
Architecture
Logistics
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

Org Co-Founder & Project Lead
Contributor
Design
Logistics
SpecOps
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.:scootangel:

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

Member
Contributor
GUI
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

Active Member
Contributor
Logistics
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?
 

Cervator

Org Co-Founder & Project Lead
Contributor
Design
Logistics
SpecOps
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

Org Co-Founder & Project Lead
Contributor
Design
Logistics
SpecOps
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

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

Cervator

Org Co-Founder & Project Lead
Contributor
Design
Logistics
SpecOps
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?
 

x3ro

Member
Contributor
GUI
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

Badges badges badges badges mushroom mushroom!
Contributor
Art
World
SpecOps
For me 120 characters sounds good. So, I vote for anything between 80 and 120 ;)
 

mkalb

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

Cervator

Org Co-Founder & Project Lead
Contributor
Design
Logistics
SpecOps
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

Active Member
Contributor
Logistics
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

Org Co-Founder & Project Lead
Contributor
Design
Logistics
SpecOps
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):

Code:
    /**
    * {@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

Lead Software Architect
Contributor
Architecture
GUI
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

Org Co-Founder & Project Lead
Contributor
Design
Logistics
SpecOps
Ahhhhhh! Sneaky sneaky IntelliJ ...

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