Aus dem Leben eines Entwicklers

Neulich durfte ich mal wieder bestehenden Code auf einer neuen Plattform zum Laufen bekommen. Der Code war natürlich gewachsen, hieß es, es haben sich schon viele daran versucht, teure Kräfte, aber meistens doch eher die billigeren Entwickler. Und deshalb komme ich nicht umhin, heute eine der Stilblüten zu veröffentlichen, über die ich im gesamten Code gestolpert bin.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
public ArrayList<KryptischesDO> holeLesebestaetigungenZuNr(String vsnr, boolean inklusiveUngelesene)
  throws RemoteException, KryptischesBackendException
{
  if (LOG.isTraceEnabled()) {
    LOG.trace("Begin: KryptischesWebBackendImpl.holeLesebestaetigungenZuNr");
  }
  if (true) {
    throw this.kryptischesBackendExceptionFactory
        .createNotImplementedException("holeLesebestaetigungenZuNr not implemented");
  }
  if (LOG.isTraceEnabled()) {
    LOG.trace("End: KryptischesWebBackendImpl.holeLesebestaetigungenZuNr");
  }
  return null;
}

Vielleicht sollte ich noch hinzufügen, dass es sich um eine Anwendung handelt, in der Spring eingesetzt wird. Und das der Code wirklich über Jahre eingesetzt wurde. Das Wort „kryptisch“ habe ich im Code ersetzt für eine wirklich schlecht lesbare kryptische Abkürzung.

Was kann an diesem Code verbessert werden?

Die einfachste Lösung wäre es, ihn zu löschen. Er existiert seit Jahren und erzeugt nur einen Fehler, also wird er mit Sicherheit auch nicht aufgerufen.

Nehmen wir mal an, diese Lösung existiert nicht, weil die Schnittstelle rückwärtskompatibel bleiben muss. Nehmen wir weiterhin an, der Code enthielte tatsächlich Geschäftslogik, die aufgerufen werden müsste.

Programmiere gegen Interfaces, nicht gegen Implementierungen!

Schauen wir uns die Signatur noch einmal an:

1
public ArrayList<KryptischesDO> holeLesebestaetigungenZuNr(...)

Warum muss es unbedingt eine ArrayList sein? Warum muss der Aufrufer wissen, welche Implementierung wir intern verwenden? Und was heißt das für den Code, wenn wir die interne Implementierung austauschen, z.B. gegen eine LinkedList?

Wenn die Reihenfolge wichtig ist, dann sollte die Signatur so ausschauen:

1
public List<KryptischesDO> holeLesebestaetigungenZuNr(...)

Wenn die Reihenfolge nicht wichtig ist, sollten wir Signatur so verändern, damit wir intern z.B. zwischen Set und List wechseln können:

1
public Collection<KryptischesDO> holeLesebestaetigungenZuNr(...)

Wenn ich mir nicht sicher bin, ob die Reihenfolge wichtig ist oder nicht, oder wenn ich auf der Liste etwas tun will, dann sollte ich dafür gleich eine eigenes Transferobjekt verwenden:

1
public KryptischeObjekte holeLesebestaetigungenZuNr(...)

Warum verwende ich LOG.isDebugEnabled() ?

Der Code ist ein wunderbares Beispiel für Copy & Paste ohne Nachdenken. Was meint der Entwickler, wie der Entwickler des Logging-Frameworks wohl den Aufruf trace umgesetzt hat?

1
2
3
if (LOG.isTraceEnabled()) {
  LOG.trace("Begin: KryptischesWebBackendImpl.holeLesebestaetigungenZuNr");
}

Egal, wie das Framework aussieht, der Code wird gewisse Ähnlichkeit mit dem folgenden stark vereinfachtem Fragment haben:

1
2
3
4
5
public void trace(final String message) {
  if (isTraceEnabled()) {
    output.append(message);
  }
}

Wann verwende ich tatsächlich isTraceEnabled() bzw. isDebugEnabled() in meinem Code? Na, wenn ich die Erzeugung der Fehlermeldung selber vermeiden möchte:

1
2
3
if (LOG.isTraceEnabled()) {
  LOG.trace("Begin: " + StackUtils.getCallingClass() + "." + StackUtils.getCallingMethod());
}

Wenn ich hier kein isTraceEnabled() abfrage, dann wird der String zusammengesetzt, bevor im Code von trace() abgefragt wird, ob überhaupt eine Ausgabe erfolgen soll. Und damit würden auch die potentiell teuren Abfragen auf dem StackTrace erfolgen.

Wenn ich allerdings nur das Zusammensetzen der Nachricht aus verschiedenen Parametern vermeiden möchte, dann hilft der kleine Umweg über String.format() schnell weiter:

1
2
3
4
5
6
7
8
9
  private static final String CLASS_NAME = "KryptischesWebBackendImpl";
 
  ...
 
  {
    String methodName = "holeLesebestaetigungenZuNr";
 
    LOG.trace(String.format("Begin: %1s.%2s", CLASS_NAME, methodName));
  }

