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) — castletheperson
– castletheperson, Commented Apr 26, 2016 at 20:20
You seem to be trying to use reflection to get at bean properties. You might refer to stackoverflow.com/questions/5856895/…, but be aware that using reflection has some performance overhead. If you are using Java 8, you might make a HashMap<String,Function<Bean,String>> with values like ("lastName",Bean::getLastName) to quickly look up a field accessor by name. — Hank D
– Hank D, Commented Apr 26, 2016 at 20:33

shmosel · Accepted Answer · 2016-04-26 20:49:57Z

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, 2016 at 20:49

shmosel

51.1k7 gold badges82 silver badges147 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

castletheperson Over a year ago

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.

Tim Williscroft Over a year ago

Use an enum instead of a string for the property field if you want to restrict the set of properties.

shmosel Over a year ago

@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 Over a year ago

@TimWilliscroft the enum would need to wrap a string value or we'd be back to square one.

castletheperson Over a year ago

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.

|

Natalia · Accepted Answer · 2016-04-26 20:34:59Z

1

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

answered Apr 26, 2016 at 20:34

Natalia

4,5322 gold badges27 silver badges28 bronze badges

Comments

castletheperson · Accepted Answer · 2016-04-26 21:26:09Z

1

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.

edited Apr 26, 2016 at 21:26

answered Apr 26, 2016 at 21:01

castletheperson

33.7k11 gold badges74 silver badges111 bronze badges

1 Comment

Foxy Over a year ago

thank you very much for your answer but shmosel's answer seems to be better in my case. thanks again.

Pete B. · Accepted Answer · 2016-04-26 20:33:34Z

0

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.

answered Apr 26, 2016 at 20:33

Pete B.

3,3066 gold badges27 silver badges41 bronze badges

1 Comment

shmosel Over a year ago

Who's constructing new strings?

Collectives™ on Stack Overflow

Better Design Pattern?

4 Answers 4

7 Comments

Comments

1 Comment

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

7 Comments

Comments

1 Comment

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related