7 Refactoring

Inhalt

Code Smells
2 Refactorings

Code Smells

Jeweils ein Code-Beispiel zu zwei Code Smells aus der Vorlesung; jeweils Code-Beispiel und einen möglichen Lösungsweg bzw. den genommen Lösungsweg beschreiben (inkl. (Pseudo-)Code)]

“If it stinks, change it.”

Mithilfe von SonarCloud wird der Code kontinuierlich auf Code Smells untersucht und diese gemeldet. Zwei davon werden hier mit dem jeweiligen Lösungsweg zum Beheben vorgestellt.

Code Smell 1: Long Method

Die Methode void makeMove() des GamePlayers (dieser hieß bei den angegebenen Commits nur Player), wurde mit der Zeit immer länger, da sie sowohl die Logik für Aktionsphase, Kaufphase und Aufräumphase umfasst. Dadurch wurde die Methode immer unhandlicher. Da Code mehr gelesen als geschrieben wird, sollte auf eine gute Lesbarkeit geachtet werden, die bei langen Methoden nicht gewährleistet ist. Außerdem ist der “Cognitive load” dann beim Lesen des Codes geringer, wenn die Methode kürzer ist und ihre zentrale Aufgabe besser kapselt. Eine gute Erklärung für dieses Prinzip findet sich hier.

Als Lösung wurde entsprechend die lange Methode in kleinere, deutlich aussagekräftigere Methoden aufgetrennt (Refactoring: Extract Method). Vor Commit 712ff sah die Methode makeMove() so aus:

@Override
public void makeMove() {
    Move move = new Move();

    // 1st PHASE - Action phase
    // player MAY play as many action card
    while (move.getActionsCount() > 0 && playerDecision.checkWantToPlayActionCard()) {
        move.looseAction();
        // Choose action card to play
        ActionCard actionCard = playerDecision.chooseActionCard(hand);
        // Execute all instructions of action card
        List<Instruction> instructions = actionCard.getAction().getInstructions();
        instructions.forEach((i) -> i.execute(this, move, this.playerDecision, GameState.stock));
    }

    // 2nd PHASE - Buy phase
    // player MAY buy cards according to money available
    // TODO: check whether there are cards that can be bought for just 1 "money"
    List<Card> buyableCards = GameState.stock.getAvailableCardsWithMaxCosts(move.getMoney());
    if (!buyableCards.isEmpty()) {
        while (!buyableCards.isEmpty() && move.getMoney() >= 1 &&
                playerDecision.checkWantToBuy()) {
            List<Card> boughtCards = new ArrayList<>();
            playerDecision.informAboutBuyableCards(buyableCards);
            Card boughtCard = playerDecision.chooseCard(buyableCards);
            boughtCards.add(boughtCard);

            move.looseBuying();
            move.looseMoney(boughtCard.getCost());

            buyableCards = GameState.stock.getAvailableCardsWithMaxCosts(move.getMoney());
        }
    }

    // 3rd PHASE - "Clean up" phase
    // player MUST put all hand & table cards to the discard deck
    hand.forEach(h -> discardDeck.put(h));
    hand = new ArrayList<>();

    // player MUST draw 5 new cards for the next move
    for (int i = 0; i < 5; i++) {
        Card card = drawDeck.draw();
        hand.add(card);
    }
}

Nach dem Auftrennen hat sich folgendes, deutlich aufgeräumteres Bild ergeben:

@Override
public void makeMove() {
    Move move = new Move();
    doActionPhase(move);
    doBuyPhase(move);
    doCleanUpPhase(move);
}

/**
 * 1st PHASE - Action phase
 * Player MAY play as many action card as he/she wants.
 *
 * @param move
 */
private void doActionPhase(Move move) {
    while (move.getActionsCount() > 0 && playerDecision.checkWantToPlayActionCard()) {
        move.looseAction();
        // Choose action card to play
        ActionCard actionCard = playerDecision.chooseActionCard(hand);
        // Execute all instructions of action card
        List<Instruction> instructions = actionCard.getAction().getInstructions();
        instructions.forEach((i) -> i.execute(this, move, this.playerDecision, GameState.stock));
    }
}

/**
 * 2nd PHASE - Buy phase
 * Player MAY buy cards according to his/her available money.
 *
 * @param move
 */
private void doBuyPhase(Move move) {
    // TODO: check whether there are cards that can be bought for just 1 "money"
    List<Card> buyableCards = GameState.stock.getAvailableCardsWithMaxCosts(move.getMoney());
    if (!buyableCards.isEmpty()) {
        while (!buyableCards.isEmpty() && move.getMoney() >= 1 &&
                playerDecision.checkWantToBuy()) {
            List<Card> boughtCards = new ArrayList<>();
            playerDecision.informAboutBuyableCards(buyableCards);
            Card boughtCard = playerDecision.chooseCard(buyableCards);
            boughtCards.add(boughtCard);

            move.looseBuying();
            move.looseMoney(boughtCard.getCost());

            buyableCards = GameState.stock.getAvailableCardsWithMaxCosts(move.getMoney());
        }
    }
}

