30

We're implementing an adapter for Jaxen (an XPath library for Java) that allows us to use XPath to access the data model of our application.

This is done by implementing classes which map strings (passed to us from Jaxen) into elements of our data model. We estimate we'll need around 100 classes with over 1000 string comparisons in total.

I think that the best way to do this is simple if/else statements with the strings written directly into the code — rather than defining each strings as a constant. For example:

public Object getNode(String name) {
    if ("name".equals(name)) {
        return contact.getFullName();
    } else if ("title".equals(name)) {
        return contact.getTitle();
    } else if ("first_name".equals(name)) {
        return contact.getFirstName();
    } else if ("last_name".equals(name)) {
        return contact.getLastName();
    ...

However I was always taught that we should not embed string values directly into code, but create string constants instead. That would look something like this:

private static final String NAME = "name";
private static final String TITLE = "title";
private static final String FIRST_NAME = "first_name";
private static final String LAST_NAME = "last_name";

public Object getNode(String name) {
    if (NAME.equals(name)) {
        return contact.getFullName();
    } else if (TITLE.equals(name)) {
        return contact.getTitle();
    } else if (FIRST_NAME.equals(name)) {
        return contact.getFirstName();
    } else if (LAST_NAME.equals(name)) {
        return contact.getLastName();
    ...

In this case I think it's a bad idea. The constant will only ever be used once, in the getNode() method. Using the strings directly is just as easy to read and understand as using constants, and saves us writing at least a thousand lines of code.

So is there any reason to define string constants for a single use? Or is it acceptable to use strings directly?

PS. Before anyone suggests using enums instead, we prototyped that but the enum conversion is 15 times slower than simple string comparison so it's not being considered.

Conclusion: The answers below expanded the scope of this question beyond just string constants, so I have two conclusions:

  • It's probably OK to use the strings directly rather than string constants in this scenario, but
  • There are ways to avoid using strings at all, which might be better.

So I'm going to try the wrapper technique which avoids strings completely. Unfortunately we can't use the string switch statement because we're not on Java 7 yet. Ultimately, though, I think the best answer for us is to try each technique and evaluate its performance. The reality is that if one technique is clearly faster then we'll probably choose it regardless of its beauty or adherence to convention.

gutch
  • 520

6 Answers6

6

If at all possible use Java 7 which allows you to use Strings in switch statements.

From http://docs.oracle.com/javase/tutorial/java/nutsandbolts/switch.html

public class StringSwitchDemo {

    public static int getMonthNumber(String month) {

        int monthNumber = 0;

        if (month == null) {
            return monthNumber;
        }

        switch (month.toLowerCase()) {
            case "january":
                monthNumber = 1;
                break;
            case "february":
                monthNumber = 2;
                break;
            case "march":
                monthNumber = 3;
                break;
            case "april":
                monthNumber = 4;
                break;
            case "may":
                monthNumber = 5;
                break;
            case "june":
                monthNumber = 6;
                break;
            case "july":
                monthNumber = 7;
                break;
            case "august":
                monthNumber = 8;
                break;
            case "september":
                monthNumber = 9;
                break;
            case "october":
                monthNumber = 10;
                break;
            case "november":
                monthNumber = 11;
                break;
            case "december":
                monthNumber = 12;
                break;
            default: 
                monthNumber = 0;
                break;
        }

        return monthNumber;
    }

    public static void main(String[] args) {

        String month = "August";

        int returnedMonthNumber =
            StringSwitchDemo.getMonthNumber(month);

        if (returnedMonthNumber == 0) {
            System.out.println("Invalid month");
        } else {
            System.out.println(returnedMonthNumber);
        }
    }
}

I have not measured, but I believe that the switch statements compile to a jump table instead of a long list of comparisons. This should be even faster.

Regarding your actual question: If you only use it once you do not need to make it into a constant. Consider however that a constant can be documented and shows up in Javadoc. This may be important for non-trivial string values.

6

Try this. The initial reflection is certainly expensive, but if you're going to use it many many times, which I think you will, this is most certainly a better solution what what you're proposing. I don't like using reflection, but I find myself using it when I don't like the alternative to reflection. I do think that this will save your team a lot of headache, but you must pass the name of the method (in lowercase).

In other words, rather than pass "name", you would pass "fullname" because the name of the get method is "getFullName()". 

Map<String, Method> methodMapping = null;

public Object getNode(String name) {
    Map<String, Method> methods = getMethodMapping(contact.getClass());
    return methods.get(name).invoke(contact);
}

public Map<String, Method> getMethodMapping(Class<?> contact) {
    if(methodMapping == null) {
        Map<String, Method> mapping = new HashMap<String, Method>();
        Method[] methods = contact.getDeclaredMethods();
        for(Method method : methods) {
            if(method.getParameterTypes().length() == 0) {
                if(method.getName().startsWith("get")) {
                    mapping.put(method.getName().substring(3).toLower(), method);
                } else if (method.getName().startsWith("is"))) {
                    mapping.put(method.getName().substring(2).toLower(), method);
                }
            }
        }
        methodMapping = mapping;
    }
    return methodMapping;
}

If you need to access data contained within members of contact, you might consider building a wrapper class for contact which has all methods to access any information required. This would also be useful for guaranteeing that the names of the access fields will always remain the same (I.e. if wrapper class has getFullName() and you call with fullname, it will always work even if contact's getFullName() has been renamed -- it would cause compilation error before it would let you do that).

public class ContactWrapper {
    private Contact contact;

    public ContactWrapper(Contact contact) {
        this.contact = contact;
    }

    public String getFullName() {
        return contact.getFullName();
    }
    ...
}

This solution has saved me several times, namely when I wanted to have a single data representation to use in jsf datatables and when that data needed to be exported into a report using jasper (which doesn't handle complicated object accessors well in my experience).

Neil
  • 22,848
4

If you're going to maintain this (make any sort of nontrivial changes ever), I might actually consider using either some sort of annotation-driven code generation (perhaps via CGLib) or even just a script that writes all the code for you. Imagine the number of typos and errors that could creep in with the approach you're considering...

Steven Schlansker
  • 1,626
2

I would still use constants defined at the top of your classes. It makes your code more maintainable since it's easier to see what can be changed at a later time (if necessary). For instance, "first_name" could become "firstName" at some later time.

Bernard
  • 8,869
1

If your naming is consistent (aka "some_whatever" is always mapped to getSomeWhatever()) you could use reflection to determine and execute the get method.

scarfridge
  • 1,816
0

I guess, annotation processing could be the solution, even without annotations. It's the thing which can generate all the boring code for you. The downside is that you'll get N generated classes for N model classes. You also can't add anything to an existing class, but writing something like

public Object getNode(String name) {
    return SomeModelClassHelper.getNode(this, name);
}

once per class should not be a problem. Alternatively, you could write something like

public Object getNode(String name) {
    return getHelper(getClass()).getNode(this, name);
}

in a common superclass.

You can use reflection instead of annotation processing for code generation. The downside is that you need your code to compile before you can use reflection on it. This means you can't rely on the generated code in your model classes, unless you generate some stubs.

I'd also consider direct use of reflection. Sure, reflection is slow, but why is it slow? It's because it has to do all the things you need to do, e.g., switch on the field name.

maaartinus
  • 2,713