Code Review Guide

Introduction

The following areas are important when conducting code review:

1. Does the solution provide custom class libraries for reusable classes & methods?

2. What classes have been extended or implemented and whether they are supported?

3. Look for deprecated classes & methods. Look for alternate methods if deprecated methods used.

4. Have the proper classes been extended to provide functionality based on functional requirements? Check whether objects have been extended too heavily where a lighter extension would suffice.

5. Are proper abstract classes & interfaces used to provide a flexible, yet cohesive design model?

6. Has code been properly documented, for instance using JavaDoc?

And in general, we will look for the 7 Deadly Sins of Software Design and make recommendation accordingly:

1. Rigidity: Make it hard to change, especially if changes result in ripple effects or when you don't know what will happen when you make changes.

2. Fragility: Make it easy to break. Whenever you change something, something breaks.

3. Immobility: Make it hard to reuse. When something is coupled to everything. When you try to take a piece of code (class etc.) it takes all of its dependencies with it.

4. Viscosity: Make it hard to do the right thing. There are usually several ways to work with a design. Viscosity happens when it is hard to work with the design the way the designer intended to. The results are tricks and workarounds that, many times, have unexpected outcomes (especially if the design is also fragile).

5. Needless Complexity (Over design): When you overdo it (the "Swiss-Army knife" anti-pattern). A class that tries to anticipate every possible need. Another example is applying too many patterns to a simple problem.

6. Needless Repetition: The same code is scattered around which makes it error prone.

7. The 7th Deadly Sin of Software Design is (the obvious) "Not doing any".

Clean Unnecessary Code

As our business needs and techniques evolve, there are more and more changes to our implementation, therefore a lot of code gets deprecated. Some of this may need to remain, due to legacy data. However, some of it can be cleaned to make maintenance easy and improve performance.

Different cases that might be found in the review:

• Whole classes are deprecated

• Code segments are unnecessary due to changes.

• Redundant registration of listener services.

A good planning at the beginning will help to avoid messy code. And whenever there is a change to be implemented, plan it first with a thorough review to its impact and identify the code that needs to be changed. Only then implementation can start.

We recommend to review code that has a lot changes remove unnecessary code and merge similar code using team discussion. This may be scheduled after migrated system go-live. It will improve code quality, make it easy for debugging, and reduce unnecessary maintenance work.

Optimize and reduce database query to improve performance

Sometimes, we need to balance between Disk/Network and RAM access. We may improve performance at cost of memory, as long as memory consumption is not the bottleneck. As a rule of the thumb, a database query is more expensive then in memory processing in terms of performance, so we need to optimize and combine some of the queries to reduce database queries as much as possible.

Another balance is between the optimization and readability. We need to manage optimization in a controllable way.

Use local cache to improve performance

It is recommended to use local caches to store frequently accessed data to avoid database queries. Such a cache mechanism is simple and easy to implement.

Avoid the use of expensive operations

Avoid using any expensive operations such as String concatenations. String concatenations are expensive because Strings are constant; their values cannot be changed after they are created. So each concatenation will create a new String object.

Proper rollback of database transactions

For database transaction take care of rollback properly. Here is an example how not to do this:

Transaction trx = new Transaction();
try
{
    trx.start();
    // do something
    if (trx != null ) trx.commit;
    trx = null;
}
catch ( Exception e )
{
    // processing exception
}
if( trx != null )
{
    trx.rollback();
}

The consequence of this code is that if there is an error when executing the transaction, the transaction will not be rolled back correctly. This will have an impact on data integrity.

We recommend using the following structure for all database transactions to ensure data integrity.

//….
finally
{
    if( trx != null )
    {
        trx.rollback();
    }
}

Hard Coding

Hard code is hard to maintain and may cause potential problems.Here is an example:

public static String buildAuthURL ( String action, String cls, WfProcess wfprocess, String rolevalue, String nextAction ) throws Exception
{
    ReferenceFactory rf = new ReferenceFactory();
    Properties urlProperties = new Properties();
    urlProperties.put( "action", action );
    urlProperties.put( "class", cls );
    urlProperties.put( "wfprocess", rf.getReferenceString( wfprocess ) ); // WfProcess Oid
    urlProperties.put( "role", rolevalue );
    urlProperties.put( "nextAction", nextAction );
    //urlProperties.put("Users", getRoleMapUsers(wfprocess, rolevalue));
    String url = GatewayURL.buildAuthenticatedURL( "XX.enterprise.URLProcessor", "generateForm", urlProperties ).toExternalForm();
    url = url.substring( ( XXProperties.getLocalProperties().getProperty( "XX.server.codebase" ) ).length() - 10 );
    return url;
}

This code will remove the host name from the URL. However, it is hard coded and will break if there is a rehosting.

Resources not released

Resource should be released when it is not needed. Here is an example:

XXProperties XXp = XXProperties.getLocalProperties();
Properties prop = new Properties();
FileInputStream fis = new FileInputStream( XXp.getProperty ("XX.home", "e:/ptc/Windchill" ) + "/codebase/ext/misccorp/misccorp.properties" );
prop.load( fis );
return prop;

The FileInputStream is not closed before returning from the method.

Comply to Java Coding Standards

Code conventions are important to programmers for a number of reasons:

• 80% of the costs, over the lifetime of a piece of software, goes to maintenance.

• Hardly any software is maintained for its whole life by the original author.

• Code conventions improve the readability of the software, allowing engineers to understand new code more quickly and thoroughly.

• If you ship your source code as a product, you need to make sure it is well packaged and clean, as any other product you create.

eXist follows the Sun coding standards. In addition to that: