How can i improve this code in Java

Question

private void somename(HttpServletRequest request, pageBean UTIL) throws Exception
    {
        checkEmpHxAction(request, UTIL);

        long empRecNum = UTIL.getNumValue("EMPLOYEE", "REC_NUM");
        ListBean list = UTIL.getListBean(request, "EMPTRNHX", true);

    }

My boss says that there is an improvement needed in the coding style.

1. There are some issues with this code.
2. Should we not use ArrayList.
3. What can i make the code more readable.
4. Is there any memory leakage.

Please help!

Answer 1

First of all you could use a SQL Query Builder. i personnally favor SqlBuilder. This way, string concatenation could totally disappear below a prettier API. Notice that, by using SqlBuilder prepared statement handling, your query params could also disappear.

As an example, a part from your first method could become

private void getEmpHistoryList(HttpServletRequest request, pageBean UTIL) throws Exception
{
    checkEmpHxAction(request, UTIL);

    long empRecNum = UTIL.getNumValue("EMPLOYEE", "REC_NUM");
    ListBean list = UTIL.getListBean(request, "EMPTRNHX", true);
    if(empRecNum != 0)
    {
        list.loadRequestParms(request, 'a', 20);
        char sort = list.getSort();
        StringBuffer sql = new StringBuffer();
        ArrayList qryParms = new ArrayList();
        SelectQuery query = new SelectQuery().addCustomColumns("C.FLDREC_NUM", "R.FLDREC_NUM", "@CHARDATE(C.FLDDATE)", "C.FLDSHORTDES")/* and so on */
            .addCustomFromTable("@SCHEMATRNROSTR R")
            .addCustomFromTable("@SCHEMATRNCLASS C")
            .addCondition(BinaryCondition.equalTo("R.FLDCLASS", "C.FLDREC_NUM"))
            .addCondition(BinaryCondition.equalTo("R.FLDEMPLOYEE", "?")) /* here you'll use prepared statement query placeholder */;
        qryParms.add(new Long(empRecNum));
        switch(sort)
        {
            default:
            case 'a':
                query.addOrdering("C.FLDDATE", Dir.DESCENDING);
                sql.append("C.FLDDATE DESC");
                break;
                /* and so on */
        }
        list.setQuery(UTIL, query.toString(), qryParms);
    }
    else
        list.init();
}

Answer 2

i have following question about the code.

1. what this checkEmpHxAction(request, UTIL) does , why u are calling it inside the getEmpHistoryList , you are not checking the return value of this method , is this an one time operation ,like you set something.

Answer 3

First of all, you need to separate layers in your application. A database component should not know anything about servlet requests and vice-versa.

MVC anyone?

Answer 4

Hi John,

I totally agree with your boss and seanizer. This code definitely needs some TLC. Firstly, if I were refactoring this, I would write a test (preferably several) first. Once you've done that and you know you're safely covered by unit tests, you can start some simple refactors. Even very small steps help readability, such as:

1. Stick to a coding convention: why is UTILS in all caps?
2. Separation of concerns: Your data access code should not be mixed with your servlet request processing code as seanizer said.
3. Magic numbers: What does 'a' and '20' mean in this: list.loadRequestParms(request, 'a', 20);? These should be constants somewhere with names that mean something.
4. Complexity: There are too many code branches in this one method and it is too long.

I would recommend this book "Clean Code: A Handbook of Agile Software Craftsmanship" which is a great read on how to write neat code. Furthermore, you should use Checkstyle, which is a great tool to finding common coding problems and enforcing a good code standard.

To answer your question (2) about ArrayList. What's wrong with ArrayList? You shouldn't be declaring it as an ArrayList, rather just as a List, but ArrayList itself is fine. Why are you adding wrapped Longs to this list? Are you not on 1.5 or better?

To answer your question (4) about memory leaks. It is impossible to tell from just reading this code. Once you have refactored it it would be easier, and with the nice tests you wrote in step one of this answer, you can easily run those tests many times to see if you memory consumption goes up given repeated calls of this method.

I hope that's enough to get you started... Good luck!

Answer 5

For readability (performance does not matter): Declare a variable just before you use it. qryParms is created somewhere at the top, few lines later used for the second time and after the switch for the first time read. Those three lines should be grouped together.

                case 'D':
                sql.append("C.FLDCOURSE DESC");
                break;
        }
ArrayList qryParms = new ArrayList();
qryParms.add(new Long(empRecNum));
list.setQuery(UTIL, sql, qryParms);

Same for char sort = list.getSort(); that should be the line before switch(sort). For example

int a=calculateSomething();
/** lots of code here*/
// more code
// still more code
if (a==234) //what? a? where did a come from?
{}

Vs.

/** lots of code here*/
// more code
// still more code
int a=calculateSomething();
if (a==234) //just calculated a
{}

The second one is easier to read: you do not have to remember a for more than one line of code.

Answer 6

In addition to all of the great things that have been said, if you have access to the code in ListBean I would recommend having getSort return an enumeration instead of a char. If an enumeration doesn't fit, then a String representing the field name you're sorting on. The goal is to remove that ugly case statement or at least make it more readable. But it seems like your method is being forced to do translation of the (apparently) meaningless and arbitrary character to a proper field name, and that shouldn't be in here.

Also, what does ListBean do? It looks like it's used for many different things.

Answer 7

I'd replace the switch with a map:

private static Map<String,String> sortMap = new HashMap<String,String>();
static {
    sortMap.put("a", "C.FLDDATE DESC");
    sortMap.put("b", "C.FLDSHORTDES");
    sortMap.put("c", "C.FLDHOURS");
    // etc
}

Now you can replace the switch statement with:

sql.append(sortMap.get(sort));

Or, more correct with respect to the implementation above:

String sortChoice = sortMap.get(sort);
sql.append(sortChoice == null ? sortMap.get("a") : sortChoice);

Then replace the "a" with a DEFAULT_SORT_CHOICE constant.

If you have to add more sort codes then you just have to add to the map, not change the SQL itself. Since the map is static it will be loaded one time not each time the method is invoked. You can do other things to this code to make it run a little faster, but the actual database fetch is likely to take far more time than what we'll save, so that's where any performance improvement should begin. And only if it's needed.