Checkstyle: Lösungsvorschläge für Design for Extension

[Jay_030]

Ich wollte nicht Leichenflederer spielen und in diesen Thread schreiben. Daher ein neues Thema dazu. Wenn die Moderatoren das anders sehen, kann man die Threads ja immer noch mergen. (Hoffe ich. ^^)

Ich nutze seit kurzem Checkstyle über Sonar und da ist mir diese Meldung aufgefallen: Design for Extension. Kurzes Googeln hat mir gezeigt, dass ich nicht der Einzige bin, der darüber stutzt. Wie aus diesem Thread ersichtlich ist, arbeite ich neuerdings mit dem DI-Framework Guice. Das heißt (grob vereinfacht für alle Nicht-DIler), ich habe eine ganze Reihe von Interfaces und dahinter jeweils eine Implementierung. Bei den Methoden der Impl-Klassen zeigt mir Checkstyle dann diese Meldung an. Auch bei den equals-, hashCode- und toString-Methoden.

Eine Lösung wäre, die Impl-Klassen zusätzlich final zu deklarieren, um nicht bei jeder Methode final dranklatschen zu müssen. Die Idee hinter dieser Regel ist ja nicht schlecht, aber irgendwie missfällt mir das vom "Codeaussehen" (was wahrscheinlich subjektiver Quatsch ist).

Wie geht ihr mit dieser Regel von Checkstyle um? Habt ihr sie ausgeschaltet oder ignoriert ihr sie einfach?

Was mich besonders interessiert: Ist es üblich, wenn es nur eine Implementierung für ein Interface gibt, diese als final zu deklarieren?

 

[Gast2]

Grundsaetzlich definiere ich alle Klassen als final wenn sie nicht explizit fuer eine Vererbungshierachie designed sind. Das ist soweit auch best practice, siehe z.B. Effective Java von J. Bloch

Oder hab ich dein Porblem jetzt falsch verstanden?

 

[Jay_030]

Grundsaetzlich definiere ich alle Klassen als final wenn sie nicht explizit fuer eine Vererbungshierachie designed sind. Das ist soweit auch best practice, siehe z.B. Effective Java von J. Bloch

Oder hab ich dein Porblem jetzt falsch verstanden?

Ja genau. Bisher habe ich nur Helper-/Utility-Klassen als final deklariert. Sollte wohl doch nochmal in Effective Java reinschauen. Die Stelle habe ich nicht mehr in Erinnerung. Best Practice geht über persönlichen Geschmack imho. Muss ich mich halt umgewöhnen. Von der Logik macht das final da ja auch Sinn.

 

[Gast2]

Musst du dir mal die Items 16,17 und 18 ansehn. Da wird auf Inhertiance eingegangen.

 

[tfa]

Wie geht ihr mit dieser Regel von Checkstyle um? Habt ihr sie ausgeschaltet oder ignoriert ihr sie einfach?

Ausschalten. Am besten Checkstyle ganz abschaffen. Das ist reine Zeitverschwendung. Nicht-wertschöpfendes Mikromanagement.

Die meisten Checks prüfen auf bloße Trivialitäten. Was ist schlimm daran, wenn eine Quelltextdatei nicht mit einem Zeilenvorschub endet oder Tabs enthält? Oder wenn man Leerzeichen vor oder hinter Typecasts oder Operatoren vergessen hat; nach dem letzten Arrayelement ein Komma kommt? Wird dadurch die Software besser?

Diese Stilfragen (Checkstyle eben) kann am einfachsten erschlagen, indem man in der IDE einheitliche Formatierungsrichtlinien festlegt und bei jedem Speichern ein Autoformat ausführt. Viele einfache Prüfungen kann mittlerweile auch Eclipse selbst erledigen.

Um Codemetriken zu untersuchen gibt es spezialisierte Tools, die man hin und wieder mal laufen lassen kann, wenn glaubt, damit Probleme zu haben. Aber ein routinemäßigen Test? Hier sollte es auch nur Richtilinien geben und keine harten Grenzen. Was, wenn eine (wunderbar geschriebene) Methode statt der erlaubten dreißig 31 Zeilen hat? Die Methode auseinander reißen oder das Limit erhöhen?

