<?xml version="1.0" encoding="UTF-8"?>
<?oxygen RNGSchema="http://www.oasis-open.org/docbook/xml/5.0/rng/docbook.rng" type="xml"?><!-- $Id: devguide_codereview.xml 17299 2012-10-10 23:47:33Z joewiz $ --><book><bookinfo subject=""><productname>eXist-db – Open Source Native XML Database</productname><title>Developer's Guide</title><subject>Code Review Guide</subject><date>September 2009</date><author><firstname>Loren</firstname><surname>Cahlander</surname><affiliation><address format="linespecific"><email>loren at syntactica.com</email></address></affiliation></author></bookinfo><chapter><title>Code Review Guide</title><section><title>Introduction</title>Several aspects of the design and deployment of the
            custom solution will be analyzed and appropriate recommendations be provided as well.
            These criteria are to be followed when conducting code review. <orderedlist><listitem><para>Does the solution provide custom class libraries for reusable classes
                        &amp; methods</para></listitem><listitem><para>What classes have been extended or implemented and whether they are
                        supported. </para></listitem><listitem><para>Look for deprecated classes &amp; methods. Look for alternate methods if
                        deprecated methods used. </para></listitem><listitem><para>Have proper classes been extended to provide functionality based on
                        functional requirements? Need to check whether too heavy of objects been
                        extended where a lighter weight object would suffice. </para></listitem><listitem><para>Has proper abstract classes &amp; interfaces been used to provide a
                        flexible, yet cohesive design model? </para></listitem><listitem><para>Has code been properly documented: JavaDoc, etc.?</para></listitem></orderedlist><para>And in general, we will look for the 7 Deadly Sins of Software
                Design and make recommendation accordingly:</para><itemizedlist><listitem><para>Rigidity: Make it hard to change, especially if changes might result in
                        ripple effects or when you don't know what will happen when you make
                        changes. </para></listitem><listitem><para>Fragility: Make it easy to break. Whenever you change something, something
                        breaks. </para></listitem><listitem><para>Immobility: Make it hard to reuse. When something is coupled to everything
                        it uses. When you try to take a piece of code (class etc.) it takes all of
                        its dependencies with it. </para></listitem><listitem><para>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 (esp. if the design
                        is also fragile). </para></listitem><listitem><para>Needless Complexity (Over design): When you overdo it; e.g. 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 etc. </para></listitem><listitem><para>Needless Repetition: The same code is scattered about which makes it error
                        prone.</para></listitem></itemizedlist><para><emphasis role="italic">The 7th Deadly Sin of Software Design is (the obvious) "Not
                    doing any". </emphasis></para></section><section><title>Clean Unnecessary Code</title><para>As our business need and technique evolves, there are more and more changes to our
                implementation, and thus there are many code deprecated. Some of them may need to
                remain due to legacy data, but some of them can be cleaned to make the maintenance
                easy and even improve the performance.</para><para>Different cases that might be found in the review:</para><itemizedlist><listitem><para>Whole classes are deprecated</para></listitem><listitem><para>Some code segments are unnecessary due to changes.</para></listitem><listitem><para>Redundant registration of some listener services.</para></listitem></itemizedlist><para>A good planning at the beginning will help to avoid mess in the code. And whenever
                there is a change to be implemented, plan it first with a thorough review to its
                impact and identify those codes need to be changed at the same time. Only after that
                actions can be taken to implement it,</para><para>We recommend to review that code have a lot changes and remove unnecessary code
                and merge similar code with team discussion. This may be scheduled after migrated
                system go-live. This will improve code quality, make it easy for debugging, and
                reduce unnecessary maintenance work.</para></section><section><title>Optimize and reduce database query to improve performance</title><para>Sometimes, we need to balance between the access of Disk/Network and RAM. And we
                may improve performance at cost of memory as long as memory consumption is not the
                bottleneck. As a rule of thumb, 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 query as much as possible. Another balance need to
                control is between the optimization and readability. We need to manage optimization
                in a controllable way.</para></section><section><title>Use local cache to improve performance</title><para>It is recommended to use local cache to store those frequently access data to
                avoid database queries. This cache mechanism is simple and easy to implement.</para></section><section><title>Avoid the use of expensive operations</title><para>Avoid using any expensive operations such as String concatenation. String
                concatenation which is expensive because Strings are constant; their values cannot
                be changed after they are created. So each concatenation will create a new String
                object.</para></section><section><title>Proper rollback of database transactions</title><para>For database transaction, fewer places don’t rollback properly, here is an
                example:</para><programlisting format="linespecific" language="java">
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();
}
</programlisting><para>The consequence of this code is that if there is an error happens when executing
                the transaction, the transaction will not be rolled back correctly. This will have
                an impact to the data integrity. </para><para>Recommendations </para><para>We recommend using the following structure for all database transactions to ensure
                data integrity. </para><programlisting format="linespecific" language="java">
