Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Review zur Übungsaufgabe: Countdown #1

Open
wants to merge 1 commit into
base: review
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 105 additions & 0 deletions resources/js/game/GameManager.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/* eslint-env browser */

import Config from "../utils/Config.js";
import { Event, Observable } from "../utils/Observable.js";
import LetterGenerator from "./LetterGenerator.js";
import WiktionaryClient from "../wiktionary/Wiktionary.js";

var currentLetters,
countdownTimeout;

function broadcastEvent(type, data) {
let event = new Event(type, data);
this.notifyAll(event);
}
Comment on lines +11 to +14

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


function getResultObjectForValidWord(word) {
let points = (word.length === Config.MAX_CHARS) ? Config.POINTS_FOR_NINE_LETTER_WORD : word.length;
return getResultObject(word, Config.POSITIVE_RESULT_MESSAGE, points);
}

function getResultObject(word, hint, points) {
return {
word: word,
hint: hint,
points: points,
};
Comment on lines +22 to +26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (currentLetters.length === Config.MAX_CHARS) {
broadcastEvent.call(this, "LastCharAdded");
}
}

function isValidCharCombination(word) {
let result = word.toUpperCase();
for (let i = 0; i < currentLetters.length; i++) {
result = result.replace(currentLetters[i].toUpperCase(), "");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (result.length === 0) {
return true;
}
return false;
}

function onTimeout() {
let event = new Event("CountdownStopped");
this.notifyAll(event);
}

function clearCountdownTimeoutIfRunning() {
if (countdownTimeout) {
clearTimeout(countdownTimeout);
}
}

class GameManager extends Observable {

reset() {
clearCountdownTimeoutIfRunning();
LetterGenerator.reset();
currentLetters = [];
}

startCountdown() {
countdownTimeout = setTimeout(onTimeout.bind(this), Config.GAME_TIME_IN_MS);
broadcastEvent.call(this, "CountdownStarted");
}

addRandomVowel() {
let vowel = LetterGenerator.getVowel();
addChar.call(this, vowel);
}

addRandomConsonant() {
let consonant = LetterGenerator.getConsonant();
addChar.call(this, consonant);
}

validateWord(word) {
return new Promise(function (resolve, reject) {
let isUsingCorrectLetters = isValidCharCombination(word);
if (!isUsingCorrectLetters) {
let result = getResultObject(word, Config.INVALID_COMBINATION_RESULT_MESSSAGE, 0);
reject(result);
Comment on lines +88 to +89

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
let wc = new WiktionaryClient();
wc.assertWordExist(word).then(function () {
let result = getResultObjectForValidWord(word);
resolve(result);
}).catch(function () {
let result = getResultObject(word, Config.INVALID_WORD_RESULT_MESSAGE, 0);
resolve(result);
});
}
});
Comment on lines +85 to +100

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Durch das direkte Erstellen des Promises wirkt der Code hier etwas unübersichtlich. Auch der Promise-Prototyp kann durch Vererbung erweitert werden. Ggf. sollten wir hier die gesamte Funktionalität als eigenständigen Prototypen auf Modul-Ebene (ggf. sogar in einer eigenen Datei) implementieren.

}

}

export default new GameManager();
78 changes: 78 additions & 0 deletions resources/js/game/LetterGenerator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/* eslint-env browser */

import Config from "../utils/Config.js";

let vowels,
consonants;

function shuffleStack(stack) {
let j, x, i;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Für eine Hilfsmethode sind die Variablen noch ok. Trotzdem denke ich, wir sollten uns Gedanken über passendere Bezeichner machen.

for (i = stack.length; i; i--) {
j = Math.floor(Math.random() * i);
x = stack[i - 1];
stack[i - 1] = stack[j];
stack[j] = x;
}
}

function prepareStacks(letterFreq) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich finde die Methode würde durch einen anderen Namen besser ihre Funktionalität kommunizieren. Z.B. createStacksForChars, da hier ja ausgehend von leeren Arrays, überhaupt erst die Buchstabenstapel erzeugt, nicht nur vorbereitet werden.

let target;
vowels = [];
consonants = [];
/**
* letterFreq is an object parsed from resources/data/letter-freq.json
* Each property of letterFreq represents an letter and its frequency:
* {
* "a": 8.167,
* "b": 1.492,
* "c": 2.782,
* "d": 4.253,
* ...
*/
for (let char in letterFreq) {
if (Object.prototype.hasOwnProperty.call(letterFreq, char)) {
// rounding the current letter's frequency
let freq = Math.floor(letterFreq[char]);
target = undefined;
// selecting the correct array (stack)
if (Config.VOWELS.includes(char)) {
target = vowels;
} else if (Config.CONSONANTS.includes(char)) {
target = consonants;
}
// Adding the current letter n-times (freq) to the array
if (target) {
for (let i = 0; i < freq; i++) {
target.push(char);
}
}
}
Comment on lines +35 to +49

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Achtung: Hier ist wahrscheinlich ein Bug im Code: Einige der Werte aus der Konfigurationsdatei die hier via freq verarbeitet werden sind kleiner als 1. Wenn wir den Wert abrunden floor, werden für diese Buchstaben keine Exemplare im Stapel ergänzt! Eine einfache Lösung (für die verwendete Wahrscheinlichkeitsverteilung) wäre es, die Werte mit 10 zu multiplizieren und dann abzurunden.

}
shuffleStack(vowels);
shuffleStack(consonants);
}

function selectRandomCharFrom(chars) {
let randomIndex = Math.floor(Math.random() * (chars.length - 1)),
randomChar = chars[randomIndex];
chars.splice(randomIndex, 1);
return randomChar;
}
Comment on lines +55 to +60

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wenn die Stacks zu Beginn zufällig gemischt werden, ist hier doch eigentlich keine Zufallsauswahl mehr notwendig, oder? Können wir nicht einfach immer den ersten Eintrag vom jeweiligen Stapel nehmen und zurückgeben?


