Javascript code Review

[Flure]

Hallo,

Ich versuche gerade meine Javascript Kenntnis aufzubessern. Dafür habe ich eine kleine application geschrieben. Mich würde interessieren, was ihr von dem Code haltet. Gerne nehme auch Kritik oder Verbesserungsvorschläge entgegen.

Danke schon mal an jedem der sich die Mühe macht hier mal rein zu schauen

Github: Github
Liveview: Liveview

PS: Hab auf das Designe ( wie man sieht) keinen großen Wert gelegt.

 

[Madman1209]

Hi,

Lied vor / zurück funktioniert nicht, wenn das Lied abgespielt wird. Playbutton bleibt dann "Pause", es spielt aber nichts ab.

VG,
Mad

 

[BeBur]

• Daten und Code trennen
• paar kleine Formfehler in den Comments
• Das ganze Ding in ein div mit Aussagekräftiger ID packen und nur drunterliegende Elemente auswählen lassen. Auf diese Weise vermeidest du Namenskonflikte, wenn das Ding irgendwo eingebaut wird.
• Entsprechend auch die globalen variablen los werden
• Listener umbauen, Funktionalität in separate Funktionen packen, so dass die einen aussagekräftigen Namen bekommen und Code-Level besser getrennt werden "if play == pause then triggerContinue()"
• Zeile 68 ist noch eine Debug Ausgabe mit rein gerutscht.
• Funktionsname "dataToHTML" ist imHo nicht selbsterklärend

 

[efcoyote]

Du solltest kein urheberrechtlich geschütztes Material (MP3) frei zum Download zur Verfügung stellen. Es gibt Menschen da draußen, die verschicken diesbezüglich gerne Abmahnungen.

 

[Toms]

Ich würde zB den Code noch ein wenig von unnötigem Ballast befreien.

z.B. kannst du statt

if (play === false)

einfach

if(!play)

schreiben.

Und auch sowas

 if (count === data.length - 1) {
    count = 0;
    dataToHTML();
  } else {
    count++;
    dataToHTML();
  }

ließe sich durch

count === data.length -1 ? count = 0 : count ++;
dataToHTML();

abkürzen. Ob das dann für dich persönlich leserlicher ist, musst du entscheiden. Leserlichkeit geht immer vor

 

[mastaqz]

Hallo Flure,

sehr toll, dass du nach einem Code Review fragst.

Hier mal ein paar Ideen und Vorschläge. Was du nutzt musst du selbst recherchieren.

Konzeptebene:

• Daten und Logik trennen (bspw. Architekur-Pattern wie MVC anschauen); senkt deinen Energieverbrauch (weniger Lesen und weniger Scrollen)
• Information Hiding: Schiebe Implementationen in Funktionen, so dass sie aus dem Weg sind; Beispiel: die Funktionen um ein Lied zu spielen oder zu pausieren; einmal geschrieben (und getestet) gehören sie in einen separaten Block, der aus deinen Augen verschwindet; allein ein guter Funktionsname erklärt WAS passiert, nicht WIE; dabei auf Single Responsibility achten und keine Gott-Objekte oder falsche Abstraktionen machen (bspw. keine Funktion bauen, die abspielt UND pausiert; Heuristik: and oder or im Name => zu viel Responsibility)
• Encapsulation: Geht Hand in Hand mit Information Hiding; deine Variablen und Funktionen leben aktuell auf der obersten Ebene; einfachste Lösung ist ein musicPlayer als Objekt, der die Sachen beinhaltet; erweitert OOP dann als Class; fortgeschritten dann als Singleton (du brauchst wahrscheinlich nur einen Player)
• häufig vorkommende hardcoded values abspeichern, bspw. die Ordnernamen der Bilder oder Lieder; sollte sich der Ordnername ändern, musst du nur eine Stelle ändern; machbar über interpolation in template literals; senkt deinen Energieverbrauch (weniger Ändern)
• ids zufällig generieren; machbar mit uuid (security; für dich nicht relevant, weil Trade-Off von uuid-Paket vs. Nutzen)

Implementationsebene:

• class für alles in CSS nutzen: erhöht die Developer Experience in fortgeschrittenen Projekten mit React/Vue/Angular/Svelte, weil es generisch nutzbar ist im Gegensatz zu id
• const anstatt let nutzen, außer es gibt einen Grund; das hat den Nebeneffekt, dass du sich ändernde Variablen schneller erkennst in einer Liste an Variablen

Generelles:

• git commits kleiner (atomar) machen, so dass Änderungen nachvollziehbarer sind und einfacher rückgängig gemacht werden können
• in Readme schreiben, was das Programm macht und wie man es benutzt
• kein urheberrechtlich geschützte Material nutzen, dessen Erlaubnis du nicht hast

Gute Arbeit.

 

[KitKat::new()]

class für alles in CSS nutzen: erhöht die Developer Experience in fortgeschrittenen Projekten mit React/Vue/Angular/Svelte, weil es generisch nutzbar ist im Gegensatz zu id

Könntest du das noch weitergehend erläutern?

 

[mastaqz]

Wenn man kein gescoped CSS nutzt, dann muss man an die CSS specifity denken:
https://www.w3.org/TR/selectors-3/#specificity

Das führt in vielen mittleren bis großen Anwendungen zu grausam komplexem CSS.

Das löst man, indem man einfach nur noch class benutzt.
Gestylte Elemente nutzen nur noch class, so hat jedes Element die gleiche CSS specifity und du musst nicht mit langen Selektorketten arbeiten.

Du musst auch nicht immer nachdenken, ob du jetzt das Tag, die Id oder die Class nimmst.

Darüber hinaus musst du dir keine Gedanken mehr machen, wie oft ein Element vorkommt, da es einfach immer eine class anstatt einer id bekommt.

Nachteil ist ein bisschen mehr Code, ist mMn aber ein sehr geringer Preis für die deutliche Senkung der Komplexität und Erhöhung der Maintainability.

 

[BeBur]

Darüber hinaus musst du dir keine Gedanken mehr machen, wie oft ein Element vorkommt, da es einfach immer eine class anstatt einer id bekommt.

Verliert man dadurch nicht semantik, also eine id ist immer eindeutig (im sinne eines design contracts) und wenn ich die selektiere dann bedeutet dies, dass ich ein Einzelelement auswähle und sonst eben ein oder auch mehrere wenn die class selektiert wird?
Gibt es guidelines wie generisch man die Klassen baut? Man will ja einerseits generische haben, aber andererseits, wenn man z.B. 1:1 typen, attribute etc. verwenden würde, dann würde man ja auch das Problem der specificity 1:1 abbilden oder? Ich kenne in erster Linie Bootstrap3/4, wie ist das in der Hinsicht einzuschätzen?

 

[mastaqz]

Ja, das beinhaltet alles eine Menge Trade-Offs.

Ein guter Text dazu hier: https://adamwathan.me/css-utility-classes-and-separation-of-concerns/

Spricht eigentlich den ganzen Themenkomplex gut an.

 

[BeBur]

Ein guter Text dazu hier: https://adamwathan.me/css-utility-classes-and-separation-of-concerns/

Mega cooler Artikel, vielen Dank. Das ganze ist vergleichbar mit der Situation bei Code. Seperation of concern, inheritance vs. composition, etc. Das Fazit des Artikels scheint mir auch genau richtig zu liegen.
Ich denke als Anfänger sollte man den Artikel lesen, als Lesezeichen speichern und 1 Jahr später nochmal lesen.