Development Standards
This page presents a list of coding standards and practices that apply to PC2. While we know that there are likely portions of the PC2v9 system which currently might not meet all these standards fully, we're working toward this goal. Most certainly, any new Pull Requests considered by the PC² maintainers would be expected to comply with the following.
All development for PC² projects should be done on a Git Topic Branch (also sometimes called a Feature Branch). For PC2v9, topic branches should be named according to the following scheme:
- the name should start with the characters "i_" (the letter 'i', followed by a single underscore).
- the next characters in the branch name should be the PC2v9 "issue number" (bug number) under which the branch work is being done (all branch work should relate to some previously-filed issue in PC2V9 Issues).
- the issue number should be followed by a single underscore.
- the remaining characters of the branch name should give a short, concise summary of the issue topic.
For example, a new branch being created to work on an issue named "Submit Button throws Exception" and filed as "Issue #1208" might be named:
i_1208_Submit_Button_throws_Exception.
All source code to be deployed to production should be under src/ or resources/.
All Unit Test classes/programs should be under test/.
All Unit Test input test data should be stored under testdata/.
All Unit test output should be written under testout/.
- Pull requests must be targeted against a specific "issue branch" in the forked repository (not for example against the develop or master branch).
- Pull requests must be linked to a GitHub Issue, by issue number. See Linking a pull request to an issue for details.
- Pull requests must involve a single issue. Submitting a PR that mixes code for several different issues is a virtually certain way to get your PR rejected.
All code fixes and enhancements to the PC2 code base should be accompanied by appropriate JUnit tests, using JUnit 4 or greater.
PC2 JUnits should extend class edu.csus.ecs.pc2.core.util.AbstractTestCase, should reside in the appropriate subpackage under the test/ folder, and should conform to the following guidelines:
- Unit tests should not output to stdout or stderr.
- Unit tests should output only to files created under the testout/ directory.
- Unit tests should use method
fail()in try/catch clauses when an Exception occurs.
See the PC2v9 Wiki page Unit Testing for additional details on using JUnits for unit testing in PC2.
-
There should be no Java compilation errors in code. (We know this is "obvious"... but sometimes it's necessary to state the obvious.)
-
There should be no Java compiler warnings in code.
- Rationale: a compiler warning is typically a situation where a coder has not thoroughly considered and dealt with subtle issues. PC2 code is expected to be not only compiler-error-free but also compiler-warning-free.
-
Use logging to record the context where problems/exceptions occur.
-
Do not leave empty catch clauses. If there is not code to handle the exception, add a comment saying why there is no code.
- Rationale: the catch is there to handle exceptional conditions (that is one major part of its power). Each exception should be reviewed/analyzed and handled; otherwise, silent failures occur and root causes may not be easily apparent.
-
For adapters and other utility classes, do not create logged output; instead, throw an Exception with good context information and let the higher level code log/handle any problem.
- Rationale: higher level code should handle errors. Avoid logging errors in utility methods; either throw exceptions or return a value that clearly indicates what happened. This helps avoid unnecessary log dependencies.
-
Very very rarely output to System.out or System.err; use a Log instead.
- To log to the console, add a ConsoleHandler.
-
If no log is available be written to, output to System.err indicating that the log is not available and also output the intended log message.
- Rationale: for debugging and other purposes if problems are only output to stdout and stderr, that effectively is a silent failure.
-
Provide a
case:for every possible element value in switch statements. -
Provide a
default:element for all switch statements. -
Inspect all loops (while, for, do, etc.) to insure there is no missing exit condition.
-
All GUI code should be handled on the AWT thread.
- Lengthy GUI-manipulation code should be spun off using Swing worker threads or SwingUtilities.invokeLater().
-
Non-GUI code should not be handled on the AWT thread.
-
Take care not to misuse the
finallyblock intry-catchclauses.-
finallyblocks are unconditionally executed; their use should typically be restricted to "clean up" or logging only.
-
-
Insure that code does not reveal passwords in the log or console.
-
See the coding style rules in our Coding Style document for further details on coding guidelines for PC2.
PC2 code is expected to be properly documented using javadoc. In particular, the following minimal standards apply:
-
Every class should have javadoc header comments explaining the purpose of the class and how it is intended to used, along with any restrictions regarding usage.
-
Every method should have javadoc header comments explaining the function performed by the method, including any side effects of the method (which of course should be avoided, but if present they must be documented).
- Javadoc method header comments should use the @parameter keyword to specify the purpose/usage of each parameter.
- Javadoc method header comments should use the @return keyword to describe what is returned by the method (if not void).
-
Javadoc comments (both class and method) should use the {@link ...} keyword to link to all referenced classes -- but especially to other PC2 classes.
-
Code should contain comments (javadoc or otherwise) explaining the purpose of each major code section. In general it is expected that a reader can discern what a given piece of code is doing by just reading the comments.
The following are some additional reasons that a Pull Request might be rejected.
-
Doesn't meet the minimum functional requirements of the corresponding issue (i.e., doesn't correctly solve the problem).
-
Introduces a significant bug into the system.
-
Fails to handle potential conditions. For example, an "if" statement with no "else" block could be an issue. (No, not every "if" needs and "else" -- but failure to include an "else" where it is needed would be a reason for PR rejection. In some cases it might simply suffice to point out [in a comment] why there is no "else" block.)
-
Generates Error or Warning messages in Eclipse. (As mentioned elsewhere, most PC2 development is done in Eclipse; we use the Eclipse Warning- and Error-flagging tools as measures of problems in code. There is no requirement that developers use Eclipse -- but you should be aware that we test all code in an Eclipse environment and so you should make sure that your preferred IDE at least has similar settings such that no Errors or Warning messages are generated.)
