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.

CodePudding user response：

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

CodePudding user response：

• 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