Principle of Most Astonishment
Published on Monday, November 7, 2016 5:00:00 AM UTC in Code Reviews & Skit
Manch Entwickler hätte sich längst in der Unfallstatistik verewigt, sähe er im Straßenverkehr ebenso selten nach links oder rechts wie beim Programmieren. Dabei ist grade das Abgucken bei anderen doch eines der wichtigsten Hilfsmittel zum Lernen und zur Weiterentwicklung. Mit Scheuklappen fällt es aber schwer, Muster zu erkennen und für sich selbst zu adaptieren.
Hinweis: Code Reviews im Tagesgeschäft sollen in angenehmer Atmosphäre ohne Druck stattfinden und positiv bestärken. Code Reviews auf meiner Seite hingegen überspitzen, machen lächerlich und wollen unterhaltsam sein. Jeder macht Fehler (ich sicherlich mehr als manch anderer), niemand soll bloßgestellt werden. Aus diesem Grund sind in meinen Posts Code-Fragmente anonymisiert und verkürzt. Außerdem werden Personen oder Projekte nicht beim Namen genannt oder verlinkt. Alle Beispiele stammen jedoch aus realen Projekten.
Der gesamte folgende Quelltext stammt aus der Feder eines Entwicklers mit deutlich mehr als 10 Jahren .NET-Erfahrung und ist - erstaunlicherweise - zeitgleich im Paket entstanden.
var uow = _unitOfWorkFactory.CreateDefaultUnitOfWork();
uow.StartTransaction();
if (TrySomething(uow))
{
DoSomethingElse();
}
Für den Leser nicht offensichtlich ist, dass der erste Fehler schon darin besteht, die Variable uow
(Unit of Work) nicht freizugeben. Das erschließt sich dem Entwickler nur, wenn er entweder schon jahrelang mit der Codebasis arbeitet check oder Intellisense benutzt check oder gar Zugriff auf den Quelltext der Unit of Work hat check - upps.
Was aber direkt ins Auge springt ist, dass zu dem Aufruf StartTransaction
scheinbar das passende Gegenstück fehlt. Kein Commit, kein Rollback weit und breit. Um vorwegzugreifen: das Gegenstück befindet sich in der Methode TrySomething
- wo es eigentlich nicht hingehört, zumindest aber für Verwirrung sorgt.
Tatsächlich ist die Methode TrySomething
auch der interessantere Teil des Codes. Was ist die Erwartungshaltung eines .NET-Entwicklers, wenn er auf eine Methode stößt, die mit Try...
beginnt und einen bool
-Wert zurückgibt? Richtig: es handelt sich um eine defensive Variante, bei der eine bestimmte Aktion versucht wird, die im Fehlerfall nicht zu einer Ausnahme führt. In der Realität sieht aber eine derartige Methode durchaus auch mal so aus:
bool TrySomething(IUnitOfWork uow)
{
try
{
// try something
uow.Commit();
return true;
}
catch (Exception ex)
{
uow.Rollback();
// log error
throw;
}
}
Von dem schon angesprochenen Problem abgesehen, dass die Transaktion außerhalb der Methode gestartet, aber innerhalb abgeschlossen wird, gibt es das ausgesprochen unerwartete Verhalten, dass lediglich der erfolgreiche Code-Pfad zu einem validen Rückgabewert führt. Bei einem Fehler wird tatsächlich eine Exception geworfen - womit im Übrigen auch die if
-Abfrage des Aufrufers überflüssiger wird als ein elektrischer Eierkocher.
In Summe existieren hier auf engstem Raum also gleich mehrere unschöne Details, vom vergessenen Freigeben von Ressourcen bis zum mehrfachen Verletzen des Principle of Least Astonishment: Jemand beendet stillschweigend die Transaktion, die ich als Außenstehender gestartet habe? Eine Methode erzeugt eine Ausnahme, obwohl der Name anderes suggeriert? Ein boolscher Rückgabewert kann immer nur true
annehmen, eine if
-Abfrage gar keine tatsächliche Entscheidung treffen?
D'oh!
Tags: .NET · C# · Codegewordenes Gammelfleisch