7

Hope your day is going well.

I want to make the following composition of two objects with the same inheritance root:

enter image description here

There will be 3 basic operations on the Wallet class: add, getCards and getPaymentCards. getCards should return both Cards and PaymentCards. This is a place where I've faced a problem of how to store these cards in the Wallet:

enter image description here

class Card {}
class BadgeCard extends Card {}
class IdCard extends Card {}

class PaymentCard extends Card {}

class WalletT21 {
    Set<Card> cards;
    Set<PaymentCard> paymentCards;

    void add(Card c) {
        cards.add(c);
        Integer i = 0;
        i = 1;
        i = 1;
        System.out.println(i);
    }

    void add(PaymentCard c) {
        paymentCards.add(c);
    }

    Set<Card> cards() {
        return Stream.concat(cards.stream(), paymentCards.stream())
                     .collect(Collectors.<Card>toSet());
    }

    Set<PaymentCard> paymentCards() {
        return paymentCards;
    }
}
class WalletT22 {
    Set<Card> cards;
    Set<PaymentCard> paymentCards;

    void add(Card c) {
        cards.add(c);
    }

    void add(PaymentCard c) {
        cards.add(c);
        paymentCards.add(c);
    }

    Set<Card> cards() {
        return cards;
    }

    Set<PaymentCard> paymentCards() {
        return paymentCards;
    }
}

All these ideas are, let's say, not perfect. The reason for uniting these classes under the one roof (Wallet) is that the higher code uses cards for identification (every card is an account identifier) and payments (not every card is payable but every paymentcard is an identifier (Card)).

Is there a more elegant way to make such composition?

  • I can't think of one. But whichever way you go with, no code outside the class needs to know about it, so it's not that bad. – immibis Nov 29 '16 at 10:03
  • 1
    What is wrong with WalletT21, where the cards() method returns the union of the two sets used internally? – Bart van Ingen Schenau Nov 29 '16 at 11:35
  • 1
    It is skewed. It would make more sense to me if Card were abstract and Wallet would only contain collections of concrete class types. Then having separate collections for different types would be convenient, they would each serve a different purpose and offer possibly different functionality that is readily accessible without the need for type casting or type checking. You could still implement an enumerator for all cards that gives you just cards to do cardy things with. – Martin Maat Nov 30 '16 at 13:50
  • You are totally right, but the intention was to create a bag for all of cards. I mean, you have 1 wallet in your pocket, not 2 ones: CreditCardWallet, TransitCardWallet etc.... – ovnia Nov 30 '16 at 19:09
4

All cards could be stored in a single Set<Card>, and then in getPaymentCards a filter by type could be applied:

cards.stream()
    .filter(card -> card instanceof PaymentCard)
    .collect(Collectors.toSet());

The problem should probably be modelled differently to avoid this problem entirely, but given these exact requirements this solution might be a cleaner way to handle it.

If a more generic version was needed, this could be implemented like so:

public Set<Card> getCardsOfType(Class<? extends Card> type) {
    return cards.stream()
            .filter(type::isInstance)
            .collect(Collectors.toSet());
}
2

Edit: Thanks to @Laiv and @BenAaronson, here I use generics instead of an actual instance to pass to the filter method. See edit history for the previous version of the code.

Well, I don't like using instanceOf because if would need adding new special cases as new types of cards arise. Also your Wallet class is coupled to concrete implementations.

So here's my two cents:

getCardsByType(Class<? extends Card> cardType)

You pass something like MyIDCard.class to the method to get a subset of all cards of that type . The consumer then can cast the cards to the type he wants if needed. I use interfaces but you could use abstract classes if you like, it would work the same.

Note that only the test class is coupled to any concrete Card implementation, which in a real app should be the factory.

Show me the code (test method at the end):

import java.util.Set;
import java.util.HashSet;
import java.util.Set;

interface Card {}
interface Badge extends Card {}
interface IDCard extends Card {}
interface PaymenCard extends Card {}
class MyIDCard implements IDCard {}
class MyBadge implements Badge {}

interface Wallet {
    public void addCard(Card card);
    public Set<Card> getCards();
    public Set<Card> getCardsByType(Class<? extends Card> cardType);
}

class MyWallet implements Wallet {
    Set<Card> cards = new HashSet<Card>();
    @Override
    public void addCard(Card card) { this.cards.add(card);}
    @Override
    public Set<Card> getCards() { return this.cards;}
    @Override
    public Set<Card> getCardsByType(Class<? extends Card> cardType) {
        Set<Card> subSet = new HashSet<Card>();         
        for (Card card: this.cards){
            if (card.getClass() == cardType){
                subSet.add(card);   
            }           
        }
        return subSet;
    }
}

    public class Test {    
        public static void main (String[] args){
            Card c1 = new MyIDCard();
            Card c2 = new MyBadge();
            Card c3 = new MyBadge();
            Card c4 = new MyBadge();
            Card c5 = new MyIDCard();
            Wallet myWallet = new MyWallet();
            myWallet.addCard(c1);
            myWallet.addCard(c2);
            myWallet.addCard(c3);
            myWallet.addCard(c4);
            myWallet.addCard(c5);
            //now my wallet has two IDs and a Badge
            //
            // I can ask for a list of cards "like this one"
            Set<Card> badges = myWallet.getCardsByType(MyBadge.class); 

            for (Card card: badges){
                System.out.println(card.getClass().getCanonicalName());
            }       
        }
    }

Output showing that the filter by type works:

$ java Test 
MyBadge
MyBadge
MyBadge

Here I use a default package, but no name colission is possible since canonical name of classes contains the package namespace like this:

$ java Test 
com.test.cards.MyBadge
com.test.cards.MyBadge
com.test.cards.MyBadge
  • Well, I'm pretty sure that only PaymentCard will be marked out, so it might be a little bit overkill. – ovnia Nov 29 '16 at 13:06
  • @What do you mean by "marked out"? – Tulains Córdova Nov 29 '16 at 13:09
  • @i.ovchynnikov If you delete the test class with all its comments, the code is shorter than yours. I'll separate the test class in the answer. – Tulains Córdova Nov 29 '16 at 13:18
  • 1
    @TulainsCórdova getCards(Class<? extends Card> cardType) then you can keep using classType.getCannonicalName() – Laiv Nov 29 '16 at 13:29
  • 1
    @Laiv Done. It indeed reflected the previous approach. – Tulains Córdova Nov 29 '16 at 15:15
1

As other people mentioned having them all in a single Set is the best option, however if you absolutely must have 2 separate Sets for whatever reason you can simply have getCards() return a combination of the Sets.

void add(Card c) {
    cards.add(c);
}

void add(PaymentCard c) {
    paymentCards.add(c);
}

Set<Card> cards() {
    Set<Card> combinedSet = new HashSet<>();
    combinedSet.addAll(cards);
    combinedSet.addAll(paymentCards);
    return combinedSet;
}

Set<PaymentCard> paymentCards() {
    return paymentCards;
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.