Separating business logic from data tempts me to use instanceof

Question

class Foo {
    abstract doStuff() {}
}

class Bar extends Foo {
    doStuff() { ... }
}

class Baz extends Foo {
    doStuff() { ... }
}

From a functional perspective doStuff does exactly the same thing, however if it should be done to bar or baz then the implementation is completely different.

I try to separate data from business logic, so as doStuff is business logic and not class behavior, it should be outside.

class Foo {...} // accessors inside
class Bar extends Foo {...} // accessors inside
class Baz extends Foo {...} // accessors inside

class Manager {
    doStuff(foo) {
        if(foo instanceof Bar) {
            return this.doBarStuff(foo);
        }
        if(foo instanceof Baz) {
            return this.doBazStuff(foo);
        }
        throw;
    }
    doBarStuff(bar) { ... }
    doBazStuff(baz) { ... }
}

Usage of instanceof has always been considered as a bad pattern for me and I always try to use polymorphism instead. However by separating business logic from data, I found myself stuck.

I could use a kind of accessor getType() and use a switch to avoid the usage of instanceof, but for me it is nearly the same issue as it seems to not use all polymorphism benefits. At the moment I have only 2 child classes and it should not be extended in the near future. So for now, I prefer to use instanceof instead of an accessor. People that read the child class will ask "What is it ?" about getType() because it is not related to data but it is a "flag" used by business logic switch.

By using a trait or composition to write business logic, business logic will be write in different files but just make it more difficult to read IMO.

What would you advise? Is there some other way I have not considered to fix it?

Answer 1

Your problem is that you're trying to use both rich domain models

I always try to use polymorphism

and anaemic domain models

I try to separate data from business logic

at the same time. Both are valid programming patterns, but they take fundamentally different approaches as to whether the business logic code sits in the domain objects or not - you can't mix and match the two. Pick one or the other and stick to it.

Answer 2

Though Philip Kendall's answer has a point, I think it is oversimplifying. One can separate data from behaviour and still use object oriented means and polymorphism.

If you need your data (Bar, Baz) separated from your business logic, maybe for some technical reasons, you can keep Bar and Bazas pure DTOs and introduce separate classes for the business logic. Let's call them BarProcessor and BazProcessor which both derive from some FooProcessor with a common entry point like doStuff(). Both classes can hold a data object the related type inside - passed by the constructor - and provide individual, polymorphic behaviour for each type.

Now, assumed you get a Foo object as input, you still face the problem to instantiate a BarProcessor for a Bar object, a BazProcessor for a Baz object, and so on. For this, you indeed may use a switch statement inside some Factory class. The point is that with a factory, you need the switch only once - the factory will encapsulate the mechanics in one central place, and it decouples it from the actual logic which may happen in your Manager class calling FooProcessor.dostuff().

(Alternatively, I am pretty sure one could also utilize a DI container instead of a factory to instantiate the related FooProcessor for a given Foo object).

Answer 3

Is there some other way I have not considered to fix it?

Double dispatch is one way to address this sort of problem. This is a way to apply inversion of control to get your polymorphic objects to trigger behavior appropriate to their classes in other objects. It is also known as the Visitor pattern. As applied to the problem posed, it might look like this:

public interface FooVisitor {
    default void visit(Foo foo) { /* empty */ }
    default void visit(Bar bar) { /* empty */ }
    default void visit(Baz baz) { /* empty */ }
}

public class Foo {
    void accept(FooVisitor v) {
        v.visit(this);
    }
}

public class Bar extends Foo {
    void accept(FooVisitor v) {
        v.visit(this);
    }
}

public class Baz extends Foo {
    void accept(FooVisitor v) {
        v.visit(this);
    }
}

public class StuffDoer implements FooVisitor {

    public void visit(Bar bar) {
        // do Bar stuff ...
    }

    public void visit(Baz baz) {
        // do Baz stuff ...
    }
}

class Manager {
    public void doStuff(Foo foo) {
        StuffDoer doer = new StuffDoer();

        foo.accept(doer);
    }
}

Note that although they are lexically identical, the three accept() methods are semantically different because each one will be bound (at compile time) to a different overload of FooVisitor.visit(). This is in fact the key to the pattern.

It may seem a bit wordy, but remember that you can define as many FooVisitor implementations as you like for tasks that need to distinguish among your polymorphic objects by their actual class.

Answer 4

I try to separate data from business logic....

That's your problem right there: you appear to be taking this as a primary goal rather than as a technique for making your code more clear and easier to maintain.

