Best Practice to avoid using 'ifs' for calling methods

Question

I have legacy code, similar to this

private Resource getResource(String type){
    Resource resource = null;       

    if("A".equals(type)){
        resource = Utils.getResourceA(); //static func      
    }else if("B".equals(type)){
        resource = Utils.getResourceB(); //static func      
    }else if("C".equals(type)){
        resource = Utils.getResourceC(); //static func      
    }else if // etc..

}

As you can see it's something that will be hard to maintain for new types...

What is the best practice to solve that?

I was thinking of creating Class for each resource function that will implement same interface.

Then create a Map<String,IResource> key is the type and IResource will be the instance of class.

But there are problems with this solution.

Where to create this map? inside getResource() method? inside class that holds this method?
I will be holding instances of classes in memory in cases where I won't be using many of them
Instead of simple static method, I will now have a full object (for each method) !

A note: I am not using any frameworks, pure java.

Thanks

I'm voting to close this question as off-topic because it should be migrated to Programmers.SE. — Laf, Oct 08 '15 at 18:36
While this is a good question, I don't think it belongs on StackOverflow. Programmers.SE would be a better place to ask for such kind of problems. You might have to edit your question so that it follows their guidelines though. — Laf, Oct 08 '15 at 18:37
Have a look at: http://stackoverflow.com/questions/32229528/inheritance-aware-class-to-value-map-to-replace-series-of-instanceof/32229931#32229931 — Ravindra babu, Oct 08 '15 at 18:40
Have a look at Visitor pattern. Depending on what your methods do, it may make sense for your use-case. — kha, Oct 08 '15 at 18:45
I had completed an answer using reflection which needed no map, ifs or switches. Just add a new getResourceTYPE method for each new TYPE you have. So no chance for bugs caused by forgetting a map.put line, or an if or case in a switch. But can't post since question is now on hold, if you reword your question so it can be reopened or you repost it at programmers.SE I'll post it. — Anonymous Coward, Oct 08 '15 at 20:48

JB Nizet · Accepted Answer · 2015-10-08T19:30:13.277

4

The usual solution is indeed the one you describe. Example using Java 8 (but that could be written, in a more verbose way, in Java7):

private Map<String, Supplier<Resource>> resourceSuppliers;

public TheClass() {
    resourceSuppliers = new HashMap<>();
    resourceSuppliers.put("A", Utils::getResourceA);
    resourceSuppliers.put("B", Utils::getResourceB);
    resourceSuppliers.put("C", Utils::getResourceC);
    ...
}

private Resource getResource(String type){
    return resourceSuppliers.get(type).get();
}

The map could also be static final and shared by all instance of TheClass, if the type/resource mapping is the same for all instances:

private static final Map<String, Supplier<Resource>> RESOURCE_SUPPLIERS = createResourceSuppliers();

private static Map<String, Supplier<Resource>> createResourceSuppliers() {
    // same code as in constructor above
}

In older Java versions, you would have to define the Supplier interface, and to define the suppliers by creating them in a more explicit way, for example, using an anonymous inner class:

resourceSuppliers.put("A", new Supplier<Resource>() {
    @Override
    public Resource get() {
        return Utils.getResourceA();
    }
});

edited Oct 08 '15 at 19:30

answered Oct 08 '15 at 18:42

JB Nizet

678,734
91
1,224
1,255

a question: value of the map, in java 6, as I can't store pointers to methods, I will have to create classes for methods? – veg Oct 08 '15 at 18:47
See my edited answer. Tha Java 7 (or 6) version is much more verbose, and using a switch statement is shorter. Both avoid being O(n) as your original code is. – JB Nizet Oct 08 '15 at 18:50
Instance init would made the instantiation code shorter. Besides that, it is important to mind the potential NullPonterException that would be thrown if client code asks for a unknown/invalid resource type. – PEdroArthur Oct 08 '15 at 18:52
JB Nizet, thanks, and last one: for your opinion, this "overhead" is better than multiple ifs? – veg Oct 08 '15 at 18:52
This still doesn't fix the underlying problem of failure for unknown types or ensuring/enforcing implementation of new `getResource` when a new type is introduced. Converting `type` to an `enum` will fix both of those issues too. – Andreas Oct 08 '15 at 18:58
@Cinnam: fixed, thanks. – JB Nizet Oct 08 '15 at 19:31
@Andreas Using an enum is a perfectly valid solution. But if the input is indeed a String and that can't change, using an enum doesn't really solve anything: you still have to remember to add an additional enum if an additional string type is introduced. But I agree that an enum should be preferred over a string to represent a fixed set of types. – JB Nizet Oct 08 '15 at 19:35

Andreas · Answer 2 · 2015-10-08T21:39:05.393

The best way it to turn type into an enum. This will prevent arbitrary types and requires implementing the getResource() method for any new type.

public enum ResourceType {
    A {
        @Override
        public Resource getResource() {
            return Utils.getResourceA();
        }
    },
    B {
        @Override
        public Resource getResource() {
            return Utils.getResourceB();
        }
    },
    C {
        @Override
        public Resource getResource() {
            return Utils.getResourceC();
        }
    };
    public abstract Resource getResource();
}

Update

Converting String to enum is easy (built-in):

ResourceType type = ResourceType.valueOf("A");

Problem with this what I will need to handle conversion of type String to type enum. — veg, Oct 08 '15 at 19:28

Best Practice to avoid using 'ifs' for calling methods

2 Answers2