Erst kürzlich machte mich ein Kollege von mir per Email auf einen Post von Neal Ford aufmerksam. Neal bahauptet und begründet das Kommentare angeblich ein Code Smell seien. Dieser Aussage stelle ich mich strikt dagegen und behaupte, das Kommentare im Code im Allgemeinen kein Code Smell sind.
Sind Inline-Kommentare schlecht ?
Neal behauptet, Kommentare wären ein Bruch des DRY-Prinzips, denn schließlich hätte man das essentielle ja schon im Code. Diesen nochmals zu kommentieren wäre eine Wiederholung dessen, was man ohnehin schon programmiert hätte.
Ganz so einfach ist das leider nicht. Es gibt viele Situationen, in denen Kommentare in gut geschriebenem Code hilfreich sind. Die pauschale Aussage, Kommentare würden gegen DRY verstoßen, ist schlichtweg naiv. Wichtig bei Inline-Kommentaren ist es, darüber zu schreiben, wieso man den Code in der gegebenen Form aufgebaut hat. Eine “stupide” Wiederholung der Statements wie z.B.
/* create new account */ IAccount account = new Account();
ist sicherlich eine unnötige Wiederholung. Aber ist dadurch der Code (also IAccount account = new Account();) automatisch schlecht? Ich würde sagen: Nein.
Das Inline-Kommentare auch besonders wertwoll sein können, verschweigen die meisten. Zumindest diejenigen, die behaupten Kommentare seien ein Code Smell. Nehmen wir z.B. folgendes Code-Fragment aus einem HttpModule:
private void ResetSessionCookieDomain(HttpApplication application)
{
string host = application.Request.Url.Host;
if (application.Session.IsNewSession && !application.Request.Url.IsLoopback)
{
HttpCookie appCookie = application.Request.Cookies[this.GetApplicationCookieName()];
if (appCookie != null)
{
appCookie.Domain = this.GetCookieDomain();
}
}
}
Auf den ersten Blick sieht das ja schon ziemlich aufgeräumt und verständlich aus. Aber der erste Blick trügt. Denn diese Methode wird mindestens drei mal unterschiedlich interpretiert werden.
Derjenige Entwickler, der wenig oder keine Erfahrung mit ASP.NET hat, wird den Code so hinnehmen wie er ist und ihn falsch verstehen. Er wird annehmen, das man in ASP.NET Cookies ausschließlich über das HttpRequest-Objekt zugreift und dort ändern, erstellen oder löschen kann.
Derjenige Entwickler, der sehr große Erfahrung in ASP.NET hat, wird den Code auch so hinnehmen wie er ist und ihn richtig interpretieren (dazu später mehr).
Derjenige Entwickler, der einige Erfahrungen in ASP.NET gesammelt hat, wird sich über den Code wundern und sich selbst die Frage stellen, wieso der Entwickler der den Code geschrieben hat gerade über HttpRequest das Cookie verändert. Schließlich gibt es ja den HttpResponse, der auch eine Cookies-Eigenschaft hat (HttpCookieCollection).
Im Ergebnis hat es die obige “saubere”, unkommentierte Methode geschafft, in drei verschiedenen Köpfen verschiedene Ausrufe- und Fragezeichen zu hinterlassen. Dabei wäre es ziemlich einfach, diese Mehrfachinterpretation durch einen kurzen Kommentar zu vermeiden:
private void ResetSessionCookieDomain(HttpApplication application)
{
string host = application.Request.Url.Host;
if (application.Session.IsNewSession && !application.Request.Url.IsLoopback)
{
/* Careful: when handling cookies using the response object new cookies are set.
* The cookie collections in the Request and Response objects are always synchronized in .Net.
* The request object is used to reset the domain and avoid generating a new cookie accidentally. */
HttpCookie appCookie = application.Request.Cookies[this.GetApplicationCookieName()];
if (appCookie != null)
{
appCookie.Domain = this.GetCookieDomain();
}
}
}
Ist das jetzt ein Code Smell? Neal behauptet ja, denn für Ihn ist die einzige Rechtfertigung für Inline-Kommentare ein komplexer Algorithmus. Ich behaupte: Nein, das ist kein Code Smell, sondern vielmehr eine Bereicherung des Codes. Ich bin mir fast sicher, das auch er den Kommentar schätzen würde, wenn er mit dem Code konfrontiert wird.
Kommentare: Störfaktor oder Dokumentationsbasis ?
Ein weiteres Argument von Neal gegen Kommentare – genauer gesagt gegen kommentierte Methoden im JavaDoc oder NDoc (<summary/>) Stil – ist der Störfaktor, der von Kommentaren ausgeht, wenn man refaktorisiert. Als ich das gelesen habe, konnte ich mir ein Schmunzeln nicht verkneifen. “Was ist denn das für eine fadenscheinige Ausrede” habe ich mir gedacht. Natürlich sind Kommentare, aus denen Dokumentationen generiert werden ein zweischneidiges Schwert. Auf der einen Seite sind sie ein weiteres, zu erstellendes und pflegendes “Code-Fragment”, auf der anderen Seite blähen sie den Sourcecode um etliche Zeilen auf und lenken von dem eigentlichen Code ab.
Ich persönlich habe bis Dato keinen Entwickler getroffen, der beim Entwickeln von Code gleichzeitig dokumentationsbasierte Kommentare erstellt hat. Es macht ja auch faktisch keinen Sinn, wenn man als Entwickler davon ausgeht, das man den Code sowieso noch mit großer Wahrscheinlichkeit ändern oder umstrukturieren muss. In einigen Fällen macht es sogar gar keinen Sinn, z.B. wenn man Prototypen entwickelt oder kleine, temporär verwendete Tools.
In einem professionellen Umfeld jedoch, wo Sourcecode ein oder sogar das einzig große Kapital eines Unternehmens ist, sind Dokumentationen allgemein ein hohes Gut. Dazu gehören nun mal auch die dokumentationsbasierten Code-Kommentare. Als Entwickler oder Entwicklungs-Team muss man sich natürlich die Frage stellen, wann und wo solche Kommentare einen effektiven Mehrwert liefern. Zumeist sind es in allererster Instanz hochgradig wiederverwendete Komponenten und Klassen, z.B. in Frameworks oder aber auch Schnittstellen. Ist der Sourcecode auch noch Teil eines Produktes, wie z.B. bei einer API, so muss dieser auf jeden Fall gut und ausreichend ausführlich kommentiert werden.
Hat man nun z.B. ein solches, gut kommentiertes Framework vor sich und muss es erweitern oder verändern, so ist es keinesfalls ein Grund, eine notwendige Refaktorisierung nicht oder nur unvollständig durchzuführen, weil die Kommentare existieren. Jeder, der sich selbst bei so einer Situation ertappt, das er nur den Code refaktorisiert oder garnicht refaktorisiert, weil er die “lästigen” Kommentare auch noch überarbeiten muss, sollte sich die Frage stellen, ob er den Ansprüchen in professionellen Umgebungen gerecht werden kann.
Unit Tests vs. Kommentare
Eine interessante Argumentation von Neal ist, das Tests den Code wesentlich besser dokumentieren als Kommentare. Dabei stelle mir die Frage, was denn Tests und Kommentare eigentlich gemeinsam haben. Verifizieren Kommentare die Funktionstüchtigkeit einer Methode? Dokumentieren Tests wieso ein Entwickler die Klasse oder Methode implementiert hat? Ich jedenfalls habe keines von beiden in der Art und Form durchführen können.
Neal stützt sich bei der “Unbrauchbarkeitsrechtfertigung” von Kommentaren nicht nur auf klassische Unit Tests, sondern auch auf den Einsatz von BDD-Frameworks wie z.B. NBehave oder StoryQ. Diese “ausführbaren” und damit auch testbaren Spezifikationen sind eine gute und besonders hilfreiche Bereicherung für die Software-Entwicklung. Ich habe NBehave schon eingesetzt und bin positiv angetan von den neuen Möglichkeiten, die solche Frameworks für die Software-Entwicklung eröffnen.
Kommentare im Kontext
Jedoch ist es keinesfalls sinnvoll, Unit Tests oder BDD mit Code-Kommentaren zu vergleichen. Der gemeinsame Nenner, den alle drei Dokumentationsformen haben, ist schlichtweg die Intention, den Code zu dokumentieren.
Dabei konzentrieren sich BDD-Stories auf die funktionale Spezifikation der zu erstellenden Software. Unit-Tests dokumentieren die funktionale und/oder technische Verifikation von Software. Code-Kommentare dienen der Beschreibung und Vermittlung essentieller technischer Implementierungsdetails von Software. Zusammengefasst also drei verschiedene Dokumentationsformen, die alle ein anderes Ziel verfolgen. Aus meiner Sicht sind sie nicht miteinander vergleichbar.
Aber überall wird behauptet, Kommentare seien “smelly”…
…jedoch ist das nicht ganz richtig. Denn “überall” wird das sicher nicht behauptet – ein Gegenbeispiel liest Du gerade. Aber es ist richtig, das es neben Neal noch weitere, z.T. hoch angesehene Quellen gibt, die behaupten, Kommentare seien ein Code Smell. Ein sehr prominentes Beispiel ist Jeff Atwood; in seiner Code Smells Liste sind Kommentare ganz oben.
Es lohnt sich hier jedoch, die Aussage genau zu lesen und auch kritisch zu hinterfragen. Denn in den meisten Fällen werden Kommentare nur als “Code Smell” deklariert, weil man entweder schlechte Erfahrungen mit Kommentaren gemacht hat oder selbst keine guten Kommentare schreiben kann. Und schon wird aus einem für mein Dafürhalten wichtiges Artefakt wie den Kommentaren ein Warnsignal für schlechten Code gemacht. Auch hier gibt es wieder einen feinen Unterschied, denn ein Code Smell ist für mich ein echtes Warnsignal, das etwas schief läuft. Die Argumentationen vieler, die Kommentare als Code Smell sehen, berufen sind jedoch kaum auf schlechten Code-Stil als auf allgemeine “Probleme” mit Kommentaren.
So wird oft zitiert, das Kommentare falsch geschrieben werden, wenn Sie nicht das “Warum?” erklären, sondern einfach nur beschreiben was gerade passiert. Es wird behauptet, Kommentare würden einen in die Irre führen, weil sie falsch oder nicht gepflegt sind. All das sind allerdings generelle Probleme des kommentierens, weniger ein Problem des kommentierten Codes.
Im Übrigen gibt es genügend Entwickler, Artikel und Bücher, die sich mit der Thematik Inline- und Code-Kommentare auseinandersetzen. Zum Beispiel gehen Andrew Hunt und David Thomas in ihrem legendären Buch The Pragmatic Programmer auf Kommentare und Dokumentationen in Punkt 44 “It’s all Writing” ein und erklären verständlich, das gutes kommentieren von Source Code unerläßlich für gute Qualität und die weitere Entwicklung ist.
Ich möchte natürlich auch Martin Fowler und sein Buch Refactoring nicht unerwähnt lassen. Fowler deklariert dort Kommentare als Code Smell – liest man jedoch seine Argumentation, warum Kommentare ein Code Smell sein können, landet man schnell wieder bei der Erkenntnis, das es hier um falsche Kommentare geht, und nicht um schlechten Code. Er gibt im Buch dennoch die folgende konrekte Handlungsanweisung:
When you feel the need to write a comment, first try to refactor the code so that any comment becomes superfluous.
Entscheidend für mich ist der Anfang des Satzes: “When you feel the need to write a comment…”. Es geht ihm also nicht darum, Kommentare zu eliminieren oder aber den Sinn von Kommentaren zu hinterfragen, sondern vielmehr um die Tatsache, das man versuchen sollte, den Code so gut wie möglich selbsterklärend zu schreiben. Das dies nicht immer der Fall sein kann oder in manchen Situationen nicht adequat ist, ist für Fowler selbstverständlich. Aus meiner Sicht ist die obige “best practice” eine gute – aber keine schlüssige Argumentation für einen Code Smell.
Kommentare sind gut und wichtig
Aus all den Betrachtungen heraus kann ich für mich nur den Schluß ziehen, das Kommentare gut und wichtig für qualitativ hochwertige und wartbare Software sind. Natürlich sollte – wie bei so vielen Dingen in der Programmierung – auf den richtigen Einsatz und die richtige Dosierung von Kommentaren geachtet werden. Wie auch für den Code selbst gilt für Kommentare, das sie erstellt und gepflegt werden müssen. Manchmal muss man sie auch löschen oder verbessern – genauso wie beim Code selbst. Entscheidend für mich beim Kommentieren ist, das ich es mindestens so durchführe, wie ich es beim ersten Lesen meines eigenen Codes erwarten würde, damit ich den Code schnell und gänzlich verstehe.
Fazit: Comments are a good thing (if you know how to use them).
byRYErnestonNovember 30th 2008Nice post u have here
Added to my RSS reader
byIlker CetinkayaonNovember 23rd 2008Hi Tobi,
zunächst einmal: Good Code!
Da sieht man mal, wie wichtig es ist, gute Kommentare zu schreiben und diese auch richtig zu lesen. Fakt ist, das Du mit der “Synchronisation” von HttpRequest.Cookies und HttpResponse.Cookies recht hast – sie werden nicht synchronisiert. Allerdings scheint es mir hast Du den Kommentar falsch interpretiert – es geht nicht um die Erstellung eines neuen Cookies, sondern um die Modifikation.
Doch auch bei der Modifikation muss ich Dir natürlich recht geben – immer über HttpResponse.Cookies gehen! Ich behaupte jedoch weiterhin, das der gezeigte Code richtig ist. Wie kommt’s?
Tja, gerade das, was Du im Kommentar kritisierst ist wohl der entscheidende Hint: die Methode heisst “ResetSessionCookieDomain”. Soll heissen, die Methode setzt den Domain-Wert des Session-Cookies zurück. Man beachte auch, das der gesamte Code wie erwähnt in einem HttpModule abläuft. Die einzige wichtige Info, die ich nicht mitgegeben habe ist, das die Methode effektiv aufgerufen wird, nachdem der SessionState von der HttpRuntime erzeugt wurde (siehe AquireRequestState / ReleaseRequestState). **In genau diesem Kontext** ist es korrekt, den Session-Cookie über HttpRequest.Cookies zu verändern. Genau genommen ist sogar der Kommentar **in genau diesem Kontext** richtig – obwohl er wirklich irreführend ist.
Denn genau auf die selbe (richtige) Art und Weise, wie Du in Deinem refaktorisierten Code machst, setzt intern der SessionStateManager bzw. der SessionIDManager das Cookie-Handling um. Es nimmt also als Ausgangsbasis HttpRequest.Cookies und modifiziert es später über HttpResponse.Cookies. Möchte man nun den Session-Cookie selbst noch modifizieren, so bleibt einem nichts anderes übrig, als das über HttpRequest.Cookies zu tun – schließlich möchte man verhindern, das die internen Modifikationen von ASP.NET ausgelassen werden. Am Ende der ASP.NET Pipeline (genauer: zu ReleaseRequestState), wird dann der Cookie über HttpResonse.Cookies intern aktualisiert.
Des Weiteren finde ich die Argumentation, das ein Methodenname die Parameter der Methode reflektieren soll nicht gerade erstrebenswert. Würde man dies stringent umsetzen, hätte man schnell zum Einen ellenlange Methodennamen und zum Anderen würde man den Fokus von dem eigentlichen Zweck – dem “was macht die Methode” – ablenken und abweichen.
Die refaktorisierte Variante von Dir ist sehr schlüssig und übersichtlich. Die Fragestellung für mich ist es nun, welcher der zwei Code-Varianten lesbarer und übersichtlicher ist. Ich persönlich tendiere immer noch zu meinem gezeigten Beispiel – obwohl Deines, wie Du schon richtig anmerkst, eine bessere Separation of Concerns gewährleistet. Entscheidend ist für mich der Kosten/Nutzen-Effekt. Deine Variante ist in Relation zum Aufwand, den Du betreiben musstest, nicht sondernlich lesbarer oder einfacher – obwohl in objektorientiertem Sinne formal korrekter.
Bei der Entscheidung zur Refaktorisierung sollte man meiner Meinung mach nicht nur den Aspekt der Korrektheit in betracht ziehen, sondern andere Faktoren wie Einfachheit, Isolationsgrad und Chattiness beachten. Nichtsdestotrotz – ich finde Deine Variante äußerst elegant – wenngleich nicht lesbarer oder einfacher. Gerade Deine Refaktorisierungsbemühungen bestätigen meine Behauptung, das Kommentare per se kein Code Smell sind. Im Bestreben, den Kommentar überflüssig zu machen, hast Du meiner Meinung nach die Komplexität erhöht und die Lesbarkeit gemindert.
Dennoch gebe ich zu, das auch mein gezeigter Code (und der Kommentar) nicht gerade gelungen ist. Der Kommentar enthält die Falschaussage “always synchronized” – das ist schon mal ein dicker Minuspunkt. Es ist besser, einen Kommentar ganz wegzulassen, anstatt einen falschen Kommentar zu schreiben – Shame on me!
Des Weiteren würde ich an dem ersten Code-Beispiel die mangelnde Konvention in der Benennung der Methoden anmahnen. Während die Methode selbst “ResetSessionCookieDomain” heisst (IMHO korrekterweise), ist der Name der Methode zum Ermitteln des Cookienamens “GetApplicationCookieName” – aus meiner Sicht sollte das wohl eher zu “GetSessionCookieName” umbenannt werden.
Am liebsten würde ich jetzt das Code-Beispiel im Post korrigieren – ich lasse es dennoch so falsch wie es ist stehen, damit man auch unsere Kommentierungen nachvollziehen kann.
Gruß,
Ilker
byTobionNovember 20th 2008Hallo,
der Kommentar in Deinem Codebeispiel ist so entweder von mir falsch verstanden worden, oder sagt etwas aus, was nicht korrekt ist. Die Cookie Collections in Request und Response sind nicht synchronisiert. Ganz im Gegenteil. Der Unterschied zwischen den beiden Collections ist, dass im Request alle vom Client geschickte Cookies, plus transidiente Cookies vorgehalten werden und in Response ausschließlich neue Cookies vorgehalten werden, die im Response im Set-Cookie Header mitgesendet werden. Korrekt ist allerdings, dass Cookies, die über Response.Cookies.Add gesetzt werden anschließend zusätzlich in Request.Cookies auftauchen. Um den Unterschied noch weiter zu verdeutlichen: Cookies, die über Request.Cookies.Add hinzugefügt werden, sind sogenannte transidiente Cookies – das heißt, sie stehen der restlichen Verarbeitungskette zur Verfügung – werden allerdings anschließend NICHT an den Client gesendet. Man kann das verifizieren, indem man Request.Cookies.Add aufruft und dabei beobachten kann, dass Response.Cookies.AllKeys keine neuen Keys enthält.
Zusätzlich finde ich, dass Dein Codebeispiel gerade ein ideales Beispiel dafür ist, dass man hellhörig (= potential Code Smell) werden muss, wenn man das Bedürfnis hat einen Kommentar zu schreiben.
Ich sehe da durchaus Verbesserungspotential. Denn zum Beispiel passt die Methodensignatur nicht zum Methodennamen. Es ist im Methodennamen von Cookies die Rede – es wird allerdings eine HttpApplication übergeben. Das passt nicht zusammen. Zusätzlich wird in der Methode auf IsNewSession geprüft – davon steht ebenfalls nichts im Methodenname.
Ich hätte Dein Codebeispiel wie folgt refactored (ich hoffe mal, dass das formatiert bleibt):
private void SetApplicationCookieDomainOnNewSession(HttpApplication application) { string host = application.Request.Url.Host; if (application.Session.IsNewSession && !application.Request.Url.IsLoopback) { CookieHandler cookieHandler = new CookieHandler(application); HttpCookie appCookie; if (cookieHandler.TryGetExistingCookie(this.GetApplicationCookieName(), out appCookie)) { appCookie.Domain = this.GetCookieDomain(); } } } public class CookieHandler { public CookieHandler(HttpApplication application) { Application = application; } public HttpApplication Application { get; private set; } /// /// Tries to retrieve an existing cookie. /// /// The name of the cookie to get /// The cookie, if is true, otherwise null /// True if a cookie was found, otherwise false. public bool TryGetExistingCookie(string cookieName, out HttpCookie appCookie) { try { HttpCookie cookie = Application.Request.Cookies[cookieName]; if (cookie != null) { appCookie = cookie; return true; } } catch (Exception ex) { LOG.LogException(ex); } appCookie = null; return false; } }Bessere SoC. Man beachte auch, dass ich wenn es um Dokumentationskommentare geht absolut für Kommentare bin (Intellisense, API Doku, …)
Trotzdem kann ich erstmal nur zustimmen: When you feel the need to write an (inline) comment, first try to refactor the code so that any comment becomes superfluous.
Denn das Refactoring hat zumindest in meinen Augen den inline(!) Kommentar ersetzt. Und wenn wir jetzt auch noch 3.5 einsetzen, dann können wir es noch sprechender gestalten:
public void DieAufrufendeMethode(HttpApplication application) { ExecuteOnIsNewSessionSafe(application, () => { CookieHandler cookieHandler = new CookieHandler(application); ResetApplicationCookieDomain(cookieHandler); } ); } private void ResetApplicationCookieDomain(CookieHandler cookieHandler) { HttpCookie appCookie; if (cookieHandler.TryGetExistingCookie(this.GetApplicationCookieName(), out appCookie)) { appCookie.Domain = this.GetCookieDomain(); } } private void ExecuteOnIsNewSessionSafe(HttpApplication application, Action action) { if (application.Session.IsNewSession && !application.Request.Url.IsLoopback) { action(); } }Dann ist endlich das HttpApplication aus der Methodensignatur draußen. (DieAufrufendeMethode ist dabei die Methode wo vorher ResetSessionCookieDomain(HttpApplication application) aufgerufen wurde.)
Viele Grüße,
Tobi