Bij sommige projecten wordt er serieus aandacht besteed aan kwaliteitscontroles. Elke class die ingecheckt wordt moet voorzien zijn van testcases die 85% van de code raken en 100% van de branches binnen de code. Ook mag CheckStyle, een tool die code beoordeeld of deze aan de standaarden voldoet, geen commentaar meer hebben en wordt de code ook nog door een reviewer bekeken, zodat de unittests niet alleen geschreven worden om de coverage te halen, maar ook daadwerkelijk controles uitvoeren. Toch kunnen er nog verdachte stukken code doorheen slippen. In een verloren uurtje heb ik FindBugs maar eens los gelaten op de code. De leukste bug is bewaard voor het einde van dit artikel.

In een eerdere blog heb ik al aandacht besteed aan de open source tools FindBugs. Deze tool zoekt in bytecode naar bekende anti-patterns, slechte constructies en mogelijke bugs. In deze blog wil ik laten zien wat er nog gevonden kan worden als je FindBugs op een real-life project los laat. De tijd die het kost om FindBugs te downloaden, op te starten, aan te geven waar de class files staan, de controle te laten doen en de uitkomsten te bekijken is daadwerkelijk minder dan een uur zodat er eigenlijk geen reden is om dit soort controles niet uit te voeren.

In het vet staan de benamingen zoals deze door FindBugs gebruikt worden. Daaronder staat een vrije vertaling van de aanvullende tekst van FindBugs. De code zijn geanonimiseerde statements die in een daadwerkelijk project voor kwamen. Dit is slechts een greep uit de meldingen die boven water kwamen. Een aantal zijn typische beginners fouten en van sommigen zal ook de meer ervaren java programmeur nog opkijken.

Dead store to local variable

public void doSomething() {
  int localeVar = 3;
 
  // een hoop code, maar localeVar wordt nooit meer gelezen.
 
}

Een instructie geeft een lokale variabele een waarde maar die waarde wordt in het vervolg van de code nooit meer uitgelezen. Sun’s javac compiler genereert vaak dead stores voor final lokale variabelen. Omdat FindBugs naar de bytecode kijkt kunnen er in dit geval wel eens onterechte meldingen gegeven worden. Gelukkig bestaat in FindBugs de mogelijkheid om per melding aan tegeven dat dit geen bug is.
Dit soort meldingen zal in de meeste gevallen duiden op ‘oude’ code die verwijderd moet worden, maar het kan ook een programmeerfout zijn en dat de variabele ten onrechte niet wordt gebruikt. De meeste IDE’s kunnen dit soort fouten ook opsporen maar het voordeel van FindBugs hierbij is dat het ook via een ant script kan worden aangeroepen en eventueel in een continuous integration kan worden opgenomen.

Method invokes inefficient Number constructor; use static valueOf instead

private static final Integer CODE = new Integer(1);

Het gebruik van new Integer(int) garandeert dat er altijd een nieuw object wordt gecreëerd. In Java 1.5 is hier een optimalisatie geintroduceerd. Door de code te vervangen door Integer.valueOf(int) kan de compiler of de JVM de gebruikte waarden cachen. Het gebruik van gecachte waarden is sneller dan het creëren van een nieuwe instantie.
Voor waardes tussen -128 en 127 wordt er gegarandeerd dat er een corresponderende gecachte instantie bestaat en het gebruik hiervan is ongeveer 3.5 keer zo snel als het creëren van een nieuwe instantie.

Method invokes inefficient new String(String) constructor

stringList.add(new String("de waarde"));

Het gebruik van de java.lang.String(String)constructor verspilt geheugen omdat de string die op deze manier wordt geconstrueerd functioneel niet verschilt van de string die als parameter wordt meegegeven. De code moet vervangen worden door de volgende constructie:

stringList.add("de waarde");

Method invokes inefficient new String() constructor

String waarde = new String();

Het creëren van een nieuwe java.lang.String met de no-argument constructor verspilt geheugen want de string die op deze manier wordt gemaakt verschilt functioneel niet van de string constante "". Java garandeert dat identieke string constanten worden gerepresenteerd door het zelfde String object. De code kan vervangen worden door:

String waarde = "";

Method invokes toString() method on a String

String waarde1 = "waarde";
// hier tussen kan nog van alles gebeuren.
String waarde2 = waarde1.toString();

De aanroep van String.toString() is volkomen overbodig en moet vervangen worden door:

String waarde1 = "waarde";
// hier tussen kan nog van alles gebeuren.
String waarde2 = waarde1;

Class defines compareTo(…) and uses Object.equals()
Dit bug-pattern treedt op als er een class wordt gemaakt waarin een compareTo(...) methode is gedefineerd en de equals() methode wordt overerfd van java.lang.Object. Volgens JavaDoc van de compareTo methode is het sterk aan te bevelen (maar niet verplicht) dat de uitkomst van compareTo enkel en alleen nul is als equals true teruggeeft. Als deze regel wordt overtreden kan er onvoorspelbaar gedrag ontstaan in classen zoals PriorityQueue. In Java 5 gebruikt de PriorityQueue.remove methode de compareTo methode, terwijl Java 6 de equals methode gebruikt.
Eigenlijk is het een goede gewoonte om altijd de equals() te implementeren voor elke class.

Call to static DateFormat

private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("yyyyMMdd");
.
.
public void doSomething() {
   StringBuider buffer = new StringBuilder();
   buffer.append(DATE_FORMAT.format(new Date()));
}

Zoals in de JavaDoc staat vermeld, zijn DateFormats niet veilig voor het gebruik in een multithreaded situatie. Het gebruik van een DateFormat die is verkregen via een static veld is bij voorbaat verdacht.
Voor meer informatie zie Sun Bug #6231579 en Sun Bug #6178997.
In dit geval zit er niets anders op dan de DateFormatter local te definiëren of de toegang synchronized te maken.

Usage of GetResource may be unsafe if class is extended

InputStream input = getClass().getResourceAsStream(resourceName);

Het gebruik van this.getClass().getResource(...) kan een ander resultaat geven dan verwacht als deze class als base class wordt gebruikt door een class in een andere package.
Dit hoeft helemaal geen bug te zijn, maar dat is voor FindBugs moeilijk vast te stellen. Gelukkig is het in FindBugs mogelijk om per gevonden item aan te geven dat dit geen bug is en bij de volgende analyse zullen die meldingen dan ook niet meer verschijnen.

Null pointer dereference

if (stringReferentie != null || !stringReferentie.equals("")) {
…
}

if (entiteit == null) {
  // entiteit niet gevonden
  throw new ObjectNotFound("Entiteit niet gevonden:" + entiteit.toString());
}

Nadat geconstateerd is dat een referentie een null waarde heeft, wordt er toch gebruik gemaakt van die referentie. Dit soort constructies gaan zeker tot null pointers leiden. De eerste is een duidelijk geval waar && had moeten staan maar er een || voor in de plaats staat. De tweede is toch wel een heel briljante denkfout waarbij de programmeur zo veel mogelijk informatie bij een fout wilde meegeven.

Conclusie

Al met al komt er toch nog heel wat naar boven met FindBugs. Zeker de null pointer dereferences zijn constructies die niet thuis horen in productie software. Het zoeken naar bugs met FindBugs is heel eenvoudig, het kost bijna geen tijd en je leert er af en toe iets nieuws van. FindBugs is geen vervanging van de overige QA maatregelen, zoals tests en code review, maar wel een hele nuttige aanvulling.

zie ook: http://findbugs.sourceforge.net/

—————————————————————————————
Meer weten over Java-specialist Finalist IT Group?