//….
finally
{
    if( trx != null )
    {
        trx.rollback();
    }
}
</programlisting></section><section><title>Hard Coding</title><para>Hard code is hard to maintain and may cause potential problems. </para><para>Here is an example:</para><programlisting format="linespecific" language="java">
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;
}
</programlisting><para>This code will remove the host name from the URL, however, it is hard coded and
                may break if there is a rehosting. </para></section><section><title>Resources not released</title><para>Resource should be released when it is not needed. </para><para>Here is an example extracted from a system: </para><programlisting format="linespecific" language="java">
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;
</programlisting><para>The FileInputStream is not closed before returning from the method.</para></section><section><title>Comply to Java Coding Standards</title><para>Code conventions are important
                to programmers for a number of reasons:</para><itemizedlist><listitem><para>80% of the lifetime cost of a piece of software goes to
                        maintenance.</para></listitem><listitem><para>Hardly any software is maintained for its whole life by the original
                        author.</para></listitem><listitem><para>Code conventions improve the readability of the software, allowing
                        engineers to understand new code more quickly and thoroughly.</para></listitem><listitem><para>If you ship your source code as a product, you need to make sure it is as
                        well packaged and clean as any other product you create.</para></listitem></itemizedlist><para>Sun coding standards is the standards eXist follows for coding. In
                addition to that, there is some addendum.</para><emphasis role="underline"><para> Comments </para></emphasis><para>Further details on JavaDoc comments over and above the Sun standard,
                can be found in the Sun Doc Comments how to guide. </para><para>Have JavaDoc comment
                for all classes </para><para>Each class should have a comment. This comment should
                describe the function, intent and role of the class. </para><para>Have JavaDoc
                comment for all methods </para><para>Each method should have a comment describing
                how the method is called and what it does. Discussion of implementation specifics
                should be avoided since this is not for the user of a method to know in most cases.
                That information belongs in implementation comments. </para><para>Within the method
                JavaDoc comment, info should be added on the parameters. Each method JavaDoc comment
                should contain an @param comment for each parameter, an @return comment if not a
                void or constructor method, and an @throws comment for each exception (cf.
                Documenting Exceptions with @throws Tag). </para><para>The method pre and post
                conditions should be documented here. Pre-conditions comprise parameter ranges and
                the overall state of the object and system expected when calling the method.
                Post-conditions should document the expected return value sets and the state of the
                object and system that will apply when the method exits. These should map to
                assertions. </para><para>The JavaDoc should also document traceability of this
                method to the design and the requirements. Have JavaDoc comment for all fields Each
                non-trivial field should have a comment describing the role and purpose of the
                field, as well as any other appropriate information such as the range.
                </para><emphasis role="underline"><para>Exceptions</para></emphasis><para>All exceptions should be handled, it is never acceptable to simply
                print the exception message and stack trace. Exceptions should be dealt and
                corrective or informative action taken to highlight the issue.</para><para>For
                debugging purposes the stack trace should be logged at the final destination of the
                exception or at whenever the exception is modified, for example throwing a
                XXException instead of a java.io.IOException.</para><emphasis role="underline"><para>Logging</para></emphasis><para>If no logging system is in use in the package already or the logging is
                unconditional, then log4j should be used for all logging. Please see the <emphasis role="bold"><emphasis role="italic">Log4J Logging
                Guide</emphasis></emphasis>.</para><para>If a class has unconditional logging, then
                it should be updated to use log4j. A case of unconditional logging is where there
                are System.out.println() in the code with no conditions surrounding the call. This
                unnecessarily clutters the log and places a burden on
                performance.</para><para>Logging should always be performed at method exit and entry
                as follows:</para><itemizedlist><listitem><para>Log entry</para></listitem><listitem>Log arguments</listitem><listitem><para>Log exit</para></listitem><listitem><para>Log return values</para></listitem></itemizedlist><para>The occurrence of exceptions and the stack trace should also be
                logged as info level items.</para><para>Logging calls should be wrapped in
                enablement checks so that arguments do not get unnecessarily evaluated for
                example:</para><programlisting format="linespecific" language="java">
  If (getLogger().isDebugEnabled() {
    getLogger().debug("Entering - method - argList");
    getLogger().debug("Arg1 :"+ theArgValue1); 
    getLogger().debug("Arg2 :"+ theArgValue2);
  }
</programlisting><emphasis role="underline"><para>Assertions </para></emphasis><para>Assertions should be used in the code to verify that the expected
                results have occurred. Assertions should be used as liberally as possible.
                </para><para>Standard Assertions should be performed at method entry and exit; these
                methods should validate the pre and post-conditions for the method. All arguments
                should be checked for validity as should the return values. Similarly the state of
                the object and broader system should be checked as appropriate on both method entry
                and exit, for example if a file is open. </para></section></chapter></book>