Wednesday, March 16, 2011

Its time for some code review

I know some of us just hate it when we hear the word code review. The thought of some one else going through our code is somewhat offensive to some people. I myself have come across many. But for me this is a good thing given that my reviewer knows what to look for and have a set of parameters set out as a base for him/her to review on. So in this article i would like to highlight a few mistakes i have come across when doing code reviews. Most of them are common but we tend to often miss out on those. This is not the end of them. So if you guys have come across any thing other than what is highlighted here please do leave a comment as it would be beneficial for me as well as the other readers.




Integer myVal = new Integer(10);

Ofcourse by the look of this code there is nothing intrinsically wrong with it. The thing is in this code snippet you create an instance of a new Integer wrapper class. You can enhance this as follows;

Integer myVal = Integer.valueOf(10);

Using the valueOf method is better performance wise as this method returns the value from the in memory cache rather than creating a new object. Check here for what the java api says in this regard.

String records = "";

        for (int i = 0; i < 10; i++) {
            records+="new record"+i;
        }

Ok the issue here is as some of you already might have guessed is string concatenation. Though this works there is an issue with performance with respect to string concatenation. For those of you who do not know, when you concatenate two strings what really happens internally is a new string buffer is created and the new string is assigned to it and toString() is called on that string buffer object. Now this happens on every loop call. So you can understand the performance impact this will have.

Ofcourse prior to java 1.6 you would have argued about so many objects being created on the heap as a result of this. But with java 1.6 escape analysis, any object created within the scope of a method is created on the stack and not on the heap. And hence that argument is invalid. So a better way to handle this situation is to use StringBuilder as follows;



StringBuilder records = new StringBuilder(100);

        for (int i = 0; i < 10; i++) {
            records.append("new record").append(i);
        }

Note that i have also specified an initial size to the String Builder within the constructor. We do this for efficiency because if you do not define an initial size the default size of 16 characters is given by default. Whenever your buffer exceeds this limit a new StringBuilder object is created and the whole String that was in the old buffer is copied to the new one. To avoid such transactions taking place we give an initial capacity that we deem appropriate.

int i=0;
       .......
       .......
       .......
       .......
       .......
       .......
       .......
       .......
       
       i = 2+4;

This is a common mistake i see many people make. You define a variable at one point and afterwards you do various other method calls and at a latter point in that method you manipulate that variable you defined early on in the method. As a best practice always use the variable as soon as it is defined. This provides much clarity to your code and keeps it clean.

Using the same example i would like to point out another thing. See that the code above defines an integer called "i". This is ok to use within a for loop but if this is in the context of a method then always use meaningful names as this will self describe your code minimizing the code commenting you need to do.

/**
     * Blah blah blah blah blah blah blah
     * Blah blah blah blah blah blah blah
     * Blah blah blah blah blah blah blah
     * Blah blah blah blah blah blah blah
     * Blah blah blah blah blah blah blah
     * Blah blah blah blah blah blah blah
     * Blah blah blah blah blah blah blah
     * Blah blah blah blah blah blah blah
     * 
     */
    public void myTestMethod() {
        
    }


Please stop writing lengthy code comments. In my point of view is this is just an utter waste of time. The fact that you need to write such a lengthy explanation to me disseminates the fact that this method does more than what it should do. Write a short description on what your method does. A simple explanation of business logic is more than enough. Make your code readable which reduces the time you have to comment.


/**
     * Sets the name
     * @param name
     */
    public void setMyName(String name) {
        this.name = name;
    }

Wow what a code comment. The author says it sets the name variable. Wow really? Thank goodness for that comment else i would not have known what was going on in that method :) . Seriously stop making useless code comments. It just clutters your code and does no use for anyone reading it. Dont comment on getter/setter methods as the name it self implies what is happening.

public void doProcess()throws MyServiceException {
        
        try {
            service.doProcess();
        }catch(MyServiceException e) {
            log.error("Error occured");
            throw e;
        }
    }

Ok whats wrong here? Problem here is that this method catches a MyServiceException and logs the error and rethrows the same exception back from the method. Why would you catch an exception if you are going to rethrow the same? It just does not make sense. Proper way to handle such a thing is to let the caller handle the exception and log the error at that end rather than you trying to handle it. Anyway it is recommended now to throw Runtime exceptions rather than checked exceptions if the error you throw is like a DB exception or IO exception as these kind of exceptions does not make sense to the client.


public void doProcess()throws MyServiceException {
        
            log.debug("Started method");
            service.doProcess();
            log.debug("Method processed");


    }

Here the issue lies in the debug log statement. Often times in a production environment your log level will mostly be set to INFO or ERROR. But when your code has debug logs the Logger will try to process this which results in unnecessary executions. You can avoid this by checkin if debug is enabled as shown in the below code snippet;

