Code Reviews
06.01.2009 Permalink Seit ich Software entwickle, hat sich nichts daran geändert, dass der Code das langlebigste maßgebliche Resultat von Softwareprojekten ist. Dokumentation ist wichtig, nutzt aber wenig, wenn die Codebasis faulig oder zerbrochen ist, denn die Mühe bei der Fehlersuche oder Erweiterung wächst und oder schrumpft mit dem Grad der Verrottung des Codes und nicht mit der Menge Prosa, mit dem das (traurige) Werk beschrieben ist.Toolgestützte, statische Codeanalyse z.B. bzgl. Paketabhängigkeiten oder CCN hilft, manche Auffälligkeiten sehr schnell zu finden. Aber eben nicht alle. So Dinge wie chaotisches Logging mit mißverständlichen Meldungen, löchriges Exception-Handling, mögliche Race Conditions, subtile Formen von Codeduplikation, Algorithmen mit hoher Laufzeitkomplexität, Overengineering oder schlicht aussageloses Javadoc sind auch 2009 kaum durch Maschinen aufdeckbar. Und die genannten Probleme sind meist nicht im Handstreich zu beseitigen, sondern können eine teure Überarbeitung erfordern.
Es gibt leider manche Programmierer, die diese Fehler haarsträubend konsequent einbauen. Das sind meist diejenigen Kollegen, die noch lernen. Solche Programmierer sind nicht in Ihrem Team? Sicher? Na, dann führen Sie wohl öfter Code Reviews durch, denn die Erfahrung ist im Code ablesbar, so einfach ist das. Nie Reviews durchzuführen heißt dagegen, auf Glück zu bauen, was in der Softwareentwicklung selten gut geht.
In den Code Reviews, die ich durchführe, finde ich die folgenden Punkte besonders häufig:
- Exception Handling ist uneinheitlich, Exceptions werden verschluckt, an zu vielen Stellen geloggt oder überall gefangen und weitergeworfen.
- Klassen, die von mehreren Threads verwendet werden, taugen nicht dafür und erzeugen nicht reproduzierbare Fehler.
- Ressourcen wie Connections oder Streams werden nicht sicher freigegeben.
- Referenzen werden nicht aus statischen Instanzvariablen entfernt und erzeugen Speicherlecks.
- Singleton Anti-Pattern wird verwendet, mindert so Testbarkeit und begünstigt Speicherlecks.
- Synchronisierte Klassen (z.B. Vector, StringBuffer) werden unnötigerweise verwendet, und beeinträchtigen Performanz.
- Paketabhängigkeiten weisen Zyklen auf oder geben die Architektur nicht wieder.
- Code wird dupliziert.
- Komplexität der Lösung geht weit über das Problem hinaus.
- Imports, Klassen, Methoden und Variablen werden nicht verwendet und verstellen die Sicht auf das Notwendige.
- Kommentare sind nicht hilfreich, missverständlich oder falsch.
- JDK 5 Syntax (z.B. Enums, for mit Collections oder Generics) wird nicht verwendet.
- Loglevel werden uneinheitlich verwendet, Meldungen sind wenig hilfreich.
- Klassennamen enden auf ...Helper oder ...Util und enthalten meist verlegenheitsplatzierten Code.
- ein Signal, dass Qualität zählt,
- eine lernende Entwicklungsmannschaft,
- ein Bild von der inneren Codequalität,
- nach Beheben der gefundenen Punkte besseren Code und
- Kostenersparnis, weil die so gefundenen Fehler nicht den Umweg über den Systemtest gehen müssen.
- einen oder mehrere Prüfer für wenige Personentage,
- zu untersuchenden Code und
- eine Tabelle oder Bugtracking Tool, um die Fundstellen zu protokollieren.
Was ist mit der Prüfgrundlage, bspw. ein Code Styleguide, oder eine projektspezifische Beschreibung, worauf zu achten ist? Hm, schwierig. Schon sinnvoll, wenn es Eigenheiten des Projekts gibt, die sich niemand selbst herleiten kann, z.B. dass alle Instanzvariablen mit einem Unterstrich beginnen sollen, oder man immer Spaces statt Tabs zu verwenden hat. Aber dieser Styleguide kann nur ein Add-on auf den gesunden Verstand sein, sonst müsste man statt den Code zu prüfen erstmal eine dreibändige Abhandlung über das Programmierhandwerk verfassen. Vielmehr Sinn macht dagegen der Verweis auf einige Bücher, z.B.:
- J.Bloch - Effective Java: A Programming Language Guide
- M.Fowler et al - Refactoring
- B.Goetz et al - Java Concurrency in Practice
- R.C.Martin - Agile Software Development