Code Review zur Übungsaufgabe: Countdown #1
Conversation
There was a problem hiding this comment.
Ich habe mir den Code des Lösungsvorschlags angeschaut. Insgesamt geht das meiste schon in Ordnung. An einigen Stellen habe ich Verbesserungsvorschläge notiert. Am wichtigsten sind meiner Meinung nach die folgenden Punkte:
- Überarbeitung der Event-Generierung (z.B. mit Factory-Methoden)
- Debugging des LetterGenerators, in dessen Stapel einige Buchstaben nicht vorkommen
Die Qualität des Codes insgesamt ist akzeptabel.
| function broadcastEvent(type, data) { | ||
| let event = new Event(type, data); | ||
| this.notifyAll(event); | ||
| } |
There was a problem hiding this comment.
Diese Methode, zumindest in dieser Form, verbessert die Übersicht und Lesbarkeit des Codes nicht wirklich. Meiner Meinung nach sollte die Erstellung des Events und der Call von notifyAll direkt an der Stelle im Modul erfolgen, an der das Event kommuniziert werden muss. Diese Form der Dekomposition geht unnötig einen Schritt zu weit. Stattdessen können wir die Generierung der Events auslagern und im Modul Methoden zum Erzeugen der unterschiedlichen Events bereitstellen.
| return { | ||
| word: word, | ||
| hint: hint, | ||
| points: points, | ||
| }; |
There was a problem hiding this comment.
Statt eines anonymen Objekt-Literals wäre hier möglicherweise ein Prototyp als Grundlage für das Ergebnis-Objekt besser. Das hier zurückgegebene Objekt wird auch außerhalb des Moduls verwendet. Durch einen Prototypen, dessen Struktur App-weit bekannt ist, wird die Arbeit mit dem Rückgabeobjekt für andere ProgrammierInnen erleichtet.
| function addChar(char) { | ||
| if (currentLetters.length < Config.MAX_CHARS) { | ||
| currentLetters.push(char); | ||
| broadcastEvent.call(this, "CharAdded", char); |
There was a problem hiding this comment.
Dieser Aufruf setzt voraus, dass addChar selbst im richtigen Objekt-Scope aufgerufen wurden. Statt über mehrere Ecken call zu verwenden, könnte hier auch direkt der Context als zusätzlicher Paramater übergeben werden. Technisch ist das identisch zur Lösung mit call, macht den (intendierten) Ablauf des Programms aber deutlicher bzw. leichter zu erkennen.
Da das Konzept sich durch das ganze Modul zieht: Ich würde die Abhängigkeit zu call insgesamt aus dem Modul entfernen. Dazu kann überall der explizite Kontext übergeben werden.
| function addChar(char) { | ||
| if (currentLetters.length < Config.MAX_CHARS) { | ||
| currentLetters.push(char); | ||
| broadcastEvent.call(this, "CharAdded", char); |
There was a problem hiding this comment.
Die Event-Typen (hier CharAdded) sind an einer zentralen Stelle, z.B. in den jeweiligen Event-Prototypen besser aufgehoben. Dadurch können wir sicherstellen, dass hier keine "falschen" Werte übergeben werden und der gesetzte Typ stets den Erwartungen entspricht. Das gilt auch für die anderen Stellen im Code, die Events erzeugen. Am besten überarbeiten wir dies im Zusammenhang mit der vorgeschlagenen Auslagerung der Event-Erstellung.
| function isValidCharCombination(word) { | ||
| let result = word.toUpperCase(); | ||
| for (let i = 0; i < currentLetters.length; i++) { | ||
| result = result.replace(currentLetters[i].toUpperCase(), ""); |
There was a problem hiding this comment.
Hier wäre vielleicht eine Konstante für den leeren String, z.B. const EMPTY_STRING = "";sinnvoll. Dadurch lassen sich Missverständnisse vermeiden, z.B die falsche Annahme, hier könnte etwas zwischen den Leerzeichen vergessen worden sein.
| class LetterGenerator { | ||
|
|
||
| reset() { | ||
| prepareStacks(Config.LETTER_FREQUENCY); |
There was a problem hiding this comment.
Vielleicht können wir hier die Aufrufe von prepareStacks und shuffleStacks trennen? Wenn wir an dieser Stelle beide Methoden direkt aufrufen, wird - ohne das man dazu weiter in den Code schauen muss - sehr schnell klar, was alles beim Initialisieren des Generators passiert.
| getVowel() { | ||
| return selectRandomCharFrom(vowels); | ||
| } | ||
|
|
||
| getConsonant() { | ||
| return selectRandomCharFrom(consonants); | ||
| } |
There was a problem hiding this comment.
Auf die zusätzliche Zufallsauswahl können wir verzichten, wenn die Stapel bereits zu Beginn gemischt werden. Dann könnte die Methode auch passenderweise in getNextVowel umbenannt werden. Beides gilt auch für getConsonant.
There was a problem hiding this comment.
Nachtrag: Der Kommentar bezieht sich vor allem auf die unnötige Implementierung von selectRandomCharFrom.
| constructor(el) { | ||
| super(); | ||
| this.setElement(el); | ||
| this.clockHand = this.el.querySelector(".hand"); |
There was a problem hiding this comment.
Nicht kritisch, aber ggf. für das Refactoring relevant: Die CSS-Klassen könnten an einer zentralen Stelle oder hier im Modul als Konstanten abgebildet werden, statt innerhalb der Methoden hard eincodiert zu werden.
| getUserInput() { | ||
| return this.inputEl.value; | ||
| } |
There was a problem hiding this comment.
Wenn wir die Methode in getCurrentUserInput umbenennen wird deutlicher, dass wir den aktuellen Inhalt des Textfelds auslesen und keinen neuen Input von den NutzerInnen anfordern.
| let event = new Event("StartButtonClicked"); | ||
| this.notifyAll(event); |
There was a problem hiding this comment.
Auch hier könnten wir über die Verwendung von CustomEvents auf Basis eigener Prototypen nachdenken und ggf. deren Erstellung auslagern. Vielleicht benötigen wir für die gesamte Anwendung eine EventFactory?
| function addVowel() { | ||
| GameManager.addRandomVowel(); | ||
| } | ||
|
|
||
| function addConsonant() { | ||
| GameManager.addRandomConsonant(); | ||
| } |
There was a problem hiding this comment.
Da die Methoden als Callback für die Events aus dem ControlsView dienen: Können wir die Namen zu onAddVowel oder etwas Ähnlichem ändern? Damit sollte klarer werden, dass es sich hier um eine Callback-Methode handelt, die von außerhalb des Moduls und durch einen Event aufgerufen wird.
| let result = getResultObject(word, Config.INVALID_COMBINATION_RESULT_MESSSAGE, 0); | ||
| reject(result); |
There was a problem hiding this comment.
Auch ein fehlerhaftes Wort ist doch ein valides Ergebnis für das Spiel an sich. Sollte dieser Fall (mit entsprechendem Hinweis und null Punkten) nicht auch über das erfolgreiche auflösen des Promise (resolve) kommuniziert werden? Den reject-Callback würde ich für wirkliche "Probleme" beim Validieren reservieren, z.B. wenn keine Verbindung zur Wiktionary-API aufgebaut werden konnte oder die Eingaben der NutzerInnen nicht ausgewertet werden konnten.
| } | ||
|
|
||
| addLetter(letter) { | ||
| let nextLetterElement = this.el.querySelector(".letter.empty"); |
There was a problem hiding this comment.
Diese Variable könnten wir vielleicht in nextEmptyLetterElement umbennen, um deutlicher zu machen, dass das nächste freie Element ausgewählt wurde.
Code Review zur Übungsaufgabe Countdown
Bitte fertigen Sie ein Code Review für den Lösungsvorschlag in diesem Repository an. Konzentrieren Sie sich auf die qualitativen Aspekte des Quellcodes und des Software Designs. Sie müssen den Code nicht ausführen und keine funktionalen Fehler oder Bugs indentifizieren. Versuchen Sie, konstruktives Feedback zur besseren Gestaltung der Teilkomponenten und Funktionen bzw. zur allgemeine Optimierung der Codeqqualität zu geben. Suchen Sie nach offensichtlichen Verstößen gegen bekannte Regeln zur Gestaltung und Formatierung von JavaScript-Code und identifizieren Sie unsaubere Lösungen, die sich negativ auf den Gesamteindruck auswirken. Vergessen Sie dabei nicht, auch positives Feedback zu geben und gelungene Stellen un Umsetzungen zu kennzeichnen.
Machen Sie sich keine Sorgen über die Qualität Ihres Feedbacks. In der Regel sind alle Hinweise und Ratschläge nützlich. Das Anfertigen des Reviews soll nicht nur den EmpfängerInnen helfen, sonder auch eine Übungsmöglichkeit für Sie selbst darstellen. Nutzen Sie die Chance, wertvolle Erfahrungen für spätere Projekte zu sammeln.
Vorgehen
Sie können das Review vollständig auf der Github-Webseite durchführen. Organisatorische Grundlage ist dieser Pull Request.
Files changedsehen Sie eine Übersicht über alle (editierten) Dateien dieses Lösungvorschlags. Sichten Sie den Code in diesen Dateien und versuchen Sie an möglichst viele Stellen Feedback zu geben.Start Reviewaus.Finish your Reviewab. Am Ende können Sie auch einen allgemeinen Eindruck zur Qualität des Codes formulieren. Schicken Sie Ihr Feedback mit einem Klick auf die SchaltflächeSubmit your Reviewab.Checkliste
Bei der Erstellung des Reviews können Sie sich an dieser einfachen Checkliste orientieren:
Weitere Hinweise und Hilfestellungen