CODEWITHSUNDEEP
javaobjectoopdesign-patterns
learner
learner

Reputation: 131

How to avoid instanceOf and dynamic getter check?

This code is from the CardUtility class I am using in my application.

public static boolean areBothColoredAndHaveSameColor(Card c1, Card c2) {
    if (c1 instanceof ColoredCard coloredCard1 && c2 instanceof ColoredCard coloredCard2)
        return coloredCard1.getColor() == coloredCard2.getColor();
    return false;
}

public static boolean areBothNumberedAndHaveSameNumber(Card c1, Card c2) {
    if (c1 instanceof NumberedCard numberedCard1 && c2 instanceof NumberedCard numberedCard2)
        return numberedCard1.getNumber() == numberedCard2.getNumber();
    return false;
}

public static boolean areBothSpecialAndHaveSameSpeciality(Card c1, Card c2) {
    if (c1 instanceof SpecialColoredCard specialColoredCard1 && c2 instanceof SpecialColoredCard specialColoredCard2)
        return specialColoredCard1.getSpeciality() == specialColoredCard2.getSpeciality();
    return false;
}

I can obviously see the code duplication happening here but I am bound by the fact that I can't use an equals check and because of the inheritance hierarchy: NumberedCard and SpecialColoredCard both extend the abstract ColoredCard class. How can I avoid redundancy? Should I make all of these implement a new CardWithKeyProperty interface and refactor these methods to a single doBothHaveTheSameKeyProperty() method? But again I am not sure because ColoredCard is one step higher in the inheritance hierarchy so all of them implementing the same interface doesn't sound right.

Upvotes: 0

Views: 134

Answers (3)

VGR
VGR

Reputation: 44328

Like tgdavies’s solution, I would use a Function argument, but I wouldn’t modify the Card subclasses. (I’m not saying I agree with your inheritance hierarchy, but I don’t have enough information to really question it.)

private static <C extends Card> boolean attributeMatches(
                                            Card c1,
                                            Card c2,
                                            Class<C> cardType,
                                            Function<C, ?> attribute) {

    return cardType.isInstance(c1) && cardType.isInstance(c2)
        && Objects.equals(attribute.apply(c1), attribute.apply(c2));
}

public static boolean sameColor(Card c1, Card c2) {
    return attributeMatches(c1, c2, ColoredCard, ColoredCard::getColor);
}

public static boolean sameNumber(Card c1, Card c2) {
    return attributeMatches(c1, c2, NumberedCard, NumberedCard::getNumber);
}

public static boolean sameSpecialty(Card c1, Card c2) {
    return attributeMatches(c1, c2, SpecialColoredCard, SpecialColoredCard::getSpecialty);
}

Upvotes: 1

tgdavies
tgdavies

Reputation: 11411

  • Put the getters on your base card class, but make their return type Optional<Colour>, Optional<Integer> etc so that cards which don't have that characteristic can return Optional.empty().

Then your code can look like this:

    enum UnoColour {
        RED, GREEN, YELLOW, BLUE
    }
    interface Card {
        Optional<Integer> getNumber();
        Optional<UnoColour> getColour();
    }

    public static boolean areBothColoredAndHaveSameColor(Card c1, Card c2) {
        return compareCards(c1, c2, c -> c.getColour());
    }

    public static boolean areBothNumberedAndHaveSameNumber(Card c1, Card c2) {
        return compareCards(c1, c2, c -> c.getNumber());
    }

    
    public static <T> boolean compareCards(Card a, Card b, Function<Card,Optional<T>> extractor) {
        Optional<T> aValue = extractor.apply(a);
        Optional<T> bValue = extractor.apply(b);
        
        return aValue.isPresent() && bValue.isPresent() && aValue.get().equals(bValue.get());
    }

So compareCards removes the duplicated code.

We can make the code more idiomatic (although perhaps harder to understand if you are not familiar with the features used), by using a method reference instead of an explicit lambda:

        return compareCards(c1, c2, Card::getNumber);

And using map in compareCards:

    public static <T> boolean compareCards(Card a, Card b, Function<Card,Optional<T>> extractor) {
        return extractor.apply(a)
           .flatMap(at -> extractor.apply(b).map(bt -> at.equals(bt)))
           .orElse(false);
    }

You can still use subclasses for each type of card, e.g.:

class ColouredCard implements Card {

        private final Optional<UnoColour> colour;

        ColouredCard(UnoColour colour) {
            this.colour = Optional.of(colour);
        }

        @Override
        public Optional<Integer> getNumber() {
            return Optional.empty();
        }

        @Override
        public Optional<UnoColour> getColour() {
            return colour;
        }
    }

It may be useful to have a static type for a Card to catch errors at compile time -- without seeing your code I don't know how useful that will be:

  ColouredCard c = new ColouredCard(BLUE);
  functionWhichOnlyTakesNumberedCards(c); // <-- helpful compile time error

Upvotes: 3

Martin
Martin

Reputation: 56

like mentioned in a comment, inheritance isn't always the best solution. Modeling this scenario with NumberedCard and ColoredCard is imho not very useful. Because a colored card will always have a number and a numbered card will always have a color, so why separate these concepts.

I would create two classes. One as NormalCard with color and number and the other as SpecialCard. And in an interface Card should exists a method compatibleTo().

just my 2c

Upvotes: 1

Related Questions