Code Review: Chromely

Im heutigen Code Review, möchte ich mir das mattkol/Chromely github Repository anschauen.

About

Vor dem Start, möchte ich noch ein paar Worte zur Wahl, zum Author und zum Projekt selber erzählen.

… the choise

Ich habe es mir eigentlich relativ einfach gemacht. Ich bin auf github gegangen und hab mir anschauen, was gerade so im Trend ist. Anschließend habe ich nach der Sprache C# eingeschränkt. Chromely hatte alleine an diesem Tag 29 Sterne bekommen.

… the author

Zunächst einmal was über den Author. mattkol heißt eigentlich Kola Oyewumi und ist .NET/WPF consultant. Er hat an der University of Liverppol studiert und lebt in North Carolina in der Stadt Charlotte.

… the project

Laut der Beschreibung handelt es sich bei diesem Projekt um eine leichtgewichtige Alternative für Electron.

Wer Electron nicht kennt, es ist ein Framework, mit dem man Cross-Plattform-Apps entwickeln und auf beliebigen Endgeräten verbreiten kann.

Um dies zu ermöglichen, wird JavaScript und HTML5 verwendet. Unter der Haube steckt ein node.js Server.

Chromely ist aktuell ca. 8 Monate alt und hat bereits 570 Sterne und 34 Forks.

Ich finde, das ist für so eine doch kurze Zeit, ein ziemlich gutes Ergebnis.

Meine Eindrücke vom Code

Zuerst einmal möchte ich betonen, dass ich das Projekt nicht gecloned, kompiliert und getestet habe. Auch habe ich keine weiteren Tools wie nDepend, FxCop, StyleCop oder ReSharper dafür verwendet.

Kommentare

Wenn ich mir das Core Namespace anschaue, sehe ich als letzten commit „Code and documentation improved with Resharper“.

Grund genung mir das anzusehen 🙂

Kola hat beispielsweise aus:

protected override CefDisplayHandler GetDisplayHandler()
{
	return m_displayHandler;
}

sowas hier gemacht:

/// <summary>
/// The get display handler.
/// </summary>
/// <returns>
/// The <see cref="CefDisplayHandler"/>.
/// </returns>
protected override CefDisplayHandler GetDisplayHandler()
{
	return this.mDisplayHandler;
}

Wenn ich solch ein Kommentar lese, denke ich mich immer „ach, echt?“ 🙂

Da fallen mir spontan 4 Sachen negativ auf:

  1. Der Kommentar ist nutzlos. Schlimmer noch, der Kommentar stört hier mehr als er hilft. Alles, was man im Kommentar lesen kann, findet man auch in der Definition der Methode. Vor allem ist es noch nicht einmal ein korrektes Englisch. Für mich sieht es stark nach auto-generated aus. Solche Kommentare sind zwar nett gemeint, blähen aber den code auf und haben absolut kein Mehrwert.
  2. „this“ ist mittlerweile Veraltet. ReSharper müsste das eigentlich anzeigen. Vor allem dingen, weil es vorher ja richtig war (also ohne das this)
  3. Aus m_displayHandler ein mDisplayHandler zu machen ist ebenfalls eine eigenart von ReSharper. Richtig wäre jedoch _displayHandler. Die Schreibweise mit dem „m_“ ist ebenfalls veraltet.
  4. Get/Set Methoden erinnern mich etwas an die (gute) alte Java Zeit. C# kann aber Properties. Somit werden solche Konstrukte obsolet.

Mein Vorschlag zu diesem Code:

protected override CefDisplayHandler DisplayHandler { get; }

Desweiteren fällt mir auf, dass alle Dateien einen Lizenzhinweis haben.

Sorry Kola, aber eine LICENSE Datei im root Ordner ist eigentlich ausreichend 😉

Namings

Ich kenne die Domäne jetzt nicht besonders gut (eigentlich gar nicht 🙂 ) aber beim stöbern, fällt mir da doch das eine ander andere auf.

Schauen wir uns mal die Datei ChromelyJsHandler an. Der Name „Handler“ sugerriert, als ob da irgendwas passiert. Es handelt sich hierbei aber eher um ein Value Object ohne jegliche Logik. Da fällt mir gerade noch auf: Warum ist es eine Klasse? Ein Struct ist schneller und verbraucht i.d.R. weniger Resourcen.

Viele der Klassen beginnen mit „Chromely“. Es ist nicht mehr so wirklich zeitgemäß und sollte entfernt werden.

Magic Value Database

Erstaunt war ich über den MimeMapper von Kola. Eine Methode über 2250 Zeilen Quellcode. Ja, richtig gelesen. Zweitausendzweihundertfünfzig Zeilen. Die Methode ist zwar trotz der länge extrem übersichtlich, trotzdem würde ich so etwas nie machen.

Hier wäre eine Datenbank angebracht. Selbst eine csv Datei wäre es besser, als dieses Koloss.

Reinventing the wheel

Mir ist schon bewusst, dass Kola eine Leichtgewichtige alternative entwickeln möchte. Mir ist auch bewusst, dass es sich um ein .NET Standard2.0 Projekt handelt was wir vor uns haben.

Was ich aber nicht vestehe ist, warum werden hier Funktionalitäten neu erfunden?

Als Logger kannst log4net verwendet werden. Ein guter IoC Container ist Ninject. Das ganze REST Zeug, kannst RestSharp sehr gut bewältigen. Und Jsons können mit Newtonsoft.Json problemlos geparsed werden.

Als Software Entwickler neigt man schnell dazu das Rad neuerfinden zu wollen. Allerdings hat man nach einer Zeit mehr Schaden als Nutzen. Eine gut gepflegte Bibliothek ist immer besser als etwas eigenes. Den die eigene Komponente muss gepflegt und gewartet werden.

Im heutigen Zeitalter, werden Komponenten wiederverwendet. Es gibt viele Projekte, die standardisierte NuGet Pakete verwenden.

Klar, leichtgewicht hin oder her, aber zumindest sollte man dem user die Möglichkeit geben sich anders zu entscheiden. Also separat eine Projekt erstellen, welches dafür sorgt eine etablierte Komponente zu verwenden.

Sehr gute Beispiele sind hierfür Ninject oder auch NancyFx. Besonders Nancy ist ein Framework, dass alles kann, aber nichts richtig. Erst mit den nötigen Erweiterungen wird es sehr, sehr möchtig. Damit gehört es zu meinen Lieblingsframeworks 🙂

Allgemein

Wie bereits erwähnt, ich kenne die Domäne nicht. Aber irgendwie wirkt der Code auf mich etwas unstrukturiert.

Die Methoden sind teilweise etwas zu lang und die langen, unnötigen Kommentare tragen zur unübersichtlichkeit bei.

Mir fällt es auch schwer den Haupteinstiegspunkt in der Applikation zu finden. Vielleicht ist es für jemanden, der sich da etwas besser aus kennt leichter. Für mich ist es aber etwas schwierig. Ich würde mich nicht in diesen Code einarbeiten wollen 🙂

Fazit – Chromely

Für ein studierten Mann, der sich .NET Consultant (Berater) nennt, finde ich den code etwas mies.

Verstehe mich nicht falsch, er hat viele gute Ansätze. Aber er ist relativ schwierig zu lesen.

Während des Code Reviews, bin ich nicht auf alle Sachen eignegangen, die mir so aufgefallen sind. Das was ich aber erwähnt habe, waren mir persönlich die wichtigsten.