Javascript code Review

Flure

Cadet 3rd Year
Registriert
Apr. 2016
Beiträge
61
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.
 
Hi,

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

VG,
Mad
 
  • 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
 
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.
 
  • Gefällt mir
Reaktionen: Nase, maloz, KitKat::new() und eine weitere Person
Ich würde zB den Code noch ein wenig von unnötigem Ballast befreien.

z.B. kannst du statt

Code:
if (play === false)

einfach

Code:
if(!play)

schreiben.

Und auch sowas

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

ließe sich durch

Code:
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 :)
 
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.
 
  • Gefällt mir
Reaktionen: maloz und KitKat::new()
mastaqz schrieb:
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?
 
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.
 
  • Gefällt mir
Reaktionen: maloz und BeBur
mastaqz schrieb:
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?
 
Zurück
Oben