This document provides details about coding standards and the purpose of code reviews. It contains the following topics:
- Purpose and Principles
- StyroFoam Checklist
- Project Level
- Java Implementation Level
- Service Implementation Checklist
- Misc DAO, JPA, WS
Purpose and Principles
The purpose of code review in Kuali Student (KS) is to enable agile, iterative development of KS as defined in the project charter. Given the distributed and ambitious nature of KS, we need to ensure that we follow a common coding standard, so that all the code in the system looks as if it was written by a single, competent individual. Standards ensure that the code look familiar and supports collective ownership. The specifics of the standard are not important as far as communication and collective ownership, but they are important to the robustness and correctness of KS. For example, if an object instance is asserted into drools working memory, then the following must happen in a standard way:
- Property accessors must have no side effects.
- equals( ) and hashCode( ) must be correctly implemented and override superclass.
To ensure all KS Developers have a common understanding and background, we suggest you read or refer to the following publications:
- Code Complete
- Effective Java
- Refactoring: Improving the Design of Existing Code
- Refactoring to Patterns
You should also review the following drafts:
Coding standards and review can be assisted by the following tools:
code format/assist tools
In general, we want to adhere to the Sun Coding Standards. Avoid all Sonar blockers and criticals: http://sonar.kuali.org/rules_configuration/index/4?searchtext=&plugins%5B%5D=&priorities%5B%5D=BLOCKER&priorities%5B%5D=CRITICAL&rule_status=ACTIVE&commit=Search
The following is an initial checklist for project and Java implementation levels.
- All generated artifacts such as Derby databases, JPA generated code, logs, and the like are in target. They are never committed to the repository.
All developers must:
- Install code templates (Copyright information), formatter, cleanup
- Run code formatter
- Run code clean-up
- Install analysis tool plugins for your IDE
- Run code analyzers (checkstyle, lint4j, findbugs)
- No errors should occur when building the source code. No warnings should be introduced by changes made to the code. Also, any warnings during the build should be within acceptable, reasonable boundaries.
Commit comments provide a summary of a change for development leads to review as they come in. Development leads will monitor changes and if necessary review the associated JIRA for rationale and further description of the change. All commit comments must include the following:
- A JIRA issue number. If there isn't a JIRA describing the task related to the commit, you should create one or have the requester create one for you.
- A description of the change. This description must be concise (less than 100 characters). The JIRA issue should contain all of the details for larger commits. Unless the task is specific to the change being made, using the title or description of the JIRA issue is not acceptable. The comment should include a summary of the changes. Rationale should be maintained in the JIRA, not in the comment.
Java Implementation Level
The following guidelines apply to Java implementation level:
- Adherence to Sun Coding Standards
- Code Smells
- Effective Java
General Java Practices and Guidelines
- Unit tests should not be order dependent. In any given test class, unit tests that create, update, or delete data must revert those changes. @Transactional can be used on a method by method basis with an automated rollback, but classes must not be annotated with @Transactional.
- Tests Should only be ignored (@Ignore annotation on class or method) if there is good reason to do so. Acceptable reasons include:
- Tested class is in the process of being removed or replaced
- Long, ongoing work causing tests to fail
- Tests written before work being done (TDD)
- All skipped test MUST be commented with a JIRA number. That JIRA should include rationale and plans or steps to remove the @Ignore.
- As a general rule of thumb, if you have a test method called
testSomething()which tests the happy path, you should have another method called
testSomethingWithInvalidInput()which tests the failure outcome scenarios when used with bad input.
Best Practices for Try-Catch in Unit Tests
Following are some guidelines and best practices for use of
try-catch in unit tests.
Minimize use of
try-catchunless you want to test for an exception being thrown. Test methods do not have any kind of contract, so it is okay to have test methods with a
In a unit test, code constructs of the following type are not very valuable:
Simply remove the try-catch and leave the code as follows:
Exceptions, when they happen, will bubble up automatically and cause failures. The code will be more readable and the next maintainer will thank you.
There are several ways to properly look for an exception in your test code. One of the ways is as follows (it is declarative and makes for cleaner code):
fail()statement makes sure that if at any point your code stops throwing the expected exception, the unit test will fail. Do not miss the
fail()statement. The happy path will go through the catch-block immediately if the expected exception is thrown. The first assertion checks for a non-null message. The second assertion makes sure an expected message is contained in the exception.
Do not leave the
Do not use booleans for checking existence of exceptions. The following code is convoluted and not easy to read:
A much more readable version is as follows:
If your expected exception provides deeper info about the error condition, write assertions against that, as in the following example:
- JUnit 4.7+ has three approaches for testing expected exceptions:
- Use the
- Specify the exception in the
- Use a
- Use the
We have been focusing only on the more traditional try-catch block approach. Testing Expected Exceptions with JUnit Rules has more details about all three approaches.
- Checked exceptions should be used sparingly and only in the case where the caller should be explicitly made aware of the exception condition and can recover from that exception.
- If checked exception is thrown, then it should be an instance of a specific sub class of Exception and not higher level generic exceptions (like Exception class).
- Never swallow exceptions if you are not going to do anything with them. Chain exceptions if you want to add more information to an exception.
Refer to the following document for more information on proper exception handling: Analysis of exception models.
One Element List Handling :: get(0)
- Using get(0) in a non-test class will result in a critical sonar issue being reported, which will result in a JIRA issue.
- Correcting get(0) depends on the cardinality constraints on the list.
- If a list constraint is changed, get(0) might not be the right answer anymore and we would prefer to expose that problem at the source rather than requiring implementers to track down the cardinality constraint enforcement in Java. This is handled thrugh the OperationFailedException being thrown in the examples below
- Cardinality of one and only one element.
Use KSCollectionUtils.getRequiredZeroElement, which does this:
- Go to the sonar issue and mark as resolved.
- Cardinality of zero or one:
Use KSCollectionUtils.getOptionalZeroElement which does this:
- go to the sonar issue and mark as resolved
- Cardinalities of more 0 or more, more than 0, and more than 1 requires removing the get(0) and replacing it with appropriate list handling.
JavaDoc and Comments
Observe the following guidelines for JavaDoc and comments:
- Describe each routine, method, and class in one or two sentences at the top of its definition. If you can't describe it in a short sentence or two, you may need to reassess its purpose. It might be a sign that the design needs to be improved and routines may need to be split into smaller more reusable units. Make it clear which parameters are used for input and output.
- All classes that are designed to be used in spring / dependency injection should identify all the properties and what they do. A spring XML example is always helpful.
- Complex areas, algorithms, and code optimizations should be sufficiently commented so other developers can understand the code and walk through it.
- There should be an explanation for any code that is commented out. Dead Code should be removed. If it is a temporary hack, it should be identified as such.
- For Pending, TODO, or FIXME, a JIRA Number is required for all code not completely implemented and tagged. Ideally a comment will also be present that describes what's left to do or is missing.
- Are constant parameters literally inserted into the code?
- Does a method definition comment document which of its parameters the method is going to change?
- Are the comments necessary?
- Are the comments accurate?
- Are variable names spelled correctly and consistently?
- Where it's not obvious, are variables documented with units of measure, bounds, and legal values?
- Do you understand the code?
Service Implementation Checklist
You should have done the following:
- Created a test class for Impl and have a test case for every method.
- Tested positive and negative conditions.
- Created a spring context for the service.
- Injected mock for all dependencies of the class that is being tested.
- If Class I:**, used DataLoader (creating Entities and putting them in static HashMap) instead of SQL insert to load data. Put your JPA entity in test persistence.xml.
- If Class II:**, tested the service with actual decorators. Mocked PermissionService, Dictionary Service used by decorators using mock impls of both.
You should have done or verified the following:
- Used validation decorator for separating out validation logic.
- Not handled and rethrown exceptions.
- Method body doesn’t exceeds 40 lines. If it does, try separating to Helper Class method.
- Annotated @transaction at the method level only.
- Every field in object is copied to and from the Entity. Except on updates, you should not copy over readOnly fields (like the type and the id).
- The meta data (create/update, user id, and dates) is updated from the parameter context.
Class II / Composed service Impl?
For Class II Service implementation, verify you've done the following:
- Created Validation and Authorization Decorators. Validation Decorators can now be generated!
- Separated out all validation logic from impl to the validation decorator.
- Avoided copy/paste code in validation and authorization decorator. Put repetitive logic in common method.
- Have *Transformers instead of *Assemblers for mapping Class II / Composed to other Class II and Class I DTOs.
- Implemented business logic methods using business logic helper classes.
- Implemented generate and complex conversions in assembly methods using Assembly Helper classes / Transformers.
- All thrown exceptions are checked for each method (missing/invalid params/version mismatch/cyclic dependency..etc).
- Check there is no direct DAO (data layer) call, unless there is a documented agreement to break that rule for performance reasons.
Class I/ Core Service Impl and JPA
For Class I Core Service implementation and JPA, verify you've done the following:
- Created JPA Entities from ERD provided by DBAs.
- Created DAOs per Entity for all needed getters.
- Used Named Queries in JPA Entity.
- Used proper (@Table, @OneToMany etc.) JPA annotations in JPA Entity.
- Have toDTO() and fromDTO() methods and a constructor in the JPA Entity.
- Query methods that expect a single result should enforce by calling javax.persistence.Query.getSingleResult().
- Transaction annotations at the highest level (not in DAO).
- Entities should never initialize collections. This can cause ORM problems where entities are no longer in sync.
- Are there test cases for code changes?
- Log4J should be used to log. Make sure the class used to initialize the logger is the correct one.
Misc DAO, JPA, WS
- DAO Updates. All EntityManager.merge(entity) calls return a different object than the entity you pass in. If your update returns the entity, it should return the result of the merge operation return em.merge(entity); instead of em.merge(entity);return entity;
- JPA. Always delete your database before changing any JPA entities. The drop-create can get out of sync with your changes and the DB will not reflect your entities correctly, especially with relationships.
- WS. Observe the following:
- From cxf generate java2wsdl. The List return types do not handle generics, so the generated wrapper objects need to be manually edited to include the generics.
- When returning objects that have Collection members, they may need to be initialized to an empty collection.
- For the servlet configuration using @Transactional on the services, the RI needs impl="ks.test.serviceImplClass" and cxf needs implementorClass="ks.test.serviceImplClass"