CODEWITHSUNDEEP
javaoopsoftware-design
Kenny Hammerlund
Kenny Hammerlund

Reputation: 506

Software Design Principle: Seperate your collection from working on an object?

Working with a group in OOP design a group member and I have a disagreement and I can't find the answer on the web.

Creating a piece of software similar to a nonprofit that has an organization with donors and their credit cards.

process diagram

One way is that creditCardList (the collection class) should only store and return the creditCard objects. Then when they are returned the Organization class should run processCard() for each object as a method of the creditCard object.

the other side would be that CreditCardList should process all and contain the loop that runs over all items in the collection.

generally speaking what is the better design of software?

Upvotes: 1

Views: 174

Answers (4)

Robert Bräutigam
Robert Bräutigam

Reputation: 7744

Here is an alternative design idea. Instead of focusing on what steps are necessary to complete your objectives, let's just model the problem first, and use the domain of the problem to come up with names for things.

Obviously there are Organizations, which accepts Donations. Ok let's try this:

public interface Organization {
    void accept(Donation donation);
}

public interface Donation {
}

Now, the money from a donation probably needs to be transferred to the organization at some point in the accept() method, so let's add that as a responsibility of the Donation object:

public interface Donation {
    void transferTo(BankReference target);
}

In the example you process multiple donations in one go. This is the iteration you asked about. This would be in this class:

public final class Donations implements Donation {
    ...
    public Donations(Donation... donations) {
        ...
    }

    @Override
    public void transferTo(BankReference target) {
        donations.forEach(donation -> donation.transferTo(target));
    }
}

Next, you want to have donations that are done using credit cards. You can do this by providing a specific donation type.

public final class CreditCardDonation implements Donation {
    ...
    @Override
    public void transferTo(BankReference target) {
        // Here is the credit card processing logic
    }
}

I obviously don't know all the details of your requirements, but my point is to start with the things you have, not the technical steps (like credit card processing) that need to take place. This applies to the method names also, not just class names!

Upvotes: 1

Andrew
Andrew

Reputation: 49626

I would choose the first option.

Since you are already using the CreditCardList as a storage, the class should not carry any other responsibilities. The functionality of this class is restricted to providing cards (fully/partially, with a condition/by criteria).

The second option rises the problem of coupling a storage with a card processing mechanism. Your repository now depends on the processing system as well as on input parameters that the system may require. In addition, the storage does know about all the processes it shouldn't be aware of.

Upvotes: 1

mbnx
mbnx

Reputation: 942

First, I find that for-each call in your diagram curious: why does the organization call the method CreditCard.processCard and passes the creditCard instance as a parameter?

What does "processing a credit card" mean in the first place? Is the credit card instance able to handle it? The first design decision I would make, is that the CreditCard object should not have dependencies on some external service. Processing a credit card sounds like dealing with something external. I would keep the CreditCard logic pure. Only put logic operating on local fields and properties here.

The CreditCardList has one primary task. Holding a set of credit cards. The way you want to retrieve and store the list of credit cards might change, thus it is a good idea to keep concerns separated and keep processing logic out of that class. One reason to run the loop inside CreditCardList could be, that you do not want to expose the concrete list of credit cards. But this can be handled in a more elegant way. Define an interface ICardProcessor and inject it as a constructor parameter into CreditCardList. The foreach could then remain inside the class and call ICardProcessor.Process(card) on each instance.

Upvotes: 2

ACloudRoamer
ACloudRoamer

Reputation: 1291

Sorry to say, but I would choose neither of these two approaches for separation of duty principle. If I understood the requirement correctly, the Organization object is an entity representing the charity organization that is accepting the donations. And the CreditCardList is a collection of all credit cards being used for the payment processing. Neither of them is a good place to hold the payment processing logic.

Rather, I would introduce a new helper class with a name something like PaymentProcessor. This class would have a method called processPayment(List<CreditCard> creditCards). This method should contain the logic of processing the payment from the credit cards. With this design, you may also introduce other methods in this class to process payments using other ways like direct debit from the bank account or PayPal.

As per OO design principles, we should have one class or interface for just one purpose for better maintainability.

I hope this might help resolve your constructive differences with your colleague. :)

Upvotes: 2

Related Questions