Lagere cross-cutting concerns einfach aus!

An dem Code ist natürlich besonders interessant, dass er sehr resistent gegen Refactoring ist. Klassennamen und Methodennamen werden als Zeichenketten in jeder Methode abgelegt. Was passiert wohl mit den Ausgaben, wenn sich der Name einer Methode ändert?

Im vorliegenden Beispiel gibt es rund 60 solcher Klassen mit durchschnittlich 10 Methoden.

Wie lange haben die Entwickler wohl daran gesessen, nur den Code zum Loggen hinzuzufügen? Jetzt hoffen wir einfach mal, dass die Klasse nicht nur erzeugt worden ist, um die Log-Ausgabe zu ermöglichen. Wobei der reale Code mich da eher zweifeln lässt. Wenn ich so oft fast den gleichen Code schreiben muss, ist es dann nicht an der Zeit, darüber nachzudenken, wie das auch einfacher geht?

In diesem Fall hätte das oberflächliche Lesen der Dokumentation zu Spring schon weiter geholfen. Selbst in älteren Versionen hätte folgende Konfiguration geholfen:

50
51
52
53
54
55
56
57
58
<bean id="customizableTraceInterceptor" class="org.springframework.aop.interceptor.CustomizableTraceInterceptor">
  <property name="enterMessage" value="Begin: $[targetClassShortName].$[methodName]"/>
  <property name="exitMessage" value="End: $[targetClassShortName].$[methodName]"/> 
</bean>
 
<aop:config> 
  <aop:advisor advice-ref="customizableTraceInterceptor" 
                  pointcut="execution(public * com.kryptisch.service.*(..))"/> 
</aop:config>

Statt CustomizableTraceInterceptor lässt sich natürlich auch der SimpleTraceInterceptor verwenden. Ab Spring 2.5 lässt sich auch prima mit @Aspect eine eigene Implementierung dagegen setzen.

Was bleibt noch vom Code, wenn wir diese Vorschläge umsetzen?

1
2
3
4
5
6
7
8
9
public List<KryptischesDO> holeLesebestaetigungenZuNr(String vsnr, boolean inklusiveUngelesene)
  throws RemoteException, KryptischesBackendException
{
  if (true) {
    throw this.kryptischesBackendExceptionFactory
        .createNotImplementedException("holeLesebestaetigungenZuNr not implemented");
  }
  return null;
}

Was wahr ist, wird auch wahr bleiben.

Warum hat der Entwickler ursprünglich if (true) { ... } eingebaut? Was wahr ist, wird immer wahr bleiben. Ganz einfach: Ansonsten hätte sich der Compiler beschwert, dass der Code nach dem Schmeißen der Ausnahme nie erreicht wird. Der Entwickler hätte also den Code umschreiben müssen und hätte dabei den Platzhalter-Code löschen müssen:

1
2
3
4
5
6
7
8
public List<KryptischesDO> holeLesebestaetigungenZuNr(String vsnr, boolean inklusiveUngelesene)
  throws RemoteException, KryptischesBackendException
{
  LOG.trace("Begin: KryptischesWebBackendImpl.holeLesebestaetigungenZuNr");
 
  throw this.kryptischesBackendExceptionFactory
        .createNotImplementedException("holeLesebestaetigungenZuNr not implemented");
}

Immerhin besser lesbarer als die ursprüngliche Version, und er hätte gleich den Nachteil seines Logging-Ansatzes gesehen: Was wird eigentlich ausgegeben, wenn die eigentliche Geschäftsmethode einen Fehler wirft? Genau, nichts.

Unternehmen wir also noch den letzten Schritt und vereinfachen den Code weiter, und voila, es bleibt nur noch der Aufruf der eigentlichen Geschäftslogik über:

1
2
3
4
5
6
public List<KryptischesDO> holeLesebestaetigungenZuNr(String vsnr, boolean inklusiveUngelesene)
  throws RemoteException, KryptischesBackendException
{
  throw this.kryptischesBackendExceptionFactory
        .createNotImplementedException("holeLesebestaetigungenZuNr not implemented");
}

Bleibt nur die akademische Frage übrig: Wie würde ich diese Funktion umsetzen, wenn ich die Schnittstelle ändern dürfte? Alle anderen Maßnahmen haben die Schnittstelle belassen wie sie ist.

Davon abgesehen würde ich zum Beispiel noch den abgekürzten Parameternamen bemängeln – was bitte schön ist eine vsnr? Ist es eine Vertragsnummer, eine Versicherungsnummer, oder ist es vielleicht ein Nenner und keine Nummer? Moderne IDEs bieten eine gute Vervollständigung des Codes an, sie darf genutzt werden, und schon wird der Code wieder lesbarer.

Genug schwadroniert. Wer findet, ich übertreibe masslos? Wer sagt, das geht aber besser? Und wer darf auch solchen Code warten? Fehlt noch etwas?