Reputation: 19854

Possible design pattern instead of instanceof?

I am writing an object conversion class, used to convert domain layer objects into UI objects and vice versa. The problem is that my UI objects are organized into a hierarchy and as a result my object conversion class contains "instanceof" statements. There is a definite code smell here but I'm not sure what the solution is.

So my UI hierarchy contains a RuleDTO as follows:

public class RuleDTO {
    protected long ruleID;
    protected long rowID;   
    protected AttributeDTO leftCondition;
    protected AttributeDTO rightCondition;
    protected OperationTypeDTO operationType; 
    protected boolean isActive;     
    // etc...
}

My RuleDTO can then be subclassed by AssignmentRuleDTO as follows:

public class AssignmentRuleDTO extends RuleDTO {
    protected String assignedToTeam;
    protected String assignmentOperator;        
    // etc...
}

RuleDTO can also be subclassed by EvaluationRuleDTO:

public class EvaluationRuleDTO extends RuleDTO {
    protected String successAction;
    protected String failureAction;
    // etc...
}

The problem is reached then in my ObjectConversionHelper class which contains the following type of logic:

{
    // Perform logic common to RuleDTO such as setting ruleID, isActive etc
    if(ruleDTO instanceof AssignmentRuleDTO) {
        // Set assignedToTeam and assignmentOperator etc
    }
    else if (ruleDTO instanceOf EvaluationRuleDTO) {
        // Set successAction and failureAction etc  
    }
}

What would be a good solution here instead? I've read about the visitor pattern, but not sure how it applies here.

Thanks

Upvotes: 3

Answers (7)

Kuba Wyrostek

Reputation: 6221

How about creating a single ObjectConversionHelper class for each DTO class? Each of them could implement a common conversion interface differently, call inherited members etc. You could then make use of some object creation factory that would create relevant Helper for DTO and vice-versa (i.e. using reflection mechanizms).

Upvotes: 0

Michael

Reputation: 4886

I think using a Visitor pattern would be a reasonable approach here. So you'd have the Visitor interface

interface RuleDTO {
    void visit(RuleDTO theRule);
    void visit(EvaluationRuleDTO theEval);
    void visit(AssignmentRuleDTO theAssign);
    ... and so on ...
}

And you'd add a method to these concrete classes to handle the double dispatch

public void accept(RuleDTOVisitor theVisitor) {
    theVisitor.visit(this);
}

Lastly, you'd create some class which implements the visitor, say SettingPropertiesVisitor, and for each method, you can do the implementation where the appropriate fields for each object are set accordingly to your application requirements.

So then to use it

aRuleDTO.accept(new SettingPropertiesVisitor());

This way the appropriate visitor method will get invoked for each type, and then within the methods for your SettingPropertiesVisitor, you can do the appropriate assignments. This will get around the instanceof checks, and decouples that setter logic from the objects.

Of course, that might be overkill if this is the only visitor you ever create, in that case, instanceof isn't like killing kittens. But the obvious drawback here is each time you extend the API, you need to modify the visitor interface, and then probably all the concrete visitors to support the new method.

Upvotes: 3

davidfrancis

Reputation: 3849

Visitor looks like overkill here IMHO.

There is no iteration over a graph of objects.
There is no requirement for double dispatch.

Remember KISS and YAGNI. Just add an abstract method or leave it as-is.
You can always refactor later - assuming you have tests in place ;)

Upvotes: 2

asteri

Reputation: 11572

There are a couple of plausible approaches. The two I would consider are using either an interface which all of your classes implement or using an enum that corresponds to your different classes.

If you have an interface (let's call it public interface DTO, you can have a method signature in it called setFields() or something similar which each of the implementing classes must implement. Then, through the magic of polymorphism, you can now treat all of your objects as DTO using typecasting and call setFields() on them without worrying what the actual object is. The setFields() method in each of the individual classes will take care of it for you.

Alternatively, you can make an enum that is essentially an ID for each of your classes and make each class have a global variable of that type (complete with getters and setters) in order to identify it. This is a somewhat "hacky" workaround but still a doable one.

Upvotes: 0

algorowara

Reputation: 1720

In your case, the visitor pattern could be applied by writing a convertToOtherClass method in RuleDTO, which is then overridden by its subclasses. You would then have, in your object conversion class, a method along the lines of convertRuleDTO (called in RuleDTO's convertToOtherClass method), which executes the relevant code, secure in the knowledge that it is operating on an instance of RuleDTO which has not been subclasses, because otherwise the subclass would override the convertToOtherClass method.

Upvotes: 1