class LetterGenerator {

reset() {
prepareStacks(Config.LETTER_FREQUENCY);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
Comment on lines +68 to +74

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@AnonymousCodeReviewer AnonymousCodeReviewer Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nachtrag: Der Kommentar bezieht sich vor allem auf die unnötige Implementierung von selectRandomCharFrom.


}

export default new LetterGenerator();
100 changes: 98 additions & 2 deletions resources/js/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,104 @@
/* eslint-env browser */

import Config from "./utils/Config.js";
import AudioPlayer from "./utils/AudioPlayer.js";
import GameManager from "./game/GameManager.js";
import ClockView from "./ui/ClockView.js";
import LetterView from "./ui/LetterView.js";
import MenuView from "./ui/MenuView.js";
import ResultView from "./ui/ResultView.js";
import ControlsView from "./ui/ControlsView.js";

let menuView,
controlsView,
letterView,
clockView,
resultsView;

function init() {
// Start here
console.log("#### Starting Countdown app ####");
initGameManager();
initUI();
initAudio();
}

function initGameManager() {
GameManager.addEventListener("CharAdded", onCharAdded);
GameManager.addEventListener("LastCharAdded", onLastCharAdded);
GameManager.addEventListener("CountdownStarted", onCountdownStarted);
GameManager.addEventListener("CountdownStopped", onCountdownStopped);
}

function initUI() {
let menuEl = document.querySelector(".menu-screen"),
controlsEl = document.querySelector(".controls"),
lettersEl = document.querySelector(".letters"),
clockEl = document.querySelector(".clock"),
resultsEl = document.querySelector(
".results");
menuView = new MenuView(menuEl);
menuView.addEventListener("StartButtonClicked", startGame);
controlsView = new ControlsView(controlsEl);
controlsView.addEventListener("AddVowel", addVowel);
controlsView.addEventListener("AddConsonant", addConsonant);
letterView = new LetterView(lettersEl);
clockView = new ClockView(clockEl);
resultsView = new ResultView(resultsEl);
}

function initAudio() {
AudioPlayer.setFile(Config.PATH_TO_BACKGROUND_MUSIC);
}

function startGame() {
menuView.hide();
controlsView.clear();
controlsView.disable();
letterView.clear();
resultsView.clear();
resultsView.hide();
clockView.reset();
GameManager.reset();
}

function addVowel() {
GameManager.addRandomVowel();
}

function addConsonant() {
GameManager.addRandomConsonant();
}
Comment on lines +63 to +69

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


function showResults(results) {
resultsView.setPoints(results.points);
resultsView.setHint(results.hint);
resultsView.show();
setTimeout(() => menuView.show(), Config.TIME_BEFORE_MENU_IS_SHOWN);
}

function onCharAdded(event) {
letterView.addLetter(event.data);
}

function onLastCharAdded() {
GameManager.startCountdown();
}

function onCountdownStarted() {
clockView.start();
AudioPlayer.play();
controlsView.focus();
controlsView.enable();
}

function onCountdownStopped() {
clockView.reset();
AudioPlayer.stop();
let word = controlsView.getUserInput();
GameManager.validateWord(word).then((results) => {
showResults(results);
}).catch((results) => {
showResults(results);
});
}

init();
23 changes: 23 additions & 0 deletions resources/js/ui/ClockView.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/* eslint-env browser */

import View from "./View.js";

class ClockView extends View {

constructor(el) {
super();
this.setElement(el);
this.clockHand = this.el.querySelector(".hand");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

start() {
this.clockHand.classList.add("hand-animated");
}

reset() {
this.clockHand.classList.remove("hand-animated");
}

}

export default ClockView;
44 changes: 44 additions & 0 deletions resources/js/ui/ControlsView.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/* eslint-env browser */

import View from "./View.js";
import { Event } from "../utils/Observable.js";

function broadcastLetterEvent(type) {
let event = new Event(type);
this.notifyAll(event);
}

class ControlsView extends View {

constructor(el) {
super();
this.setElement(el);
this.inputEl = this.el.querySelector(".word-input");
this.el.querySelector(".add-vowel").addEventListener("click", broadcastLetterEvent.bind(this, "AddVowel"));
this.el.querySelector(".add-consonant").addEventListener("click", broadcastLetterEvent.bind(this, "AddConsonant"));
}

clear() {
this.inputEl.value = "";
}

enable() {
this.inputEl.disabled = false;
}

disable() {
this.inputEl.disabled = true;
}

focus() {
this.inputEl.focus();
}

getUserInput() {
return this.inputEl.value;
}
Comment on lines +37 to +39

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



}

export default ControlsView;
30 changes: 30 additions & 0 deletions resources/js/ui/LetterView.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/* eslint-env browser */

import View from "./View.js";

class LetterView extends View {

constructor(el) {
super();
this.setElement(el);
}

clear() {
let letters = this.el.querySelectorAll(".letter");
for (let i = 0; i < letters.length; i++) {
letters[i].innerHTML = "";
letters[i].classList.add("empty");
}
}

addLetter(letter) {
let nextLetterElement = this.el.querySelector(".letter.empty");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diese Variable könnten wir vielleicht in nextEmptyLetterElement umbennen, um deutlicher zu machen, dass das nächste freie Element ausgewählt wurde.

if (nextLetterElement !== undefined) {
nextLetterElement.innerHTML = letter;
nextLetterElement.classList.remove("empty");
}
}

}

export default LetterView;
Loading