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