Göm menyn

Förklaringar av varningar

IDEA kan ge en hel del varningar genom automatisk inspektion av programkod. Många varningar har bra förklaringar i IDEA: Sätt textmarkören på den färgmarkerade kodsnutten och tryck Ctrl-F1. Här följer några utökade förklaringar av sådana varningar där IDEAs beskrivning kanske inte är så intuitiv. Meddela examinatorn om ni får andra varningar som ni inte förstår, så kan vi utöka sidan!

Den inspektionsprofil (konfiguration) vi använder i kursen har flera olika nivåer på varningar:

  • ERROR (röd): Felaktig kod, kan normalt inte kompileras, måste fixas

  • SEVERE (orange): Allvarligt problem, måste fixas

  • WARNING (gul): Generell varning

  • COMPLEX (lila): Kan vara komplicerat / svårläst / svårförståeligt

  • OPTIMIZE (blå): Ineffektivt, kan optimeras

  • STYLE (grön): Mindre allvarliga problem med kodstil, tydlighet och läsbarhet

  • DOC (grön): Dokumentationsproblem

Som diskuterats i kvalitetskriterierna ska alla varningar antingen fixas eller (i de fall varningarna är "false positives") kommenteras tydligt.

(Listan kan i och för sig innehålla en del riktlinjer för hur man ska programmera, men ni behöver inte nödvändigtvis läsa den förrän ni faktiskt undrar över någon varning som IDEA ger.)

En JAR-fil med IDEA-inställningar finns tillgänglig för den som arbetar hemma. Ladda ner den och kör File | Import Settings. Ni kan sedan behöva göra vissa manuella val av profiler i File | Settings, t.ex. välja inspektionsprofilen TDDD78-2015-v1 i File | Settings | Inspections. Det kan hända att "färgkodningen" för inspektionsvarningar inte blir identisk med den på universitetets datorer.

