Terasology Code Style Conventions

[Immortius]

In general Terasology aims to stick to the standard Java code style, as described in the Java Code Conventions document and evidenced in the standard Java library.

At the moment Terasology targets Java 6, so you should avoid the new features in Java 7. This will be reviewed in the future. Where code can be improved with the use of Java 7, add a // JAVA7 comment above it. Edit: We're on Java 7 now!

Additionally:

• Use // TODO comments to note where something needs to be done
• Don't use Hungarian notation to denote types
• Where there are underscores before variables this is left over from before these conventions were created, feel free to remove them
• Use interfaces over concrete classes for variables (e.g. Map instead of HashMap). Feel free to use guava to construct the actual instance (e.g. Maps.newHashMap())
• IntelliJ is our primary development IDE, so for things like mass organising imports it is preferred it is done in IntelliJ. By default IntelliJ uses import some.package.*; if you are importing 5 or more classes from that package, suggest Eclipse users set things up the same way.

 

[Cervator]

I wonder how much fun on this topic we can fit into the Checkstyle bit mkalb is hooking up via Gradle? He needs to get a forum account already

 

[mkalb]

Immortius, thank you for starting this thread.

I think we should discuss changes for checkstyle in this thread.

 

[Immortius]

I guess a basic first point of discussion:

Change to no-star imports for standard imports, while still allowing star-imports for static imports (on, say, 3 or more occurrences)?

 

[Cervator]

And beyond imports - what other areas do we want to cover? I'm figuring there's probably a bracket style in there somewhere, maybe something on instance variables, lots more beyond that I'm sure

Edit: Ran a manual test job in Jenkins with "gradle check" working properly now, first yellow-balled build, yay! We can has broken unit tests and unused imports, gasp

 

[mkalb]

Thank you for the Jenkins integration.

I think we should add "name" checks. But then we get a lot of warnings.

 

[Immortius]

Well, those warnings are fair enough if they are for the _ variables (which need to be fixed).

Would like to see some checks around the braces for if statements at least.

There are a few things that should be excluded from check style - particularly the protobuf classes as they are generated.

 

[mkalb]

Yes, I am aware of the protobuf code. The current checkstyle rules works with the generated code.

 

[Cervator]

I kept an eye out for a good example and with the GUI stuff from miniME89 we got a few "violations" in for the star import rule we're playing with

http://jenkins.movingblocks.net/job/TestCheckstyle/8/checkstyleResult

Any more thoughts on exactly how we should do the import rules? Just that IntelliJ default thing? One of the GUI files did look dramatic with a dozen or more explicit imports for one package (or a few, I forget)

 

[Immortius]

There are some conventions around the use of Component classes that would be nice to be able to check for, although I'm not sure how hard it is to do. Basically nothing should inherit any class that implements the Component interface, and preferably there should be no class variables referencing them.

I should do a bigger write up of the entity system and the conventions involved in using it. It is a bit different from OO code.

 

[mkalb]

I allowed the star import for static imports and added more checkstyle rules.

Please look at this pull request: https://github.com/MovingBlocks/Terasology/pull/338

What do you think about the rules?

 

[x3ro]

Left you a comment on GitHub

 

[Cervator]

More checkstyle feedback now that the rules are in the main develop job and fixes are happening

https://github.com/MovingBlocks/Terasology/pull/345/files#r1501952

I'm torn on examples like that. Very brief single liners in bulk. In theory we should spread them out to match the style and stay consistent. But there does seem to be a functional use when you can condense some things. Reference:

if (pointOnAABB.z == min.z && testZ) normals.add(new Vector3f(0, 0, -1));
if (pointOnAABB.z == max.z && testZ) normals.add(new Vector3f(0, 0, 1));
if (pointOnAABB.x == min.x && testX) normals.add(new Vector3f(-1, 0, 0));
if (pointOnAABB.x == max.x && testX) normals.add(new Vector3f(1, 0, 0));
if (pointOnAABB.y == min.y && testY) normals.add(new Vector3f(0, -1, 0));
if (pointOnAABB.y == max.y && testY) normals.add(new Vector3f(0, 1, 0));

 

[x3ro]

I think that in this case it adds "clearness" to the code keeping it in one line. Do we have a checkstyle rule that enforces writing it on three lines? If so, then I'd say comply with the rule to avoid checkstyle errors. If not, I think it is a case where one could allow a deviation from the style guide

 

[Immortius]

I would ask whether there is a better way to write the method in the first place?

 

[Cervator]

I believe it does not add violations if you add curly braces which is what Perdemot did to fix the warnings from there. The spots where it is just a return statement of something should probably be expanded, but in that case a hugged return statement on the same line might look funny.

 

[miniME89]

In my opinion all should be consistent, so every if-statement would be at least a 3-liner.

If you would allow exceptions in particular cases, it would always be a question of where to draw the line between the use of those two methods. One programmer will always see it different than another programmer. This would lead to inconsistence in the code styling in the future.

 

[Kai Kratz]

Looking at the class I see other problems regarding method naming and SRP. The way the if clauses are structured currently actually add readability in this special case.

Honestly I'm not to fond about checkstyle. Checkstyle assumes "cleanliness" of code can be expressed by enforcing names and layout. This is true to some narrow extend. However there are much more important aspects: good decomposition, SRP, concise and expressive naming.

So to me seeing the code is "well" structured i.e. checkstyle produces no warnings is worth nothing. I think this effort might be better put into something different, more worthwhile.

 

[Immortius]

Well, just because it can't solve all of an issue doesn't mean it doesn't have some use. But I agree we shouldn't be mindless in our application of it, nor overstate its role.

I think a few simple rules around stuff that can easily be applied through IDE auto-formatters aren't an issue. It obviously isn't sufficient to actually achieve clarity of design, but it at least prevents "edit wars" where different people keep switching the style of code, and the merging issues such things can cause. I wouldn't want to get bogged down spending the next X months updating the code to an overly prescriptive set of rules though.

Findbugs is probably more useful as it identifies actual problems.

 

[mkalb]

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.