Die halbwegs sinnvollen Checks (und vieles mehr) leisten auch andere statische Analysetools wie Findbugs. Findbugs nimmt nicht die Quelltexte auseinander sondern arbeitet direkt mit Class-Files. Und was daran an Bugs automatisch gefunden wird, ist echt faszinierend. Wer es noch nicht kennt, sollte es unbedingt ausprobieren. Dann wandert Checkstyle schnell in die Tonne.

 

[stareagle]

Hallo,

meiner Meinung - andere mögen anderer Meinung sein - ist Checkstyle ein wenig zu kleinkariert. Persönlich verwende ich PMD und FindBugs. Mit den beiden Tools sind meiner Meinung nach die meisten möglichen Problemstellen gut abgedeckt.

Gruß

Stareagle

 

[Gast2]

Ausschalten. Am besten Checkstyle ganz abschaffen. Das ist reine Zeitverschwendung. Nicht-wertschöpfendes Mikromanagement.
[...]
Diese Stilfragen (Checkstyle eben) kann am einfachsten erschlagen, indem man in der IDE einheitliche Formatierungsrichtlinien festlegt und bei jedem Speichern ein Autoformat ausführt. Viele einfache Prüfungen kann mittlerweile auch Eclipse selbst erledigen.
[...]

Das sehe ich doch anders. An fuer sich hast du Recht. Solange du dich darauf verlassen kannst das da alle beteiligten Entwickler mitspielen. Was grade bei groeseren Firmen, Outsourcing oder Firmenuebergreifenden Projekten nicht immer gegeben ist.

Wenn man Checkstyle seinen Coding Conventions entsprechend angepasst hat und in einem Pre-Commit Trigger verwendet spart es echt eine Menge Aerger. Dann kommt halt nur das ins Repsoitory was den Coding Conventions entspricht. Das hat wenig mit Findbugs zu tun. Hier geht es einzig und allein um den wie du schon schoen gehighlighted hast - den Stil.

Was machst du wenn 3 Firmen mit 3 IDEs und je 20 Entwicklern an einem Projekt arbeiten. Die dann ale ihre eigenen Sets an Formatierungsrichtlinien haben? Das kannst du nur zentral glatt ziehen und b ei sowas ist Checkstyle sehr hilfreich.

 

[tfa]

Wenn man Checkstyle seinen Coding Conventions entsprechend angepasst hat und in einem Pre-Commit Trigger verwendet spart es echt eine Menge Aerger. Dann kommt halt nur das ins Repsoitory was den Coding Conventions entspricht.

Aber wie weit soll das führen? Wird hier nur auf einfache Formatierungsregeln geprüft (also sind die Leerzeichen/Klammern an der richtigen Stelle)? Das kann man eh nur durch Autoformatter lösen, warum sollte man das nochmal prüfen? Wenn sich jemand nicht dran hält, fällt das früh genug auf (spätestens beim Einchecken ins Repository), und man kann das Problem mit dem betreffenden individuell lösen.

Oder hast du auch tiefer gehende Checks? Was soll dann also passieren, wenn eine Methode, die ansich ideal geschrieben ist, genau eine Zeile zu lang ist? Dann darf ich die nicht einchecken! Soll ich sie nur deswegen in zwei Methoden aufspalten und den Code dadurch weiter aufzublähen und ggf. unleserlicher und schwerer wartbar zu machen?
Vielleicht braucht eine Methode mal 6 Parameter, statt der maximal erlaubten 5. Wenn das nicht überhand nimmt ist auch nichts dagegen einzuwenden.

Oder das Beispiel des TS. Ich darf in einer erweiterbaren Basisklasse kein toString() implementieren, weil sonst der "Design for Extension"-Checker meckert. Das sollte doch lieber der Entwickler entscheiden und nicht ein dummes Quelltextanalysetool.

Design- und Programmierregeln sind gut. Aber man darf sie nicht zum Gesetz erheben. Und genau das tust du, wenn du Checkstyle zum Torwächter über dein Repository machst. Es gibt immer wieder Ausnahmesituationen, wo man die Regeln übertreten können muss.

Was machst du wenn 3 Firmen mit 3 IDEs und je 20 Entwicklern an einem Projekt arbeiten. Die dann ale ihre eigenen Sets an Formatierungsrichtlinien haben?

Dann hat man sicherlich ganz andere Probleme als sie sich mit Checkstyle beheben ließen.