Child pages
  • Best Practices for Kernel code
Skip to end of metadata
Go to start of metadata

11-Dec-2008 This work mostly refers to Kernel 1 and is being updated and integrated into Best Practices for High Quality Code by Aaron Zeckoski and Alan Berg.

Information

This defines the best practices and standards for code being added to the Sakai kernel. This should include suggested best practices as well as required standards.

Minimum Requirements

  • Defines the minimum requirements that must be met by code going into the Sakai kernel. These are primarily to maintain high performance and ensure that the code is maintainable and flexible.

Paging performance focused interfaces

All kernel interfaces should provide methods to retrieve large record sets in pages. For example, one should be able to retrieve a set of all users like so (where inputs of 0 and 0 would retrieve all users in the system):

Methods which provide for limited retrieval should also include this "pagination" capacity. For example:

All caching must use centralized caching

Heavily used parts of the kernel (user lookups, authz permission checks, etc...) must be cached. The caching should be handled using a centralized cache mechanism and should NOT be handled using a collection or (even worse) a syncronized collection. Using Caching in Sakai should provide information about how this should be done.

No swallowed exceptions

All exceptions in the kernel must be handled. They should either be rethrown as a runtime exception or handled in the code according to the logic and situation. There should never be empty exception blocks with a comment in them (except in the case of a try-catch-finally-try-catch).

No dependencies on code outside the kernel

Kernel code can have absolutely no dependencies on code outside the current kernel. If packages need to be added to the kernel then this must be kept to the absolute minimum and must be approved by the technical community.

No usage of static covers

Kernel code should not make use of the static covers (also known as the "singleton" method in Java, not to be confused with the use of Spring singletons) and should instead use Spring setter injection. This makes testing and debugging easier (and possible) and also makes it easier to debug issues. It also provides for a more flexible and well defined kernel. Code which makes use of covers will be refactored.

No lookups of components (spring beans) when they can be loaded via injection

Kernel code should always use setter injection to load singleton dependencies. Loading components via lookups and methods like "getBean" makes testing virtually impossible. It also makes the bean dependency graph unpredicatable. The issues are generally the same of the ones encountered when using static covers.

Programmatic testing required

All code used in the kernel must have programmatic tests. These could be unit tests, integration tests, system tests or even validity and load tests. The important thing here is to make sure that the code is behaving the way it is designed to behave. Testing also ensures that code is more change tolerant since the tests will fail if something changes improperly.

No data inconsistency

Logically atomic database operations must be wrapped by atomic database transactions. Depending on the operation, this may involve joining an open transaction or beginning a new one. In either case, no kernel code should leave the system in an inconsistent state.

Static utils should be able to run in isolation

Utils should not use anything that is not also static (and certainly should not depend on Spring beans).
Utils should strive to have no dependencies if possible.

General Best Practices

  • Documents and defines best practices for programming in Sakai (though many of these are generally good pratices to follow for Java in general).

Minimal usage of synchronized collections (Hashtable, Vector, etc.)

The use of synchronized collections (Vector, Hashtable) should be a last resort and only used when absolutely necessary. These should primarily be used for thread safety. However, the cost of writes AND reads are expensive when using these (easily 10-100x more than using the basic unsychronized versions). The cost of synchronization can become even more severe in a highly concurrent environment like Sakai, especially when it is deployed on a multi-core server.
When thread safety is not needed (99% of the time in Sakai) an ArrayList, HashSet, or HashMap should be used instead.
If thread safety is needed together with the performance of lock-free code, the best choice will usually be ConcurrentHashMap. CopyOnWriteArrayList and CopyOnWriteArraySet are other classes that could be useful in very specialised situations.
More info on concurrent collections here: http://www.ibm.com/developerworks/java/library/j-jtp07233.html

Use StringBuilder for appending strings when appropriate

Strings can be appended in many ways in java. The most common are to simply add the strings together, use StringBuilder, or use StringBuffer. These are all appropriate at different times.

  1. Adding Strings
    This is appropriate when all the strings are appended in a single statement.
    You might hear people telling you that you should be using a StringBuffer/StringBuilder here but that is silly because that is what the compiler converts this into anyway. No reason to make your code longer and uglier.
  2. Using StringBuilder
    This is appropriate when you are appending strings in multiple statements. I would personally not bother unless there are 3+ strings to append.
  3. Using StringBuffer
    In Java 1.5-level code and above, any explicit use of StringBuffer should be replaced with the lock-free equivalent StringBuilder. There is almost never a good reason to use StringBuffer. This is a synchronized object and therefore very slow to use unless you actually need the synchronization. In general, you should never use this over StringBuilder unless you have a very good reason (and need thread safety).

Minimize API dependencies

Interfaces in Sakai services (and in general) should be written to minimize dependencies on other packages when possible. This means not requiring a User object when the identity of a user is needed, but instead requiring the userID. When using an id instead of the actual object you should clearly identify what is expected in the javadocs like so:

Use numeric autogenerated ids in database tables

There are many ways to generate IDs for database tables, but one of the simplest, fastest, and safest is to allow the database to do it for you using numeric increamenting. This is supported in every database and is easy to do with hibernate. This also can reduce locking issues in MySQL and creates relatively simple looking IDs. Here are some helpful links:
HSQLDB Identity MYSQL autoincrement ORACLE sequence

  • If you want to ensure that you also have globally unique ids for your data, there are 2 ways to make these globally safe
    1. Append prefixes like: edu.rutgers.sakai.evaluation.answer.123, (system-prefix).(app-prefix).(table-prefix).id
    2. Store an eid (GUID, or whatever you like) in the tables along with the autogened id and use that externally

Compare strings/constants by putting them on the left side of the comparison

You can avoid many NullPointerExceptions by simply always placing hardcoded string and constants on the left side of an object comparison. This works well for any object constants (not just strings).
For example, this code can generate a NullPointerException if myString is null (bad):

This example code can never generate a NullPointerException (good):

Cleanup user submitted strings before storing them (i.e. do not trust input)

You can avoid issues related to XSS (Cross Site Scripting) by simply cleaning up the strings that are submitted to your services. This should be done in all systems and can be handled in the webapp (tool) or in the services (probably more ideal). In Sakai this can be done using the utils from the kernel but external jars are fine as well if you have one you are comfortable with.
For example, this code takes a user submitted string and returns the cleaned up version. It would be called any time there is any user data being input (that includes data from web form radio buttons and pulldowns).

Ideally a method that does not require creating a StringBuffer would be available but that is not currently the case.

Other resources on Best practices