Coding Standards
Introduction
Releases typically happen every second Sunday Canberra, Australia (UTC+11) time. There may be code freezes on release days, but these will be announced in advance on the developers and experimental lists. There are 2 taboos:
- Checking in code that breaks the build.
- Checking in code that obviously wasn't tested.
We have a couple of code monkeys who make sweeps through the code to make sure it's all in the proper format, and we have others who go through looking to optimize code. Everyone has their role and things have gone very smoothly. If you are not certain about something, ask! Nobody is here to bite your head off (unless they are role-playing a troll). Try the developer's mailing list or any of the Yahoo! Groups or the Silverbacks.
Code Style
Coding styles can be assisted by your IDE, see the standards folder in the SVN repository.
- For Eclipse you can use 3.2 Formatter Rules
Voting for the code conventions is currently restricted to active code monkeys (have checked in in the last 6 months), the current votes (and therefore standard) is below.
| Rule | For | Against |
|---|---|---|
| Use Tabs | 4 | 4 |
| Use Spaces | 4 | 4 |
| Have braces around 1 line control statements | 8 | 0 |
| Open and close braces on their own line | 5 | 3 |
| 1 space padding inside parenthesis when calling a method | 0 | 8 |
| 1 space padding inside parenthesis when declaring a method | 0 | 8 |
| Space after a comma | 8 | 0 |
Who Has Voted (simply link in your name when you have done so)
- Martijn Verburg
- Aaron Divinsky
- Andrew Wilson
- Brian (telechus)
- Devon Jones
- Greg Bingleman
- James Dempsey
- Jonas Karlson
- Stefan Radermacher
- Tir Gwaith
- Tod Milam
Code Warnings and Errors
Code Warnings and Errors may vary by tool.
Please note that IDEA 9.01 can utilise Eclipse formatting rule exports, so we no longer need to cover that case
| Rule | Eclipse 3.5.1 | IDEA 9.01 | NetBeans | Description |
|---|---|---|---|---|
| Non-static Access to a static member | Y | ? | Y | - |
| Indirect access to a static member | Y | ? | Y | - |
| Unqualified access to an instance field | Y | ? | Probably Y | Referencing instance fields with this keyword |
| Undocumented Empty block | Y | ? | Y | - |
| Access to a non-accessible member of an enclosing type | Y | ? | Probably Y | - |
| Method with a constructor name | Y | ? | Y | - |
| Parameter Assignment | Y | ? | Y | - |
| Non-externalized Strings (missing/unused $NON-NLS$tag) | Y | ? | N | - |
| Serializable class without serialVersionUID | Y | ? | Y | - |
| Assignment has no effect (x = x;) | Y | ? | Y | - |
| Possible accidental boolean assignment if (a = b) | Y | ? | N | - |
| finally does not complete normally | Y | ? | ? | - |
| Empty Statement | Y | ? | Y | - |
| Using a char array in String concatenation | Y | ? | ? | - |
| Hidden catch block | Y | ? | ? | - |
| Inexact type match for vararg arguments | Y | ? | ? | - |
| Boxing and unboxing conversions | Y | ? | ? | Basically wants you to explicitly box and unbox basic types |
| Enum type constant not covered in switch statement | Y | ? | N | - |
| switch case falls through | Y | ? | N | - |
| Null pointer access | Y | ? | Y | - |
| Potential null pointer access | Y | ? | Y | - |
| Comparing identical values ('x' == 'x') | Y | ? | N | - |
| Missing synchronized modifier on inherited method | Y | ? | Y | - |
| Class overrides equals() but not hashCode() | Y | ? | Y | - |
| Dead code (e.g. 'if (false)') | Y | ? | Y* | Netbeans is covered by 'Unreachable Statement' option |
| Field declaration hides another field or variable | Y | ? | Y | - |
| Local declaration hides another field or variable (General) | Y | ? | Y | - |
| Local declaration hides another field or variable (Constructors and Gettors/Setters) | Y | ? | Y | - |
| Type parameter hides another type | Y | ? | Probably Y | - |
| Method does not override package visible method | Y | ? | Probably Y | - |
| Interface method conflicts with protected method in Object | Y | ? | N | - |
| Deprecated API | Y | ? | Y | - |
| Deprecated API (signal use of deprecated api inside deprecated code) | Y | ? | Y | - |
| Deprecated API (signal overriding or implementing method) | Y | ? | Y | - |
| Forbidden Reference (access rules) | Y | ? | Y | - |
| Discouraged Reference (access rules) | Y | ? | Y | - |
| Local variable is never read | Y | ? | Y | - |
| Parameter is never read (non overriding methods) | Y | ? | Y | - |
| Parameter is never read (overriding methods) | Y | ? | Y | - |
| Parameter is never read (even if documented with @param tag) | Y | ? | Y | - |
| Unused import | Y | ? | Y | - |
| Unused local or private member | Y | ? | Y | Includes unused private methods |
| Unused method | Y* | ? | Y | Eclipse is covered by 'Unused local or private member' option |
| Unreachable statement | Y* | ? | Y | Eclipse is covered by 'Dead code' option |
| .equals on Array | N | ? | Y | - |
| .equals on Incompatible Types | N | ? | Y | - |
| Comparing Strings using == or != | N | ? | Y | - |
| Synchronization on non-final field | N | ? | Y | - |
| Redundant null check | Y | ? | N | - |
| Unnecessary else | Y | ? | N | - |
| Unnecessary cast or instanceof | Y | ? | Y | - |
| Unnecessary declaration of thrown checked exception (non overriding methods) | Y | ? | Y | - |
| Unnecessary declaration of thrown checked exception (overriding methods) | Y | ? | Y | - |
| Unnecessary declaration of thrown checked exception (ignore exceptions declared with @throws or @exception) | Y | ? | ? | - |
| Unnecessary declaration of thrown checked exception (ignore Exception and Throwable) | Y | ? | ? | - |
| Unused break/continue label | Y | ? | N | - |
| Redundant super interface | Y | ? | ? | - |
| Unchecked generic type operation (JDK1.5) | Y | ? | Y | - |
| Usage of a raw type (JDK1.5) | Y | ? | Y | i.e. Using List instead of List<String> |
| Generic type parameter declared with a final type bound (JDK1.5) | Y | ? | ? | - |
| Missing Override annotation | Y | ? | Y | - |
| Missing Deprecated annotation | Y | ? | Y | - |
| Annotation is used as a super interface | Y | ? | Y | - |
| Unhandled token in SupressWarnings token | Y | ? | N | - |
| Unused SupressWarnings token | Y | ? | N | - |
| Enable SupressWarnings annotations | Y | ? | Y | - |
Standard 2004-Present
Coding warnings and errors can be highlighted for correction by your IDE, see the standards folder in the SVN repository.
Voting for the code warnings is currently restricted to active code monkeys (have checked in in the last 6 months), the current votes (and therefore standard) is below.
| Rule | For | Against | Description |
|---|---|---|---|
| Non-static Access to a static member | 3 | 0 | - |
| Indirect access to a static member | 3 | 0 | - |
| Unqualified access to an instance field | 0 | 3 | Referencing instance fields with this keyword |
| Undocumented Empty block | 2 | 1 | - |
| Access to a non-accessible member of an enclosing type | 2 | 1 | - |
| Method with a constructor name | 3 | 0 | - |
| Parameter Assignment | 3 | 0 | - |
| Non-externalized Strings (missing/unused $NON-NLS$tag) | 2 | 1 | Basically Internationalization will need this |
| Serializable class without serialVersionUID | 0 | 3 | - |
| Assignment has no effect (x = x;) | 2 | 1 | - |
| Possible accidental boolean assignment if (a = b) | 3 | 0 | - |
| finally does not complete normally | 3 | 0 | - |
| Empty Statement | 3 | 0 | - |
| Using a char array in String concatenation | 3 | 0 | - |
| Hidden catch block | 2 | 1 | - |
| Boxing and unboxing conversions | 0 | 3 | Basically wants you to explicitly box and unbox basic types |
| Inexact type match for vararg arguments | 3 | 0 | - |
| Enum type constant not covered in switch statement | 3 | 0 | - |
| switch case falls through | 1 | 2 | - |
| Null reference | 3 | 0 | - |
| Field declaration hides another field or variable | 3 | 0 | - |
| Local declaration hides another field or variable (General) | 3 | 0 | - |
| Local declaration hides another field or variable (Constructors and Gettors/Setters) | 1 | 2 | - |
| Type parameter hides another type | 3 | 0 | - |
| Method overridden but not package visible | 3 | 0 | - |
| Interface method conflicts with protected method in Object | 3 | 0 | - |
| Deprecated API | 3 | 0 | - |
| Forbidden Reference (access rules) | 3 | 0 | - |
| Discouraged Reference (access rules) | 3 | 0 | - |
| Local variable is never read | 3 | 0 | - |
| Parameter is never read (non overriding methods) | 3 | 0 | - |
| Parameter is never read (overriding methods) | 0 | 3 | - |
| Unused import | 3 | 0 | - |
| Unused local or private member | 3 | 0 | - |
| Unnecessary cast or instanceof | 3 | 0 | - |
| Unnecessary else | 2 | 1 | - |
| Unnecessary declaration of thrown checked exception (non overriding methods) | 3 | 0 | - |
| Unnecessary declaration of thrown checked exception (overriding methods) | 2 | 1 | - |
| Unused break/continue label | 3 | 0 | - |
| Unchecked generic type operation (JDK1.5) | 3 | 0 | - |
| Usage of a raw type (JDK1.5) | 3 | 0 | i.e. Using List instead of List<String> |
| Generic type parameter declared with a final type bound (JDK1.5) | 0 | 3 | - |
| Missing Override annotation | 2 | 1 | - |
| Missing Deprecated annotation | 2 | 1 | - |
| Annotation is used as a super interface | 2 | 1 | - |
| Unhandled token in SupressWarnings token | 3 | 0 | - |
Who Has Voted (simply link in your name when you have done so)
- Martijn Verburg
- Aaron Divinsky
- Andrew Wilson
- Brian (telechus)
- Devon Jones
- Greg Bingleman
- James Dempsey
- Jonas Karlson
- Stefan Radermacher
- Tir Gwaith
- Tod Milam
General Coding guidelines
- Arguments should have meaningful names, even if that makes them really long. variables that are temporary and throwaway you can name something inane - I like to name temporary strings aString for lack of any additional creative effort on my part :) (But for some of us, plain old s is just fine.) You will see aPC almost everywhere for PlayerCharacter instances, regardless of other conventions.
- Name arguments to methods something reasonable so IDE's that display the argument names can give a guide as to what is expected. If the method is something like setVision(String arg) it's pretty clear what's expected, but for methods which take multiple arguments it's especially important to name the arguments well.
- Always keep optimization in mind. Avoid creating unnecessary variables. But don't go out of your of your way to optimize if it clouds the meaning of the code or makes it harder to maintain. When in doubt, correctness trumps performance.
- Keep scope in mind — if a variable or method can be made private, protected or final, do so. If you're not sure what the difference is in these, don't be afraid to ask. This gets into the minutest of Java and we're glad to educate you so we can all make the best code possible.
- Write unit tests for new code, see Unit Testing.
- Add comments and Javadoc.
- If you change the syntax of a lst tag or output sheet token or modify the GUI or anything else that needs to be reflected in the Documentation, post a note here with Doc Monkeys! in the title so it gets their attention. If you can type up your notes so it fits with the format used in the actual documentation, the Doc Monkeys will love you. If you're not able to do that, give them at least something to work with and they'll make the rest up. :)
- Be friendly and courteous. If someone checks in code that annoys you — e-mail them. If you can't reach them or get no response, e-mail me. Slamming here shouldn't be necessary.
- Don't break the build. I've broken the build myself, so I don't hold it against anyone if they break it (though I don't expect anyone to make a habit of it!). Running ant complete pretty much saves you from this embarrassment.
- Make sure the JUnit tests pass before checking in your code. Running ant test is the answer here. (The "complete" target for ant includes "test", by the way.) Keep an eye on the autobuilds. If the software is presently failing for the auto-builds, it might be a good time to focus on build-fixing changes.
Remember we're all doing this for fun and because we want to produce something that can support character creation/maintenance for all our favorite books (and our own creations!).
If anyone wants to tackle a bug or feature but doesn't know where the code they should focus on is located, feel free to ask. I'm more than happy to point out where I think the related code would be located if that means its another bug or feature I don't have to worry about! PCGen has a fairly large code base so new developers will need some time to get used to how everything is organized. I don't mind helping to speed that process along, and I'm sure others would be willing to answer questions if they're posted here as well.
