current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Is there a better way to handle this ClassNotFoundException ?

private Class<?> getClass(String value)
{
    Class<?> columnClass = null;
    try
    {
        columnClass = Class.forName(StringUtils.trim(value));
    }
    catch (ClassNotFoundException ex)
    {
        if (value.contains("double") || value.contains("Double"))
        {
            columnClass = Double.class;
        }
        else if (value.contains("int") || value.contains("Int"))
        {
            columnClass = Integer.class;
        }
        else if (value.contains("bool"))
        {
            columnClass = Boolean.class;
        }
        else if (value.contains("long") || value.contains("Long"))
        {
            columnClass = Long.class;
        }
        else
        {
            log.error("FAILED. Class object is not supported: " + value, ex);
        }
    }
    return columnClass;
}
share|improve this question
    
Can you provide more context about how this code is called, and how often? –  rolfl♦ Apr 2 '14 at 22:50
    
I have an internal table describing a huge data set with each type of column having different types of classes (Integer, Double, Boolean etc...), so when I load the data into the table or GUI, the formatting for each column is displayed appropiately based upon class type. –  Brian Apr 3 '14 at 13:56
    
It would only call this method once per column, with only a few columns < 10. –  Brian Apr 3 '14 at 14:20

2 Answers 2

up vote 3 down vote accepted

With a limited set as this, you might as well skip the Class.forName() entirely and just keep the if statements. Exceptions are expensive, if statements not so much and there are only 4 options anyway.

Furthermore you could reduce the semi-repeating a little by providing a unified version of the input to compare against (all characters in lower/uppercase).

In fact, I would change it to use a simple lookup table.

This results in something like this:

static Map<String, Class<?>> lookup = new HashMap<>();

static {
    lookup.put("double", Double.class);
    lookup.put("int", Integer.class);
    lookup.put("bool", Boolean.class);
    lookup.put("long", Long.class);
}

private Class<?> getClass(String value){
    for(String key : lookup.keySet()){
        if(value.toLowerCase().contains(key.toLowerCase())){
            return lookup.get(key);
        }
    }
    return null;
}

You can remove the intermediate columnClass variable entirely since the try-catch is now gone and there is no additional logic.

share|improve this answer
    
The OP's 'contains' logic and your switch logic are not the same. –  rolfl♦ Apr 2 '14 at 22:40
    
Ah, right. I first switched everything to if statements and stopped thinking at that point and moved it to a switch instead. Let me just revert that.. –  Jeroen Vannevel Apr 2 '14 at 22:41
    
I think the OP's use-case is significantly flawed anyway.... really, your answer is fine-ish, given the question. –  rolfl♦ Apr 2 '14 at 22:42
    
3rd approach, best approach. –  Jeroen Vannevel Apr 2 '14 at 22:51
up vote 3 down vote

You do not provide much context for this method, so I can only give you general advice.

Logic

  1. never use exception handling as part of the routine/regular code path in you program. Most Java VM's require significant locking and processing to generate the actual exception and its stack trace. I have seen 10% performance improvements in large commercial applications where simple pre-validation of common exceptional conditions saves a huge amount of processing resources.

  2. What if the user enters the non-existent class name dev.null.TurningIntoZomies, they get an Integer.class back.

  3. In the event that the Class.forname(...) fails, you do exception handling and return null. If the program asks for the same value again, it will do the full search again, and so on. If the class exists, it is not normally very slow, but, if the class does not exist, the Class.forname() has to search the entire classpath. Cache the results (of both the successes and the failures so you only have to call Class.forName() once.

Code Style

  • Use correct Java brace placement, at the end of the line.

Finally, you can use short-circuit return statements to make it easier:

private Class<?> getClass(String value) {
    try {
        return Class.forName(StringUtils.trim(value));
    } catch (ClassNotFoundException ex) {
        if (value.contains("double") || value.contains("Double")) {
            return Double.class;
        }
        if (value.contains("int") || value.contains("Int")) {
            return Integer.class;
        }
        if (value.contains("bool")) {
            return = Boolean.class;
        }
        if (value.contains("long") || value.contains("Long")) {
            return Long.class;
        }
        log.error("FAILED. Class object is not supported: " + value, ex);
        return null;

    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.