/**
 * 3rd PHASE - "Clean up" phase
 * Player MUST put all hand & table cards to the discard deck.
 * Player MUST draw 5 new cards for the next move.
 *
 * @param move
 */
private void doCleanUpPhase(Move move) {
    hand.forEach(h -> discardDeck.put(h));
    hand = new ArrayList<>();

    for (int i = 0; i < 5; i++) {
        Card card = drawDeck.draw();
        hand.add(card);
    }
}

Gleichzeitig konnte dadurch der Code Smell “Code Comments” behoben werden. Die Kommentare haben einen guten “Auftrennpunkt” (Code Seams) für das Aufspalten in einzelne Methoden geboten und wurden nun in die Java Doc Strings aufgenommen. Zudem boten die Kommentare bereits Namen für die neuen Methoden wie doCleanUpPhase. Kommentare haben dementsprechend durchaus ihre Berechtigung, um für weniger Cognitive Load zu sorgen und besser stellen zu identifizieren, die aufgetrennt werden können.

Code Smell 2: Large Class

Das Interface PlayerDecision hat mit der Zeit immer mehr Methoden bekommen, die entsprechend alle in einer einzigen Klasse implementiert wurden. Vor Commit 1cfef sah das Interface entsprechend so aus:

public interface PlayerDecision {

    Card chooseCard(List<Card> cards);
    void informStartActionPhase();
    void informStartBuyingPhase();
    void informNoActionCardsPlayable();
    void informNoCardsBuyableWithMoney(int money);
    void announceResults(List<PlayerResult> results);
    void announceWinners(String... names);
    void informYourTurn(String name);
    List<Card> chooseCards(List<Card> cards);
    ActionCard chooseActionCard(List<ActionCard> cards);
    Optional<ActionCard> chooseOptionalActionCard(List<ActionCard> cards);
    MoneyCard chooseMoneyCard(List<MoneyCard> cards);
    List<Card> chooseCardsToBuy(List<Card> cards);
    Optional<Card> chooseOptionalCardToBuy(List<Card> cards);
    Optional<MoneyCard> chooseOptionalMoneyCard(List<MoneyCard> cards);

}

Die Klasse hat dabei zwei Verantwortlichkeiten vereint:

den Spieler informieren über den Spielzustand bzw. über ihn selbst, z.B. seine Karten
(void informNoCardsBuyableWithMoney(int money))
die Spielerin Entscheidungen treffen lassen über ihren Spielzug
(MoneyCard chooseMoneyCard(List<MoneyCard> cards))

Um den Code Smell zu beheben wurde das Interface aufgeteilt auf zwei neue Interfaces PlayerInformation sowie PlayerDecision:

public interface PlayerInformation {

    void startActionPhase();
    void startBuyingPhase();
    void noActionCardsPlayable();
    void noCardsBuyableWithMoney(int money);
    void yourTurn(String name);
    void results(List<PlayerResult> results);
    void winners(String... names);

}

public interface PlayerDecision {

    Card chooseCard(List<Card> cards);
    List<Card> chooseCards(List<Card> cards);
    ActionCard chooseActionCard(List<ActionCard> cards);
    Optional<ActionCard> chooseOptionalActionCard(List<ActionCard> cards);
    MoneyCard chooseMoneyCard(List<MoneyCard> cards);
    List<Card> chooseCardsToBuy(List<Card> cards);
    Optional<Card> chooseOptionalCardToBuy(List<Card> cards);
    Optional<MoneyCard> chooseOptionalMoneyCard(List<MoneyCard> cards);

}

Das bisherige Interface wurde von PlayerDecision zu PlayerInteraction umbenannt, das wie bereits hier bei den Aggregates beschrieben zwei Methoden PlayerDecision decision() und PlayerInformation information() anbietet, um das jeweilige Objekt vom richtigen Typ für die entsprechende Aufgabe zu erhalten:

public final class PlayerInteraction {

    private PlayerDecision decision;
    private PlayerInformation information;

    public PlayerInteraction(PlayerDecision decision, PlayerInformation information) {
        this.decision = decision;
        this.information = information;
    }

    public PlayerDecision decision() {
        return decision;
    }

    public PlayerInformation information() {
        return information;
    }

}

Damit ergibt sich nun dieses UML-Diagramm:

Aggregate PlayerInteraction

2 Refactorings

Zwei unterschiedliche Refactorings aus der Vorlesung anwenden, begründen, sowie UML vorher/nachher liefern; jeweils auf die Commits verweisen

Refactoring 1: Extract method

Der Code zum Ausgeben einer Karte auf die Konsole (inklusive Kartennummern) in der Klasse GameCLI wurde vor Commit 0140db an mehreren Stellen verwendet:

@Override
public Card chooseCard(List<Card> cards) {
    ...
    for (int i = 0; i < cards.size(); i++) {
        Card card = cards.get(i);
        System.out.print((i + 1) + ": " + card.getName());
    }
    ...
}

private Optional<Card> chooseOptionalCard(List<Card> cards) {
    ...
    for (int i = 0; i < cards.size(); i++) {
        Card card = cards.get(i);
        System.out.print((i + 1) + ": " + card.getName());
    }
    ...
}

