Better Design Pattern?

Question

I have myArrayList that already have values in it. For this example lets say there are only two elements in it(firstName and lastName). I need to get values from myArrayList and compare them to String and if it matches then get value from the bean and put it into map:

        Map<String,String> myMap;
        for(String element: myArrayList){
               if(element.equalsIgnoreCase("firstName")){
                   myMap.put("firstName", bean.getFirstName());
               }else if(element.equalsIgnoreCase("lastName")){
                   myMap.put("lastName", bean.getLastName());
               }
        }

The problem is when you have thirty-forty elements in myArrayList you will have performance issues(I assume), and it just doesn't feel right.

I tried this:

        String name = null;
        String value = null;
        for(int i = 0; i < myArrayList.size(); i++){
            name = myArrayList.get(i);
            value = bean.get(name);
            myMap.put(name, value);
        }

But the line "value = bean.get(name);" is saying that method get(String) is undefined in bean class, indeed we don't have such method in bean class, it has only standard getter and setter methods:

public class Bean implements Serializable {
    private String firstName;
    private String lastName;

    public String getFirstName(){
        return firstName;
    }

    public void setFirstName(String firstName){
        this.firstName = firstName;
    }

    public String getLastName(){
        return lastName;
    }

    public void setLastName(String lastName){
        this.lastName = lastName;
    }

}

Now I am thinking how I could come up with some design pattern that optimizes my logic and doesn't affect performance of the code. Please feel free to ask questions, I will edit if you need more info. Any help is greatly appreciated. Thanks.

Edit: shmosel's answer was pretty good for me, thank you all for your help! Cheers!

30-40 elements won't see much improvement from optimisation, but 1000 elements would. (It's still good to optimize though if it means more elegant code) — 4castle, Apr 26 '16 at 20:20
You seem to be trying to use reflection to get at bean properties. You might refer to http://stackoverflow.com/questions/5856895/java-reflection-beans-property-api, but be aware that using reflection has some performance overhead. If you are using Java 8, you might make a HashMap> with values like ("lastName",Bean::getLastName) to quickly look up a field accessor by name. — Hank D, Apr 26 '16 at 20:33
http://stackoverflow.com/questions/8524011/java-reflection-how-can-i-get-the-all-getter-methods-of-a-java-class-and-invoke — Sleiman Jneidi, Apr 26 '16 at 20:47

score 1 · Answer 1 · answered Apr 26 '16 at 20:34

You can try to use reflection see javaDoc

However, I'll not recomend to use it, until it is really required. Possible, you should refactor you code to avoid having list of fields, you need to get.

If you decide you use reflection, there is ReflectionUtils in springframework

score 1 · Accepted Answer · answered Apr 26 '16 at 20:49

1

@HankD and @Natalia have offered some valid solutions, but another option I don't see mentioned is refactoring Bean to support a get(String) method:

public class Bean implements Serializable {
    private Map<String, String> properties = new HashMap<>();

    public String get(String property) {
        return properties.get(property);
    }

    public void set(String property, String value) {
        properties.put(property, value);
    }

    public String getFirstName(){
        return get("firstName");
    }

    public void setFirstName(String firstName){
        set("firstName", firstName);
    }

    public String getLastName(){
        return get("lastName");
    }

    public void setLastName(String lastName){
        set("lastName", lastName);
    }

}

answered Apr 26 '16 at 20:49

shmosel

49,289
6
73
138

I really like this solution. It's very easy to understand, but it also doesn't enforce what properties people could store, and it feels dirty. Also, I'm not sure if it would be an optimization. – 4castle Apr 26 '16 at 20:54
Use an enum instead of a string for the property field if you want to restrict the set of properties. – Tim Williscroft Apr 26 '16 at 20:59
@4castle, `set()` could be made private to restrict properties. Though my goal was not optimization (and I have my doubts whether that's really OP's goal), I expect that the `get()` would perform better than an if-else chain. A switch might be different. – shmosel Apr 26 '16 at 21:03
@TimWilliscroft the enum would need to wrap a string value or we'd be back to square one. – shmosel Apr 26 '16 at 21:05
I meant that this might be an even less optimal solution. It drags down the speed of all the getters instead of only dragging down the `get`. Granted, optimization really isn't needed, that's why it's no biggie. – 4castle Apr 26 '16 at 21:10
@TimWilliscroft, maybe I need to spell this out, if you use an enum as a key, there's no benefit to OP who wants to look up a property by its name, unless the enum wraps a string value, in which case you could require the enum when setting, but (optionally) only the string when getting. – shmosel Apr 27 '16 at 04:30
@shmosel your answer not only allows me to use less code but I can also get rid of all getter and setter methods as I don't need anymore thanks to your properties map. Appreciate very much! – Foxy Apr 27 '16 at 20:22

4castle · Answer 3 · 2016-04-26T21:26:09.783

Your get(String) method is a really good idea, it just needs to be done right. Here's how I would do it, it's very similar to what you did outside of the Bean, but it allows separation of concerns, which is a good thing.

public String get(String field) {
    switch(field.toLowerCase()) {
        case "firstname":
            return firstName;
        case "lastname":
            return lastName;
        default:
            throw new IllegalArgumentException(field + " is an invalid field name.");
    }
}

I'm using a switch statement here because the Java Docs note that:

The Java compiler generates generally more efficient bytecode from switch statements that use String objects than from chained if-then-else statements.

If you can't change your Bean class, then you should at least use this logic inside your loop instead of your current logic simply for the better speed of switch statements and calling toLowerCase() once instead of using equalsIgnoreCase() multiple times.

thank you very much for your answer but shmosel's answer seems to be better in my case. thanks again. — Foxy, Apr 27 '16 at 20:19

score 0 · Answer 4 · answered Apr 26 '16 at 20:33

This seems like a strange way to do things. As 4castle points out, a few elements are not likely to cause performance problems in itself. Are you experiencing performance problems?

If it was me, I'd do something like this:

public static final String lastNameValue = "lastname";

for(String element: myArrayList){
    if(element != null) element = element.toLowerCase();

    if(lastNameValue.equals(element){
        myMap.put("lastName", bean.getLastName());
    } ....
}

The constants prevent the construction of a new String each time this method is called. You are not checking for a null element. Doing the toLowerCase() once is more efficient than doing it a number of times.

Who's constructing new strings? – shmosel Apr 26 '16 at 20:41 — shmosel, Apr 26 '16 at 20:41

Better Design Pattern?

4 Answers4