Severe

  • Chain of 'instanceof' checks: Koden testar explicit vilken typ ett objekt har, t.ex. om det är en Circle, Rectangle eller Square, och gör sedan olika saker beroende på typen. Detta är oftast ett misstag eftersom man istället vill hantera detta via t.ex. subtypspolymorfism eller i vissa fall ad-hoc-polymorfism.

  • 'instanceof' check for 'this': Koden testar explicit vilken typ detta objekt har. Detta ska nästan alltid hanteras via polymorfism och overriding istället.

  • Class without package statement / Class '...' lacks a package statement: Som vi har sagt tidigare ska all kod ligga i paket.

  • No-op method in abstract class / No-op method '...' should be made abstract: Om man ger en tom metodimplementation i en abstrakt klass är det lätt att missa att implementera metoden "på riktigt" i en subklass. Normalt vill man ha en icke-implementerad metod i den abstrakta klassen istället, dvs. "method();" istället för "method() {}". Då klagar kompilatorn om man glömmer att implementera den i en konkret subklass.

  • Empty 'catch' block: Fel måste hanteras, inte ignoreras.

  • Unused 'catch' parameter: Koden fångar ett undantagsobjekt men använder det inte. Det betyder att felmeddelandet som undantaget innehåller ignoreras, vilket sällan är bra. Exempel:

      try  {
        ...
      catch (IOException e) {
          System.out.println("Could not listen on port: "+port+".");
          System.exit(-1);
      }      
    Här ska man normalt använda det undantag "e" som man fångar. Skriv ut felet med printStackTrace(), skicka vidare ursprungsfelet med throw efter att du har städat upp, och/eller kasta en "chained exception".

  • Unused 'catch' parameter: Det exceptionobjekt som fångas används inte.

  • Prohibited exception thrown: Koden kastar ett undantag av generell typ, t.ex. Exception, istället för att använda en passande specifik typ.

  • Overly broad 'catch' block / Catch of '...' is too broad, masking exception(s) '...': Koden fångar upp breda grupper av undantag istället för de specifika typer av undantag som faktiskt kastas i try-blocket. Om man t.ex. fångar Exception kommer alla typer av kontrollerade undantag att fångas upp, även sådana som inte fanns när koden först skrevs men introducerades först när man ändrade i koden. Detta kan lätt leda till att man missar att introducera nya catch-satser för nya typer av fel.

  • Static field '...' may not be initialized during class initialization: Ge det statiska fältet ett initialvärde, istället för att förlita dig på defaultvärdet zero/null/false.

  • Assignment to static field '...' via an instance: Du använder ett statiskt fält via en objektinstans, antingen implicit eller genom att använda fältet okvalificerat inuti en metod. Detta är missvisande, eftersom det ser ut som om du använder ett vanligt instansfält som har ett värde för varje instans, medan det i själva verket bara lagras ett värde för hela klassen. Kvalificera med klassnamnet: classname.field.

  • Access static member via instance reference / Static member '...' accessed via instance reference: Liknar ovanstående problem.

  • Unsafe lazy initialization of 'static' field / Lazy initialization of 'static' field ... is not thread-safe: En statisk variabel tilldelas sitt initialvärde av en metod som skulle kunna anropas från flera trådar på samma gång, och det finns ingen trådsynkronisering som garanterar att variabeln bara initialiseras en enda gång. Ofta kan man istället undvika "lazy initialization" och helt enkelt initialisera variabeln där den deklareras. Då kan detta problem inte uppstå. Vill man absolut använda lat initialisering (där variabelns värde inte beräknas förrän det verkligen ska användas) kan man t.ex. se Initialization-on-demand holder idiom, som är trådsäkert.

  • Raw use of parameterized class: Koden använder en generisk typ där ingen typparameter är angiven. Till exempel kanske den deklarerar en variabel med den råa typen List istället för List<String>. Detta leder till sämre typsäkerhet.

  • Unchecked warning / Unchecked call to '...' as a member of raw type '...': Koden använder en medlem i en generisk typ där ingen typparameter är angiven. Till exempel kanske den använder add()-metoden i en lista som deklarerats som den råa typen List istället för List<String>. Detta leder till sämre typsäkerhet eftersom add() inte vet vilken elementtyp den returnerar. Skillnaden från föregående problem är att detta reporterar när man använder medlemmar i en variabel av rå typ, snarare än när man använder den råa typen själv.

  • Floating point equality comparison: Koden testar om två floating point-värden är exakt lika. Detta är problematiskt eftersom beräkningar med ändlig precision ofta får olika resultat beroende på i vilken ordning de utförs, och avrundningsfel är vanliga. Exempel:

        float val = 0.1f + 0.1f;
        System.out.println(val == 0.2);

    Detta program ger svaret false!

  • Integer multiplication or shift implicitly cast to long: Ett värde beräknas som 'int' (med 32-bitars heltal) och konverteras sedan till en 'long' (med 64-bitars heltal). Ofta menade man att utföra även själva beräkningen med 64 bitar. Som koden är skriven nu kan den ge "overflow" utan att detta märks.

  • Comparable implemented but 'equals()' not overridden / Class '...' implements 'java.lang.Comparable' but does not override 'equals()': Gränssnittet Comparable innehåller en compareTo()-metod som jämför om ett objekt är före, efter, eller på samma plats som ett annat i en viss sorteringsordning. Om man implementerar detta måste man också implementera equals(), så att det inte kan finnas två objekt som är lika enligt equals() men ska vara på olika position enligt compareTo(). Annars kan de datatyper som faktiskt använder compareTo() bli mycket förvirrade.

  • Condition '...' is always false / Condition '...' is always true: IDEA genomför en dataflödesanalys och identifierar villkor vars värden är kända redan vid kompileringstillfället. Detta uppstår ofta på grund av tankefel i programmeringen och är därför viktigt att åtgärda.

  • Method invocation '...' may produce 'java.lang.NullPointerException': According to IDEA's analysis, the object whose method you want to invoke may be null.

  • Condition 'x instanceof Y' is redundant and can be replaced with '!= null': IDEA's analysis shows that x is either null or an object of type Y. Therefore, 'x instanceof Y' is true if and only if x!=null.

  • Object comparison using '==' instead of 'equals()': Objektjämförelse med '==' testar om två pekare pekar på exakt samma objekt, på samma maskinadress. Oftast vill man istället använda 'equals()', som ska jämföra om två objekt är lika (om två strängar innehåller samma tecken, till exempel).

    Vissa undantag finns. I implementationen av equals() eller compareTo() kan man av effektivitetsskäl börja med att testa om man jämför ett objekt med sig själv (samma pekare). Det finns också andra fall där det faktiskt är identitet, och inte likhet, som man vill testa. De flesta fall av "==" mellan objekt som vi ser i inlämnade projekt är dock misstag.

  • Result of method call ignored: IDEA har en lista på metoder vars returvärden är viktiga och inte får ignoreras. Det inkluderar t.ex. File.mkdir(), som indikerar om metoden lyckades via returvärde istället för via undantagshantering, och Object.hashCode(), som ju inte har sidoeffekter utan anropas enbart för dess returvärde.

  • Result of object allocation ignored / Result of '...' is ignored: Indikerar att man har skapat ett objekt men inte sparat en pekare till objektet någonstans. I de allra flesta fall är detta meningslöst och leder bara till att objektets kommer att samlas in som skräp. Vissa undantag finns i Javas klassbibliotek, främst i fråga om Frame / JFrame där konstruktorn skapar ett fönster som registrerar sig i fönstersystemet och sedan "lever vidare" på egen hand. Inspektionen får alltså ignoreras för Frame och dess subklasser. Denna metod där objekt registrerar sig själva någonstans är normalt inget man ska efterlikna, annat än i mycket specifika undantagsfall.

  • Suspicious indentation after control statement without braces: Rapporterar när indenteringsnivån är missvisande. Kan fixas genom att filen indenteras om (Code|Auto-Indent Lines) eller formatteras om (Code|Reformat Code).

  • Duplicate condition in 'if' statement: En if-sats har flera grenar som testar samma villkor, eller testar samma villkor flera gånger i en gren. Detta är sällan vad programmeraren menade, och kan indikera att man antingen ska ta bort en gren eller ville ha ett annat villkor på den grenen.

  • Duplicate condition on '&&' or '||': Ett villkorsuttryck testar samma villkor flera gånger. Detta är sällan vad programmeraren menade, och kan indikera att man antingen ska ta bort en del av grenen eller ville ha ett annat villkor den andra gången.

Andra nivåer

  • Magic Number: Du använder magiska tal vars värden kan vara svåra att tolka. Ser man "width/30" vet man kanske inte vad 30 betyder. Ser man "width/BLOCK_SIZE", där man har "public final static int BLOCK_SIZE = 30", är det lättare att förstå. Detta är ett bra sätt att göra koden mer självdokumenterande.

    Det finns vissa undantag där magiska tal kan användas utan att förvirra. Ett sådant exempel är för färger, där "new Color(0,255,0)" knappast behöver namngivna konstanter. Detta behöver ni inte dokumentera vid varje färg.

  • Utility class is not final: En hjälpklass är en klass som bara har statiska fält och metoder. Det finns inget att tjäna på att göra en subklass till en sådan, så den kan lika gärna vara markerad "final" för att undvika att man skapar en subklass av misstag.

  • Utility class without private constructor: En hjälpklass är en klass som bara har statiska fält och metoder. Det finns inget att tjäna på att göra en instans av en sådan. Man kan inte låta klassen sakna konstruktorer helt – om man inte anger någon konstruktor skapas en defaultkonstruktor som är public. Men man kan ge den en privat konstruktor, så kan i alla fall ingen kod i en annan klass skapa en instans av hjälpklassen. Sedan ser man helt enkelt till att inte anropa konstruktorn inifrån den egna klassen heller.

  • Declaration access can be weaker: Fält, metod eller klass som inte behöver vara tillgängligt så brett som det är nu utan kan göras "mer privat".

  • Public constructor in abstract class: Man kan inte kan skapa en instans av en abstrakt klass, utan bara instanser av dess subklasser. Därför är det onödigt att den abstrakta klassens konstruktor är public. Det räcker gott med att den är protected.

  • Refused bequest: "Bequest" är en testamenterad gåva, dvs. ett arv. Varningen betyder att en metod foo() "override:ar" en annan, utan att anropa den ursprungliga implementationen, super.foo(). Detta kan vara korrekt (om man faktiskt vill ignorera superklassens implementation) men kan också leda till buggar om man bara ville "utöka" superklassens funktionalitet. Om en GUI-komponents paintComponent()-metod glömmer att anropa super.paintComponent() kommer den t.ex. inte automatiskt att rita ut sin bakgrundsfärg.

  • Public field: Publika fält ska normalt inte användas, eftersom det gör att man exponerar interna implementationsdetaljer för andra. När andra klasser direkt använder de publika fälten blir det svårt att ändra den interna datarepresentationen.

    Vissa undantag kan göras, främst för enklare datalagringsklasser som t.ex. Rectangle i Java.

  • Package-visible ...: Ett fält, en metod eller en klass är åtkomlig från alla klasser inom samma paket. Detta kan vara motiverat i vissa begränsade fall men är oftare ett tecken på problematisk modellering där man har tagit hänsyn till implementationsdetaljer istället för hur klasserna egentligen borde modelleras. Det kan också indikera att man helt enkelt har glömt bort att ange en synlighetsnivå.

  • Call to simple getter '...' from within class: En enkel getter är en metod som helt enkelt returnerar värdet på ett fält. Det är bra att använda sådana metoder för att undvika behovet av publika fält, eftersom publika fält gör att andra klasser får reda på den interna representationen. Inom klassen där fältet deklareras finns mindre behov av detta, och där kan det vara bättre att använda fältet direkt för att få kortare och mer lättläst kod.

  • Call to overridden method '...' during object construction: En konstruktor för klass A anropar en metod som finns implementerad i klass A , men är omimplementerad (overridden) i en underklass B. Detta gör att klass A inte har full kontroll över vad som sker när den konstrueras (det är ju delvis kod i klass B som utförs), vilket kan leda till problem.

  • Boolean method name '...' does not start with question word: Det är bra om metoder som ger ett ja/nej-svar börjar med ett frågeord. Till exempel använder man window.isVisible() istället för window.getVisible(), och colorModel.hasAlpha() istället för colorModel.getAlpha().

  • Collection declared by class, not interface: Man bör normalt deklarera collection-variabler eller fält med en gränssnittstyp, som List, istället för en konkret klass, som ArrayList. Detta hjälper till att undvika onödiga beroenden på exakt vilken konkret implementation av gränssnittet som används och gör det därmed lättare att byta implementation senare (till exempel att byta till LinkedList istället för ArrayList utan att byta ut deklarationstypen List). Med andra ord, "List list = new ArrayList()" istället för "ArrayList list = new ArrayList()".

  • Abstract class without abstract methods: En klass behöver bara vara abstrakt om man vill ange att alla subklasser ska ha vissa metoder, men inte inte kan ge en "standardimplementation" för alla dessa metoder. Abstrakta klasser ska alltså normalt ha abstrakta metoder. Om alla metoder har en implementation gäller något av följande:

    • Klassen är fullt funktionsduglig och behöver inte vara abstrakt.
    • En eller flera av metoderna har en orimlig implementation som gör att klassen inte är fullt funktionsduglig. Den implementationen borde i de flesta fall tas bort och ersättas med en abstrakt metod.

  • Class explicitly extends a Collection class: När man skapar en subklass till en Collection-klass öppnar man samtidigt upp för godtycklig manipulering av klassens objekt genom alla metoder som Collection-klassen ger tillgång till. Detta är ett typiskt fall när delegering brukar vara ett bättre val än ärvning – då bestämmer man själv vilken av Collection-klassens funktionalitet som ska exponeras utåt.


Sidansvarig: Jonas Kvarnström
Senast uppdaterad: 2015-04-17