Does this method belong to Value object or Manager

Question

In an e-commerce application, below are the high level API

interface Order{
    public List<PaymentGroup> getPaymentGroups(); 
}

interface PaymentGroup{}

class PaymentGroupImpl implements PaymentGroup{}

class CreditCard extends PaymentGroupImpl{}

class GiftCard extends PaymentGroupImpl{}

class OrderManager{ //Manager component used to manipulate Order}

There is a need to add some utility methods like hasGiftCard(), hasCreditCard(), getGiftCards(), getCreditCards()

Two approaches - 1) Add these in Order. However, this would result in coupling between Order and PaymentGroup implementors (like CreditCard, GiftCard) Example -

interface Order {
         public List<GiftCard>  getGiftCards();
}

2) Move these to OrderManager.

class OrderManager{
   public List<GiftCard> getGiftCards(Order order){}
}

I personally prefer 2), am just curious would there be any reason to choose 1) over 2)

Answer 1

I have two answers. One is what I'll call Old Skool OOP and the other I'll call New Skool OOP.

Let's tackle New Skool first. The GoF and Martin Fowler changed the way people look at OOP. Adding methods like hasGiftCard() leads to adding conditional logic/branching into the code. It might look something like this:

if (order.hasGiftCard()) {
    //Do gift card stuff
} else {
    //Do something else
}

Eventually this kind of code becomes brittle. On a big application, lots of developers will be writing predicate methods. Predicate methods assert something and return true or false. These methods usually start with the word "has", "is" or "contains". For example, isValid(), hasAddress(), or containsFood(). Still more developers write conditional logic that uses those predicate methods.

To avoid all of this conditional logic software engineers changed how they thought about object-orientation. Instead of predicate-methods-and-conditional-logic, they started using things like the strategy pattern, visitor pattern, and dependency injection. An example from your problem domain might look like this:

//Old Skool
if (this.hasCreditCard()) {
    orderManager.processCreditCard(this.getCreditCards());
}

Here is another approach to solving the same problem:

//New Skool
for(PaymentItem each : getPaymentItems()){
    each.process(this);
}

The New Skool approach turns the problem on its head. Instead of making the Order and OrderManager responsible for the heavy lifting the work is pushed out to the subordinate objects. These kind of patterns are slick because:

1. they eliminate a lot of "if" statements,
2. the code is more supple and it is easier to extend the application, and
3. instead of every developer making changes to Order and OrderManager, the work is spread out among more classes nd code merges are easier.

That's New Skool. Back in the day, I wrote a lot of Old Skool object-oriented code. If you want to go that route, here are my recommendations.

• IMHO, you don't need both a PaymentGroup interface and a PaymentGroupImpl class. If all payment classes extend PaymentGroupImpl, then get rid of the interface and make PaymentGroup a class.
• Add methods like isCreditCard(), isGiftCertificate() to the PaymentGroup class. Have them all return "false".
• In the subclasses of PaymentGroup, override these methods to return true where appropriate. For example, in the CreditCard class, isCreditCard() should return "true".
• In the Order class, create methods to filter the payments by type. Create methods like getCreditCards(), getGiftCertificates(), and so on. In traditional Java (no lambdas or helper libraries), these methods might look something like this

List getCreditCards() {     
    List list = new ArrayList();
    for(PaymentGroup each : getPaymentGroups()){
        if(each.isCreditCard()) {
            list.add(each);
    }
    return list;
}

-In the Order class, create predicate methods like hasCreditCards(). If performance is not an issue, do this:

boolean hasCreditCards() {
    return !getCreditCards().isEmpty();
    }

If performance is an issue, do something more clever:

boolean hasCreditCards() {
    for(PaymentGroup each : getPaymentGroups()){
        if(each.isCreditCard()) {
            return true;
        }
    return false;
    }
}

Realize that if you add a new payment group, a code must be added in a lot of places in the Old Skool paradigm.