public void doProcess() throws MyServiceException {

        if (log.isDebugEnabled()) {
            log.debug("Started method");
        }
        service.doProcess();
        if (log.isDebugEnabled()) {
            log.debug("Method processed");
        }

    }



if(server.equals("TEST")) {
            //do something
        }else if(server.equals("TEST123")) {
            //do something
        }else if(server.equals("TEST1234")) {
            //do something
        }else if(server.equals("TEST12345")) {
            //do something
        }else if(server.equals("TEST123456")) {
            //do something
        }

In an instance where you are tangled in multiple if/else statement it is always much cleaner to use switch statements and use enums to hold the constants.With JDK 1.7 you are allowed to use Strings in a switch statement. So your code above will look like this with use of enums and switch statement;

switch (server) {
        case TEST:
            // do something
            break;

        case TEST123:
            // do something
            break;
        case TEST1234:
            // do something
            break;
        case TEST12345:
            // do something
            break;
        case TEST123456:
            // do something
            break;
        }

Those are few of the things that came to my mind at the time of writing this post. Ofcourse there are many more which i will follow in subsequent posts. Pls do leave a comment which will be an additions to this post that you deem appropriate for a code review session.


Cheers Guys

10 comments:

  1. Have you actually tested your claims about on-stack allocation of strings thanks to escape analysis? I think you might be suprised of differences between what is promised (or, even better, what is wrongly perceived as being promised) and what is really done in JDK1.6.

    ReplyDelete
  2. Hi Artur,

    I actually have not taken the time to test this feature specifically. is there any article or test done to prove its not working as it is said to be working? thx for bringing this up.. Will investigate on this..

    Cheers
    Dinuka

    ReplyDelete
  3. Referring to Log4J: The first line of LOG.debug() is a conditional that calls LOG.isDebugEnabled(), so your recommended preemptive check is not necessary unless you are attempting to avoid method parameter construction, which your example does not illustrate.

    http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Category.html#debug(java.lang.Object)

    //NO!
    if (log.isDebugEnabled()) {
    log.debug("Method processed");
    }

    //YES!
    if (log.isDebugEnabled()) {
    log.debug("Method processed at time: " + new Date());
    }

    ReplyDelete
  4. thx for sharing that pointer... Appreciate it :)

    Cheers

    ReplyDelete
  5. Hi,

    This is all marvelous and fine, but in my view, exemples showed theres are not really interresting for a code review. Most of them can be found in tool like sonar/PMD/FindBug/Checkstyle automaticaly anyway. And really theses things are details or in some case more about a matter of taste.

    For my taste :

    Integer myVal = new Integer(10);
    I really don't care except if you can prove me it is inside a big loop and most number are in the -127 +128 range. If this not the case, it isn't worth the trouble and is even slower for number outside range.
    Byt my replacement of choice would be :
    Integer myVal = 5;

    String concatenation : who care for a loop with ten iterations ? Code is quite readable and is correct. We should care about performance only on critical code.

    Variable declaration : some code review state the exact oposite : a matter of state. I really don't care myself.

    Comments : It depend of what is your job. If you make an API, you should provide extensives comments even for getters. If you are just making a small, non critical program you can live with just a few comments.

    "Anyway it is recommended now to throw Runtime exceptions rather than checked exceptions if the error you throw is like a DB exception or IO exception as these kind of exceptions does not make sense to the client."
    -> Depend of the context. A file not found typicaly matter to the client. More than that, this type of error are the most common and can be easily be recovered. In theses case i would typically use CheckedException because the code can do something about it. I would use RuntimeException for bugs and unrecoverables errors where there isn't much solutions to recover.

    ReplyDelete
  6. Nice comments only one comment, when you say that log.isDebugEnabled(), this condition is only aplicable for log4j logger familiy, for example in Logback this is not required.

    And one question I have also a blog, how do you do for creating source code with lines? Thank you very much and keep writing.

    ReplyDelete
  7. @Nicolas - Thx for your comment. Its very descriptive and many will benefit from it. Thx alot. As for your comments. The ten iterations was just for an example. what i tried to imply was that when you have alot of iterations its best not to use String concatenation.

    And also Integer i = 5 works and i prefer it too. But in most cases we use wrapper classes in the even when you need to pass a string to it so i have to use either method in that case.

    @Alexander Ashitkin - Oops seems like i offended you in some way :) ... a better criticism would have been better for me to improve my self. Sorry if my writing bothers you. Thx for leaving a comment though.. Cheers


    @Alex - Hi man. Thx alot for that feed back and happy you loved my article... As for the code block snipper i have used syntaxhighligher where you can find at http://code.google.com/p/syntaxhighlighter/

    Hope that helps. Cheers man

    ReplyDelete
  8. your welcome.. glad you liked it.. Cheers man

    ReplyDelete
  9. @ arapidhs thx for your comment man.. Unfortunately i mistakenly pressed the delete button and erased your comment. So Sry mate. Thx for leaving by a comment. Cheers man

    ReplyDelete