Interface Hell or Acceptable Design?

Question

I am refactoring some legacy code that was doing close to the same thing over and over via a case statement:

switch(identifier)
    case firstIdentifier: 
        (SomeCast).SetProperties(Prop1,Prop2,Prop3);
        break;
    ...
    case anotherIdentifier:
        (SomeDifferentCast).SetProperties(Prop1, Prop2, Prop3);
        break;

So, I tried to create a unique interface so that this could become

(SameInterfaceCast).SetProperties(Prop1,Prop2,Prop3);

However, I then found that some items do not even use all of the properties. So, I began to think of something more like this:

if(item is InterfaceForProp1)
    (InterfaceForProp1).SetProp1(Prop1);
if(item is InterfaceForProp2)
    (InterfaceForProp2).SetProp2(Prop2);
if(item is InterfaceForProp3)
    (InterfaceForProp3).SetProp3(Prop3);

and you could create a class like this:

public class MyClassUsesProp2And3 : InterfaceForProp2, InterfaceForProp3

However, I am afraid that I am overly fragmenting this code and it could balloon too much. Maybe I should not be too fearful of what will essentially be one method interfaces, but I wanted to see if I am missing a design pattern before going down this path? (the only ones that popped into my mind but were not quite the right fit were the Decorator or Composite patterns)

UPDATE

All properties are unique types.

Ultimately, this is a form of dependency injection. The code is too messed up to use something like Ninject right now, but eventually I might even be able to get rid of some of these and use an injection container. There is also currently some logic that is being done beyond just setting a variable. This is all legacy code, and I am just trying to clean it up little by little.

Answer 1

I don't know if there is a "right" answer to this but here's what I would do.

class Properties {
    prop1
    prop2
    prop3
}

interface PropertySetable {
     setProperties(Properties prop);
}
public class MyClassUsesProp2And3 implements PropertySetable {
    setProperties(Properties prop) {
       //I know I need only 2 and 3
       myProp2 = prop.prop2;
       myProp3 = prop.prop3;
    }

}

In calling function, you should not have a cast.

 someFunc(..., PropertySetable, Properties,...) {
      PropertySetable.setProperties(Properties); 
 } 

That is the basic structure.

You should encapsulate Properties - make properties private and have relevant constructors. Or use the Builder pattern to build Properties... and many more..

Answer 2

I think the answer depends on why you want to refactor the code in the first place.

• If you want to get rid of the switch block entirely then you'll have to implement an interface with one method taking three arguments. Each class would have to figure out which arguments apply and what to do with those arguments.
• If you want to reduce the overall amount of code then I'm not sure that adding the class-specific property setting logic is any less code than the original switch statement
• If you want to exploit your editor's code completion capability to insert the correct method with the correct number of properties/args then just implement the correct method(s) on each class and avoid the interface altogether

Answer 3

The visitor pattern is the standard solution when you are faced with switch statements and typecasts. Your code for each case will go into a separate method in the visitor class. And your classes will each implement only one interface -- an accept method to accept visitors. You will end up with more code than you have now but it will read a lot cleaner.

Answer 4

Basically you want to make the actors (type with setProp methods) the same type 'Actor' AND make the properties (prop1...n) the same type 'Prop'. This would reduce your code to

actor.setProp(prop)

If you want to avoid using instanceOf, the only way I can think of is to go with the Visitor pattern, making 'Prop' the visitor. I would also use template method to make my life easier. In Java I would make it look like this (for two actual kinds of Prop).

class Actor {

    protected void set(Prop1 p1) {
        // Template method, do nothing
    }

    protected void set(Prop2 p2) {
        // Template method, do nothing
    }

    public void setProp(Prop p) {
        p.visit(this);
    }

    public interface Prop {
        void visit(Actor a);
    }

    public static Prop makeComposite(final Prop...props ) {
        return new Prop() {

            @Override
            public void visit(final Actor a) {
                for (final Prop p : props) {
                    p.visit(a);
                }
            }
        };
    }

    public static class Prop1 implements Prop {
        public void visit(Actor a) {
            a.set(this);
        }
    }

    public static class Prop2 implements Prop {
        public void visit(Actor a) {
            a.set(this);
        }
    }
}

This allows you to do stuff like this:

    ConcreteActor a = new ConcreteActor();
    Prop p = Actor.makeComposite(new ConcreteProp1(42), new ConcreteProp2(-5));
    a.setProp(p);

...which is super nice!