Aus dem Leben eines Entwicklers


01.06.2016 von

https://www.iteratec.de/fileadmin/Bilder/News/iteratec_logo.png https://www.iteratec.de/fileadmin/Bilder/News/iteratec_logo.png iteratec GmbH

Jeder träumt davon, auf der grünen Wiese anzufangen. So ist das Leben aber leider nicht. Besonders wenn man wie ich gerne in Java unterwegs ist. Die Sprache existiert schon ein wenig länger und es wurde bereits viel Code damit geschrieben. Somit bleibt es nicht aus, auch mal den Code anderer Entwickler zu erweitern.

Letztens 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. Auch teure Kräfte, auch gute Entwickler, meistens jedoch eher die billigeren Entwickler. Und deshalb komme ich nicht umhin, heute eine der Stilblüten zu veröffentlichen, über die ich im gesamten Code vielfach gestolpert bin.

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.

Was könnten wir dann tun?

Programmiere gegen Interfaces, nicht gegen Implementierungen!

Schauen wir uns die Signatur noch einmal an:

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:

public List<KryptischesDO> holeLesebestaetigungenZuNr(...)

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

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 ein eigenes Transferobjekt verwenden:

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?

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

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

public void trace(final String message) {
  if (isTraceEnabled()) {
    output.append(message);
  }
}

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

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:

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:

<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. Dieser könnte vereinfacht wie folgt aussehen:

@Aspect
public class LogServiceCalls
{
  @Around( "execution(* com.xyz.service.*.*(..))" )  
  public Object log(ProceedingJoinPoint join) throws Throwable {
    final Signature signature = join.getSignature();
    final String description = signature.getDeclaringTypeName() + "." + signature.getName(); 

    final Logger log = ...;

    log.trace(String.format("Begin: %1s, description));
    Object retVal = join.proceed();
    log.trace(String.format("End: %1s, description));

    return retVal;
  }

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

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:

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.

Überlassen wir das Loggen also dem Aspekt und unternehmen noch den letzten Schritt und vereinfachen den Code weiter. Somit bleibt nur noch der Aufruf der eigentlichen Geschäftslogik über:

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

Den obigen Aspekt müssten wir nur um ein paar Zeilen erweitern:

@Aspect
public class LogServiceCalls
{
  ...

  @AfterThrowing( "execution(* com.xyz.service.*.*(..))" )
  public void failure(...) {
    // ...
  }
}

Und sonst so?

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?

Diesen Artikel bewerten
 
 
 
 
 
 
 
0 Bewertungen (0 %)
Bewerten
 
 
 
 
 
 
1
5
0
 

Artikel im Warenkorb:

0