@Override
public List<Card> chooseCards(List<Card> cards) { {
    ...
    for (int i = 0; i < cards.size(); i++) {
        Card card = cards.get(i);
        System.out.print((i + 1) + ": " + card.getName());
    }
    ...
}

Auch dieser relativ kleine, aber dennoch duplizierte “Codeschnipsel” macht den Code schwerer wartbar, da er an mehreren Stellen verwendet wird. Möchte man nun die Ausgabe von mehreren Karten ändern, müsste man alle Codestellen finden, an denen die Ausgabe stattfindet und darf dabei keine Methode übersehen. Mittels “Extract Method” wurde diese Funktionalität entsprechend in eine eigene Method innerhalb derselben Klasse ausgelagert, sodass der Code nach dem Commit so aussah:

private void printCardsWithNumbers(List<Card> cards) {
    for (int i = 0; i < cards.size(); i++) {
        Card card = cards.get(i);
        System.out.print((i + 1) + ": " + card.getName());
    }
}

@Override
public Card chooseCard(List<Card> cards) {
    ...
    printCardsWithNumbers(cards);
    ...
}

private Optional<Card> chooseOptionalCard(List<Card> cards) {
    ...
    printCardsWithNumbers(cards);
    ...
}

@Override
public List<Card> chooseCards(List<Card> cards) { {
    ...
    printCardsWithNumbers(cards);
    ...
}

Auf ein UML-Diagramm soll an diese Stelle verzichtet werden, da dieses lediglich zeigen würde, dass eine neue Methode print CardsWithNumbers(List<Card> cards) hinzugekommen ist.

Refactoring 2: Replace Conditional with Polymorphism

Das Verhalten zum Ausgeben eines Kartenrumpfes (Body) wurde vor Commit 2098d mithilfe von If-Else-Statements gesteuert, wie hier zu sehen:

public final class CardPrinter {

    public static void printCard(Card card) {
        printHeader(card);

        if (card instanceof ActionCard) {
            ActionCardPrinter.printBody((ActionCard) card);
        } else if (card instanceof MoneyCard) {
            MoneyCardPrinter.printBody((MoneyCard) card);
        } else if (card instanceof PointCard) {
            PointCardPrinter.printBody((PointCard) card);
        } else {
            printDefaultCardBody();
        }

        printFooter(card);
    }

}

Problematisch ist hier, dass die einzelnen Printer teilweise dieselben Funktionalitäten benötigen. Außerdem müsste hier bei Erweiterung ein hässliches und klobiges if-else-Konstrukt angepasst werden, das darüber hinaus mit instanceof arbeitet und damit bereits auf einen besseren Ansatz mit Polymorphie hindeutet.

Zur Lösung des Problems wurde die abstrakte und generische Klasse CardBodyFormatter<T extends Card> eingeführt, die unter anderem die Methode public abstract String getBody(T card) bereitstellt. Ein MoneyCardFormatter kann dann beispielsweise von dieser Klasse erben und die Methode String getBody(MoneyCard card) überschreiben. Entsprechendes ist beim PointCardFormatter und dem ActionCardFormatter der Fall. Von großem Vorteil ist nun, dass beispielsweise der MoneyCardFormatter Methoden wie getEmptyLineWithBorder() des CardBodyFormatters verwenden kann. Methoden, die der CardFormatter implementiert sind also für alle Unterklassen wiederverwendbar.

Statt des If-Else-Konstrukts wird jetzt eine HashMap verwendet:

public final class CardFormatter {

    private static final Map<Class<? extends Card>, Function<Card, String>> formatterMap = Map.of(
            ActionCard.class, c -> new ActionCardFormatter().getBody((ActionCard) c),
            MoneyCard.class, c -> new MoneyCardFormatter().getBody((MoneyCard) c),
            PointCard.class, c -> new PointCardFormatter().getBody((PointCard) c));

    public static String getFormatted(Card card) {
        StringBuilder str = new StringBuilder();

        // Header
        str.append(getHeader(card));

        // Body
        // will fail if class not in map, which is intended
        Function<Card, String> formatFunction = formatterMap.get(card.getClass());
        str.append(formatFunction.apply(card));

        // Footer
        str.append(getFooter(card));

        return str.toString();
    }

}

Der CardPrinter wurde in diesem Commit übrigens zu CardFormatter umbenannt, da die Karten nicht mehr direkt auf die Konsole mittels System.out.println(...) ausgegeben werden sollen, sondern die Methoden entsprechende Strings zurückgeben. Diese können dann unabhängig von der Formatierung der Karten in anderen Klassen auf die Konsole gebracht werden.

Das UML-Diagramm sieht nach dem Refactoring wie untenstehend gezeigt aus. Vor dem Refactoring war nur die rote Klasse CardPrinter (sie heißt nun CardFormatter) vorhanden, jedoch ohne die formatterMap sowie mit leicht geänderten Methodensignaturen (z.B. wurde aus void printBody(Card card) nun String getFormatted(Card card)).

Refactoring CardFormatter