-
Notifications
You must be signed in to change notification settings - Fork 0
Kapitel 7: Refactoring
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)
Die Spiellogik der beiden Spielmodi Training und LocalMultiplayer ist, wie in den Code-Beispielen zu sehen, nahezu identisch. Der einzige Unterschied ist die Anzahl der Spieler. Während der LocalMultiplayer dazu auf das UserRepository zugreift und alle Benutzer in das Spiel aufnimmt, wird der Trainingsmodus immer nur mit einem Spieler gespielt. An dieser Stelle kämen zwei mögliche Lösungswege in Frage. Da mittlerweile eine Benutzerverwaltung implementiert wurde, könnte der Trainingsmodus entfernt werden. Der lokale Spielmodus könnte dann entsprechend der aktuell eingestellten Benutzer mit einer beliebigen Anzahl an Spielern gespielt werden. Dies hätte den Nachteil, dass für eine Trainingsrunde alle Benutzer entfernt werden müssten. Dem könnte eine Auswahl der Benutzer für das Spiel vor Spielbeginn entgegenwirken. Eine andere Möglichkeit wäre es, die duplizierte Logik in eine Methode auszulagern, die mit der Anzahl der Spieler aufgerufen wird. Dann würden weiterhin zwei separate Spielmodi im Menü angeboten werden, die allerdings die gleiche Logik verwenden. Eine Entscheidung steht noch aus.
TrainingScreenProvider.java
public void execute() {
while (!quizGame.isFinished()) {
if (quizGame.isSelectingCategory()) {
String playerName = quizGame.getCurrentPlayer().getPlayerName();
OptionScreen optionScreen = screenFactory.createOptionScreen(playerName + ", select category", quizGame.getRemainingGameCategories());
optionScreen.render();
int actionValue = optionScreen.getOptionInput().getActionValue();
quizGame.setCurrentCategory(actionValue - 1);
} else {
Question currentQuestion = quizGame.getCurrentQuestion();
OptionScreen optionScreen = screenFactory.createOptionScreen("Question: " + currentQuestion.getQuestion(), List.of(currentQuestion.getQuestionOptions()));
optionScreen.render();
int actionValue = optionScreen.getOptionInput().getActionValue();
quizGame.submitQuestionAnswer(actionValue - 1);
}
}
List<String> lines = new ArrayList<>();
Player trainingPlayer = quizGame.getPlayers()[0];
lines.add(String.format("%s - %d points", trainingPlayer.getPlayerName(), trainingPlayer.getCurrentScore().getIntScore()));
screenFactory.createInformationScreen("Result", lines).render();
}LocalMultiplayerScreenProvider.java
public void execute() {
while (!quizGame.isFinished()) {
if (quizGame.isSelectingCategory()) {
String playerName = quizGame.getCurrentPlayer().getPlayerName();
OptionScreen optionScreen = screenFactory.createOptionScreen(playerName + ", select category", quizGame.getRemainingGameCategories());
optionScreen.render();
int action = optionScreen.getOptionInput().getActionValue();
quizGame.setCurrentCategory(action - 1);
} else {
Question currentQuestion = quizGame.getCurrentQuestion();
OptionScreen optionScreen = screenFactory.createOptionScreen("Question: " + currentQuestion.getQuestion(), List.of(currentQuestion.getQuestionOptions()));
optionScreen.render();
int action = optionScreen.getOptionInput().getActionValue();
quizGame.submitQuestionAnswer(action - 1);
}
}
List<String> lines = new ArrayList<>();
Player[] players = quizGame.getPlayers();
for (Player player : players) {
lines.add(String.format("%s - %d points", player.getPlayerName(), player.getCurrentScore().getIntScore()));
}
screenFactory.createInformationScreen("Result", lines).render();
}Mit dem GameRoom gab es eine Klasse mit sehr vielen Methoden, wie im Code-Beispiel zu sehen. Dies ist meist ein Hinweis darauf, dass eine Klasse zu viel Verantwortung in sich vereint und eine Unterteilung sinnvoll sein könnte.
public abstract class GameRoom {
private final User user;
private final String code;
public GameRoom(User user, String code) {
this.user = user;
this.code = code;
}
public String getNameOfPlayingUser() {
return user.getName();
}
public String getCode() {
return code;
}
public abstract void waitForGameToFinish();
public abstract void addListener(GameRoomListener gameRoomListener);
public abstract String getRoomName();
public abstract List<User> getPlayers();
public abstract CategoryRepository getRoomCategoryRepository();
public abstract void dispatchRoomJoin();
public abstract void dispatchPlayerUpdate();
public abstract void dispatchGameStart();
public abstract void dispatchGameTurnAction();
public abstract void dispatchGameTurnListen();
public abstract void dispatchGameTurnListenCategorySubmission(int selectedCategoryId);
public abstract void dispatchGameTurnListenQuestionOptionSubmission(int selectedQuestionOptionIndex);
public abstract void sendSelectedCategoryId(int categoryId);
public abstract void sendSelectedQuestionOptionIndex(int questionOptionIndex);
}In diesem Commit wurden die Methoden der Klasse daher auf mehrere Interfaces verteilt. Im folgenden Beispiel wird nur exemplarisch das Interface GameRoomActionDispatcher nach dieser Unterteilung gezeigt.
public abstract class GameRoom implements GameRoomActionDispatcher, GameRoomActionSender, GameRoomLifetimeDispatcher {
private final User user;
private final String code;
public GameRoom(User user, String code) {
this.user = user;
this.code = code;
}
public String getNameOfPlayingUser() {
return user.getName();
}
public String getCode() {
return code;
}
public abstract String getRoomName();
public abstract List<User> getPlayers();
public abstract CategoryRepository getRoomCategoryRepository();
}public interface GameRoomActionDispatcher {
void dispatchGameTurnAction();
void dispatchGameTurnListen();
void dispatchGameTurnListenCategorySubmission(int selectedCategoryId);
void dispatchGameTurnListenQuestionOptionSubmission(int selectedQuestionOptionIndex);
}Zwei unterschiedliche Refactorings aus der Vorlesung anwenden, begründen, sowie UML vorher/nachher liefern, jeweils auf die Commits verweisen
An vielen Stellen in der Anwendung werden Menüs mit einer Liste an Optionen angezeigt. Beispielsweise ist dies beim Wählen des Spielmodi oder beim Ändern von Spieleinstellungen der Fall. Implementiert werden diese mit einem sogenannten OptionScreen, der diese Funktionalität anbietet. Das Erzeugen des OptionScreen sowie die Entgegennahme und Überprüfung der Rückgabewerte wurde dann im jeweiligen ScreenProvider erledigt. Dies hat in vielen Fällen zu sehr ähnlichen Strukturen geführt, die mit dem MenuCreator in eine eigene Klasse extrahiert werden konnten. Nach der Einführung des MenuCreators konnten diese Strukturen
in weiteren ScreenProvidern ersetzt werden. Diese Deduplizierung vereinfacht die Logik der verschiedenen ScreenProvider und macht den Code dadurch verständlicher.
Vorher
public void execute() {
int selection = 0;
while (selection <= 0 || selection > 4) {
OptionScreen optionScreen = screenFactory.createOptionScreen("Select Game Mode", List.of(GameModeEnum.values()));
optionScreen.render();
selection = optionScreen.getOptionInput().getActionValue();
}
switch (selection) {
case 1 -> this.nextScreenProviderType = ScreenProviderType.TRAINING;
case 2 -> this.nextScreenProviderType = ScreenProviderType.LOCAL_MULTIPLAYER;
case 3 -> this.nextScreenProviderType = ScreenProviderType.ONLINE_MULTIPLAYER;
}
}classDiagram
direction BT
class GameModeScreenProvider {
+ GameModeScreenProvider(Repository, ScreenFactory)
- ScreenProviderType nextScreenProviderType
- ScreenFactory screenFactory
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class GameSettingsScreenProvider {
+ GameSettingsScreenProvider(Repository, ScreenFactory)
- ScreenProviderType nextScreenProviderType
- ScreenFactory screenFactory
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class MenuScreenProvider {
+ MenuScreenProvider(Repository, ScreenFactory)
- ScreenFactory screenFactory
- Repository repository
- ScreenProviderType nextScreenProviderType
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class OnlineMultiplayerScreenProvider {
+ OnlineMultiplayerScreenProvider(Repository, ScreenFactory, GameRoomManager)
- GameRoom gameRoom
- ScreenFactory screenFactory
- Repository repository
- GameRoomManager gameRoomManager
- QuizGame onlineQuizGame
- User playingUser
+ onJoinedRoom() void
+ onGameTurnListen() void
+ execute() void
+ onGameTurnListenQuestionOptionSubmission(int) void
+ onGameStart() void
+ onGameTurnListenCategorySubmission(int) void
+ getNextScreenProviderType() ScreenProviderType
- displayCategorySelection(boolean) void
+ onGameTurnAction() void
- joinRoom() void
- createNewRoom() void
+ onUpdatePlayers() void
}
class RemoveUserScreenProvider {
+ RemoveUserScreenProvider(Repository, ScreenFactory)
- ScreenProviderType nextScreenProviderType
- Repository repository
- ScreenFactory screenFactory
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class ScreenProvider {
<<Interface>>
+ getNextScreenProviderType() ScreenProviderType
+ execute() void
}
GameModeScreenProvider ..> ScreenProvider
GameSettingsScreenProvider ..> ScreenProvider
MenuScreenProvider ..> ScreenProvider
OnlineMultiplayerScreenProvider ..> ScreenProvider
RemoveUserScreenProvider ..> ScreenProvider
Nachher
public void execute() {
MenuCreator menuCreator = new MenuCreator("Select Game Mode", GameModeEnum.values(),
screenFactory);
int selection = menuCreator.displayAndGetSelection();
switch (selection) {
case 1 -> this.nextScreenProviderType = ScreenProviderType.TRAINING;
case 2 -> this.nextScreenProviderType = ScreenProviderType.LOCAL_MULTIPLAYER;
case 3 -> this.nextScreenProviderType = ScreenProviderType.ONLINE_MULTIPLAYER;
}
}classDiagram
direction BT
class GameModeScreenProvider {
+ GameModeScreenProvider(Repository, ScreenFactory)
- ScreenProviderType nextScreenProviderType
- ScreenFactory screenFactory
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class GameSettingsScreenProvider {
+ GameSettingsScreenProvider(Repository, ScreenFactory)
- ScreenProviderType nextScreenProviderType
- ScreenFactory screenFactory
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class MenuCreator {
+ MenuCreator(String, Displayable[], ScreenFactory)
- String menuTitle
- Displayable[] menuItems
- ScreenFactory screenFactory
+ displayAndGetSelection() int
}
class MenuScreenProvider {
+ MenuScreenProvider(Repository, ScreenFactory)
- ScreenFactory screenFactory
- Repository repository
- ScreenProviderType nextScreenProviderType
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class OnlineMultiplayerScreenProvider {
+ OnlineMultiplayerScreenProvider(Repository, ScreenFactory, GameRoomManager)
- GameRoom gameRoom
- ScreenFactory screenFactory
- Repository repository
- GameRoomManager gameRoomManager
- QuizGame onlineQuizGame
- User playingUser
+ onJoinedRoom() void
+ onGameTurnListen() void
+ execute() void
+ onGameTurnListenQuestionOptionSubmission(int) void
+ onGameStart() void
+ onGameTurnListenCategorySubmission(int) void
+ getNextScreenProviderType() ScreenProviderType
- displayCategorySelection(boolean) void
+ onGameTurnAction() void
- joinRoom() void
- createNewRoom() void
+ onUpdatePlayers() void
}
class RemoveUserScreenProvider {
+ RemoveUserScreenProvider(Repository, ScreenFactory)
- ScreenProviderType nextScreenProviderType
- Repository repository
- ScreenFactory screenFactory
+ execute() void
+ getNextScreenProviderType() ScreenProviderType
}
class ScreenProvider {
<<Interface>>
+ getNextScreenProviderType() ScreenProviderType
+ execute() void
}
GameModeScreenProvider ..> MenuCreator : «create»
GameModeScreenProvider ..> ScreenProvider
GameSettingsScreenProvider ..> MenuCreator : «create»
GameSettingsScreenProvider ..> ScreenProvider
MenuScreenProvider ..> MenuCreator : «create»
MenuScreenProvider ..> ScreenProvider
OnlineMultiplayerScreenProvider ..> MenuCreator : «create»
OnlineMultiplayerScreenProvider ..> ScreenProvider
RemoveUserScreenProvider ..> MenuCreator : «create»
RemoveUserScreenProvider ..> ScreenProvider
In einer ursprünglichen Implementierung des Screen Handlings (siehe Codestand bei folgendem Commit) wurde der Kontrollfluss der Anwendung von einer Klasse in der Plugin-Schicht gesteuert (QuizClashCLI). Diese hat sich einen ScreenProviderManager instanziiert, welcher eine Referenz auf den derzeitigen ScreenProvider hat und durchgeführte Aktionen in der CLI in den derzeitigen ScreenProvider weiterreichen konnte. Dies widerspricht vielen Prinzipien der clean architecture Methodologie, da die Steuerung des Kontrollflusses nicht in der Plugin-Schicht liegen sollte. Weiterhin bestand das Problem, dass die render Methode von QuizClashCLI einen Screen als Eingabe erhält, welcher vom ScreenProviderManager durchgegeben wurde. Es war somit notwendig, die Instanz des Screens zu überprüfen, um eine korrekte Darstellung je nach Typ zu ermöglichen (TextInputScreen, etc.). Dies wurde über mehrere instanceof Aufrufe geregelt.
Vorher
if (screen instanceof OptionScreen optionScreen) {
List<? extends Displayable> optionList = optionScreen.getScreenOptions();
Action<Integer> integerAction = selectFromOptions(optionList);
screenProviderManager.submitIntegerAction(integerAction);
} else if (screen instanceof TextInputScreen textInputScreen) {
Action<String> stringAction = enterTextFromRequest(textInputScreen.getInputRequest());
screenProviderManager.submitStringAction(stringAction);
} else if (screen instanceof NumberInputScreen numberInputScreen) {
Action<Integer> integerAction = enterNumberFromRequest(numberInputScreen.getInputRequest());
screenProviderManager.submitIntegerAction(integerAction);
} else if (screen instanceof InformationScreen informationScreen) {
for (String line : informationScreen.getLines()) {
cliWindow.println(line);
}
cliWindow.moveToActionField();
cliWindow.waitForEnter();
}classDiagram
direction BT
class InformationScreen {
+ InformationScreen(String, List~String~)
- List~String~ lines
+ getLines() List~String~
}
class NumberInputScreen {
+ NumberInputScreen(String, String)
- String inputRequest
+ getInputRequest() String
}
class OptionScreen {
+ OptionScreen(String, List~Displayable~)
- List~Displayable~ displayableList
+ getScreenOptions() List~Displayable~
}
class QuizClashCLI {
+ QuizClashCLI(int, int, Repository)
- CLIWindowManager cliWindow
- ScreenProviderManager screenProviderManager
- enterNumberFromRequest(String) Action~Integer~
+ run() void
- enterTextFromRequest(String) Action~String~
+ destroy() void
- selectFromOptions(List~Displayable~) Action~Integer~
- render(Screen) void
}
class Screen {
+ Screen(String)
- String screenName
+ getScreenName() String
}
class ScreenProviderManager {
+ ScreenProviderManager(Repository)
- ScreenProvider currentScreenProvider
- Repository repository
+ getCurrentScreenProvider() ScreenProvider
+ updateScreenProvider() void
+ submitStringAction(Action~String~) void
+ submitIntegerAction(Action~Integer~) void
}
class Starter {
+ Starter()
+ main(String[]) void
}
class TextInputScreen {
+ TextInputScreen(String, String)
- String inputRequest
+ getInputRequest() String
}
InformationScreen --> Screen
NumberInputScreen --> Screen
OptionScreen --> Screen
QuizClashCLI ..> ScreenProviderManager : «create»
QuizClashCLI "1" *--> "screenProviderManager 1" ScreenProviderManager
Starter ..> QuizClashCLI : «create»
TextInputScreen --> Screen
Man sieht, dass durch das instanceof keine vernünftige Kopplung ersichtlich ist und so das Konzept nicht schlüssig ist. Die Klasse QuizClashCLI muss immer dann geändert werden, wenn eine neue Screen Implementierung vorliegt. Daher wurde hier ein Factory Pattern eingeführt (siehe folgender Commit), um einerseits den Kontrollfluss der Anwendung wieder in die Application-Schicht zu bringen und das Rendering als abstrakte Methode in der Screen Basisklasse zu definieren, um dynamische Implementierungen zu ermöglichen. Weiterhin liegt die Erstellung des Objekts vom Typ ScreenProviderManager nicht mehr im Aufgabenbereich von QuizClashCLI. Das resultierende UML Diagramm ist im Folgenden dargestellt.
Nachher
run Methode von ScreenProviderManager
public void run() {
while (currentScreenProvider != null) {
currentScreenProvider.execute();
this.updateScreenProvider();
}
}classDiagram
direction BT
class CLIInformationScreen {
+ CLIInformationScreen(String, List~String~, CLIWindowManager)
+ CLIInformationScreen(String, List~String~, boolean, CLIWindowManager)
~ CLIWindowManager cliWindow
+ render() void
}
class CLINumberInputScreen {
+ CLINumberInputScreen(String, String, CLIWindowManager)
- CLIWindowManager cliWindow
- int userInput
+ render() void
+ getNumberInput() Action~Integer~
}
class CLIOptionScreen {
+ CLIOptionScreen(String, List~Displayable~, CLIWindowManager)
- CLIWindowManager cliWindow
- int userOption
+ getOptionInput() Action~Integer~
+ render() void
- selectFromOptions(List~Displayable~) int
}
class CLIScreenFactory {
+ CLIScreenFactory(CLIWindowManager)
~ CLIWindowManager cliWindow
+ createOptionScreen(String, List~Displayable~) OptionScreen
+ createNumberInputScreen(String, String) NumberInputScreen
+ createTextInputScreen(String, String) TextInputScreen
+ createInformationScreen(String, List~String~) InformationScreen
+ createInformationScreen(String, List~String~, boolean) InformationScreen
}
class CLITextInputScreen {
+ CLITextInputScreen(String, String, CLIWindowManager)
- CLIWindowManager cliWindow
- String userInput
+ getTextInput() Action~String~
+ render() void
}
class InformationScreen {
+ InformationScreen(String, List~String~, boolean)
+ InformationScreen(String, List~String~)
- boolean isBlocking
- List~String~ lines
+ getLines() List~String~
+ isBlocking() boolean
}
class NumberInputScreen {
+ NumberInputScreen(String, String)
- String inputRequest
+ getInputRequest() String
+ getNumberInput() Action~Integer~
}
class OptionScreen {
+ OptionScreen(String, List~Displayable~)
- List~Displayable~ displayableList
+ getScreenOptions() List~Displayable~
+ getOptionInput() Action~Integer~
}
class QuizClashCLI {
+ QuizClashCLI(int, int)
- CLIWindowManager cliWindow
+ destroy() void
+ getCLIScreenFactory() CLIScreenFactory
}
class Screen {
+ Screen(String)
- String screenName
+ render() void
+ getScreenName() String
}
class ScreenFactory {
<<Interface>>
+ createNumberInputScreen(String, String) NumberInputScreen
+ createTextInputScreen(String, String) TextInputScreen
+ createInformationScreen(String, List~String~, boolean) InformationScreen
+ createOptionScreen(String, List~Displayable~) OptionScreen
+ createInformationScreen(String, List~String~) InformationScreen
}
class ScreenProviderManager {
+ ScreenProviderManager(Repository, ScreenFactory, GameRoomManager)
- ScreenFactory screenFactory
- GameRoomManager gameRoomManager
- ScreenProvider currentScreenProvider
- Repository repository
+ run() void
+ updateScreenProvider() void
}
class Starter {
+ Starter()
+ main(String[]) void
}
class TextInputScreen {
+ TextInputScreen(String, String)
- String inputRequest
+ getTextInput() Action~String~
+ getInputRequest() String
}
CLIInformationScreen --> InformationScreen
CLINumberInputScreen --> NumberInputScreen
CLIOptionScreen --> OptionScreen
CLIScreenFactory ..> CLIInformationScreen : «create»
CLIScreenFactory ..> CLINumberInputScreen : «create»
CLIScreenFactory ..> CLIOptionScreen : «create»
CLIScreenFactory ..> CLITextInputScreen : «create»
CLIScreenFactory ..> ScreenFactory
CLITextInputScreen --> TextInputScreen
InformationScreen --> Screen
NumberInputScreen --> Screen
OptionScreen --> Screen
QuizClashCLI ..> CLIScreenFactory : «create»
ScreenProviderManager "1" *--> "screenFactory 1" ScreenFactory
Starter ..> QuizClashCLI : «create»
Starter ..> ScreenProviderManager : «create»
TextInputScreen --> Screen