Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
3  
You don't plan on manually keyboarding a 1000 if statements do you? –  JeffO Apr 24 '12 at 2:58
    
Yes we do! No matter how we do implement this, we somehow have to define each unique mapping. We could, say, use Java annotations but then we'd need 1000 annotations... not necessarily an improvement. –  gutch Apr 24 '12 at 3:23
1  
I find it so sad how unpleasant something this simple can be in some languages… –  Jon Purdy Apr 24 '12 at 4:08
5  
Java 7 allows Strings as switch labels. Use a switch instead of if cascades. –  Martin Schröder Apr 24 '12 at 6:53
1  
enum conversion is 15 times slower if you convert a string to its enum value! Pass enum directly and compare with another enum value of the same type! –  Neil Apr 24 '12 at 7:26

5 Answers 5

up vote 4 down vote accepted

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).

share|improve this answer
    
I like the idea of a wrapper object with the methods called via .invoke(), because it does away with string constants entirely. I'm not so keen on runtime reflection to set up the map, though maybe executing getMethodMapping() in a static block would be OK so that it happens on startup instead of once the system is running. –  gutch Apr 27 '12 at 6:13
    
@gutch, wrapper pattern is one I use often, since it tends to risolve a lot of problems related to interface/controller. Interface can always use the wrapper and be happy with it and the controller can be turned inside out in the meantime. All you need to know is what data you want available in the interface. And again, I say for emphasis, I don't normally like reflection, but if it's a web application, it's completely acceptable if you do it on startup since the client isn't going to see any of that wait time. –  Neil Apr 27 '12 at 8:42

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.

share|improve this answer
2  
Concerning the jump table. The String switch is replaced by to switches, first is based on the hashcode (equality is checked for all constants with same hashcode) and selects the branch index, second switches on the branch index and selects the original branch code. The latter one is clearly suitable for a branch table, the former is not due to the distribution of the hash function. So any performance advantage is probably due to the hash based realization. –  scarfridge Apr 24 '12 at 8:09
    
A very good point; if it performs well it might be worth moving to Java 7 just for this... –  gutch Apr 27 '12 at 6:16

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...

share|improve this answer
    
We considered annotating existing methods, but some mappings traverse multiple objects (for example "country" mapping to object.getAddress().getCountry()) which is difficult to represent with annotations. The if/else string comparisons aren't pretty, but they're fast, flexible, easy to understand and easy to unit test. –  gutch Apr 24 '12 at 4:40
1  
You're right about the potential for typos and errors; my only defence there is unit tests. Of course that means yet more code... –  gutch Apr 24 '12 at 4:45

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.

share|improve this answer
    
I agree, however, if this code will be generated automatically and the constants are not used elsewhere, then it does not matter (the OP says they need to do this in 100 classes). –  Emmad Kareem Apr 24 '12 at 3:18
3  
I just don't see the "maintainability' angle here you change "first_name" to "givenName" once in one place in either case. However in the case of named constants you are now left with a messy variable "first_name" which refers to a string "givenName" so you probably want to change that as well so you now have three changes in two places –  James Anderson Apr 24 '12 at 5:34
    
With the right IDE, these changes are trivial. What I am advocating is that it is more apparent where to make these changes because you've taken the time to declare constants at the top of the class and don't have to read through the rest of the code in the class in order to make these changes. –  Bernard Apr 24 '12 at 12:26
    
But when you are reading the if statement, you have to go back and check that the constant contains the string you think it contains -- nothing saved here. –  James Anderson Apr 25 '12 at 10:26
    
Perhaps, but this is why I name my constants well. –  Bernard Apr 25 '12 at 13:35

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

share|improve this answer
    
Better getSome_whatever(). Might be breaking camel case, but it's far more important to make sure reflection works. Plus it has the added advantage that it makes you say, "Why the heck did we do it that way.. oh wait.. Wait! George don't change the name of that method!" –  Neil Apr 24 '12 at 7:29

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.