Experienced developers treat things such as "separate data from business logic" as guidelines, not as hard-and-fast rules. They are not applied blindly, but applied because (and only when) they achieve better code. Every time you apply guideline such as this, you should be able to say not just, "I separated the business logic from the code," but "I separated the business logic from the code in this situation because it made the code better than the alternative in the following way: ...."

You need to look at your particular situation and imagine how it would look and how maintainable it would be if you used technique X. If you're finding it difficult to imagine, you might even try it both ways. That can be done in the short term (sketch it out both ways and commit the one you find better now) or even in the long term (use a certain design now, and weeks or even months later try changing your approach if the current approach is not working out). Since the result is dependent on the context of the code, which changes over time, it may even be the case that when you start one technique works better but as that area of code changes and expands, or the code in other connected parts of the system changes, the best technique to use there changes. (This is one of the reasons we refactor code.)

Note that trying it both ways may even remain embedded in the production code for some time. It's not unusual for me to have a chunk of code mostly done in one way but where I've introduced a change in technique for a part of it to see how it works out. This can take some time and require examination by and opinions from other developers on the team; as we move forward and better understand the implications and effects that technique we'll either start refactoring other code towards that design or reverse course and bring the new code back to the original design. (Many of my attempts to improve code start with an incomplete move towards a different design precisely so that I don't spend too much effort before being able to evaluate the results, and so that if it turns out not to be going well I don't need to spend too much effort reversing course.)

If you have any doubts about whether or not separating the business logic is a good idea in your case, try it. There's no problem having "experiments" in your code so long as everybody knows that those are experiments and why they're being done. This should be explained in comments in the code and also mentioned in the commit message. Writing out a concise explanation of what problem you're wanting to solve, what you're trying in order to solve it, and the benefits and problems the new design introduces will not only help other developers understand what's going on, but will help you better understand what's going on there.

This "try it out" technique does assume that you can fairly easily change the code if what you're currently doing does not work out as well as you'd hoped. That generally presupposes that you have support for this in the form of being able easily to write and run tests and so on. If you don't yet have that, you should probably focus some effort on doing that first, especially since this can significantly affect the design of the code.

Answer 5

Like Dave points out in the comments, I believe your question is too abstract to be answered. I took the liberty turn it into a concrete (but still simplified) example.

class Pet {...} // accessors inside
class Cat extends Pet {...} // accessors inside
class Hamster extends Pet {...} // accessors inside

class PetFeeder {
    void feedPet(Pet pet) {
        if(pet instanceof Cat) {
            this.feedChicken(pet);
        }
        if(pet instanceof Hamster) {
            this.feedCarrots(pet);
        }
        throw;
    }
    void feedChicken(Pet) { ... }
    void feedCarrots(Pet) { ... }
}

So what is the problem with this code? It is tightly coupled; whenever you want to add a new type of pet you also have to adjust PetFeeder, even for tiny changes like changing the name of Cat to HouseCat. Instead, you want to have the model contain the differences that make you treat your Bar and Baz differently.

In our example this would be by adding a isCarnivore method to the Pet super class.

class Pet { abstract boolean isCarnivore(); }
class Cat extends Pet { boolean isCarnivore() { return true;} }
class Hamster extends Pet { boolean isCarnivor() { return false;} }

class PetFeeder {
    void feedPet(Pet pet) {
        if (pet.isCarnivore()) {
            this.feedChicken(pet);
        } else {
            this.feedCarrots(pet);
        }
    }
    void feedChicken(Pet) { ... }
    void feedCarrots(Pet) { ... }
}

If you would now add a new class Bunny, you can do it all in one place, let isCarnivore return true and be done with it.

In this design there is no automatic throw if a new class isn't known about. This is a feature and helps you to decouple business logic. But the drawback is that you wouldn't know if your new Pet is a carnivore, but only eats live mice.

If you need to be sure that your pets are not starving, you would want to keep the coupling of your previous design and look into the visitor pattern to get rid of the instanceof.

Answer 6

Use function overloading to only determine the subclass of Foo when you only have a Foo and don't know if it is a Bar or Baz, while allowing the caller to skip that step if they already know what it is.

class Manager {
    doStuff(Foo foo) {
        if(foo instanceof Bar) {
            return this.doStuff((Bar)foo);
        } else if(foo instanceof Baz) {
            return this.doStuff((Baz)foo);
        }
        throw;
    }
    doStuff(Bar bar) { ... }
    doStuff(Baz baz) { ... }
}

This is a bit of a kludge and not elegant (not to mention almost identical to your original example), but if you must keep data separate from business logic it would work while still allowing the caller to skip the unnecessary steps if they can.

You're still trying to adhere to potentially conflicting principles, but this should do what you asked for when you already know if it is a Bar or Baz.