Göm menyn

Vanliga varningar från IDEA

En hel del potentiella problem i programkod kan upptäckas automatiskt, genom algoritmer som analyserar koden och upptäcker specifika mönster. IDEA har hundratals sådana "inspektionsvarningar", och vi har satt upp en profil (TDDD78-årtal-version) som är anpassad till kursen.

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 beskrivningar kanske inte är så pedagogiska. 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

  • SEVERE (orange): Allvarligt problem

  • WARNING (gul): Generell varning

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

  • OPTIMIZE (blå): Ineffektivt, kan optimeras

  • STYLE (grön): Kodstil, tydlighet och läsbarhet

  • DOC (grön): Dokumentationsproblem

Som diskuterats i projektbedömningen ska alla varningar antingen fixas eller (i de fall varningarna är "false positives") kommenteras tydligt, vid varje plats där varningen uppkommer (givetvis kan man hänvisa till längre förklaringar på annan plats).

En JAR-fil med IDEA-inställningar finns tillgänglig för den som arbetar hemma. Ladda ner den och följ instruktionerna på den länkade sidan.

Varningar i alfabetisk ordning

  • 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 i sig själv 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.

  • Access static member via instance reference / Static member '...' accessed via instance reference – Du använder en statisk metod via en objektinstans, antingen explicit (object.staticmethod()) eller implicit (staticmethod() anropad från en icke-statisk metod i samma klass). Detta är missvisande, eftersom det ser ut som om man använder en vanlig instansmetod. Kvalificera med klassnamnet: classname.staticmethod().

  • Assignment to static field '...' via an instance – Du använder ett statiskt fält via en objektinstans, antingen explicit (object.staticfield) eller implicit (staticfield används från en icke-statisk metod i samma klass). 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.

  • Auto-boxing – Här kommer kompilatorn själv att se till att ditt primitiva värde (t.ex. en int) slås in / "wrappas" i ett objekt (t.ex Integer). Vi varnar för detta av två anledningar: (1) Innan man blir van vid att se var detta kan ske är det bra om inslagningen är explicit (t.ex. Integer.valueOf(myPrimitiveVar)). (2) Det kan hända att inslagningen sker på grund av en felaktig typdeklaration, till exempel att du har råkat ge en metod returtypen Boolean istället för boolean.

    IDEAs felbeskrivning säger att "Code which relies on auto-boxing will not work in pre-Java 5.0 environments", men detta är egentligen irrelevant för våra syften.

  • 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().

  • 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.

  • 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.

  • 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.

  • Class explicitly extends a Collection class – När man skapar en subklass till en Collection-klass (t.ex. class Foo extends List) öppnar man samtidigt upp för att andra kan manipulera den nya klassens objekt genom alla metoder som man ärver ner från Collection-klassen. 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.

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

  • 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, använd "List list = new ArrayList()" istället för "ArrayList list = new ArrayList()", så ökar du din egen frihet att ändra koden i framtiden!

  • 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.

  • Condition 'x instanceof Y' is redundant and can be replaced with '!= null' – testen x instanceof Y testar om x är ett verkligt objekt av typen Y. Den får alltså inte vara null, vilket inte är ett objekt alls. I det här fallet har IDEAs kodanalys redan visat att om x är ett objekt, måste det vara av typen Y. Alltså räcker det att testa att x verkligen är ett objekt, det vill säga x!=null.

  • 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".

  • 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 bero på att man har missat något och skrivit fel – kanske man 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, till exempel foo && bar && foo. Detta är sällan vad programmeraren menade, och kan indikera att man antingen ska ta bort en del av uttrycket eller ville ha ett annat villkor den andra gången.

  • Empty 'catch' block – Fel måste hanteras, inte bara fångas och ignoreras. I projektet finns specifika krav på detta för att nå upp till olika betyg.

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

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

    Detta program ger svaret false!

    Om du har satt ett exakt värde, och sedan testar om variabeln har detta exakta värde, är jämförelsen inget problem. I så fall kan du förklara detta vid varningen. Fråga handledaren om du är osäker.

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

  • 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.

  • 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.

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

  • 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.

  • 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.

  • 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.

  • Package naming convention – ska tala om när ett paketnamn (package xyz) bryter mot namnstandarden. Fråga handledaren om du är osäker på anledningen.

  • 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å.

  • Prohibited exception caught – Koden försöker fånga upp ett fel som normalt beror på felaktig programmering och som redan kan ha "förstört" något i resten av programmet. Detta kan till exempel gälla NullPointerException och IndexOutOfBoundsException. Istället för att fånga dessa ska man kontrollera i förväg om en pekare kan vara null, respektive kontrollera att indexet är korrekt innan man försöker indexera i en array eller lista.

    Samma gäller för IllegalArgumentException: Istället för att skicka in felaktiga argument och fånga IllegalArgumentException, är det bättre att kontrollera i förväg att argumentet är korrekt. (Och om man själv vill signalera ett problem som ska fångas upp, är det alltså normalt inte IllegalArgumentException man ska kasta för att göra den signaleringen. IllegalArgumentException ska kastas när någon har brutit mot metodens kontrakt genom att skicka in "uppenbart" felaktiga parametrar, t.ex. att en Timer ska anropa en metod var -20:e millisekund.)

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

  • 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.

  • 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.

  • 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.

  • 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.

  • 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.

  • 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.

  • 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).

  • 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.

  • 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.

  • 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) {
        // Instead of logging the message,
        // let's *assume* we know what happened... 
        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 e.printStackTrace(), skicka vidare ursprungsfelet med throw efter att du har städat upp, och/eller kasta en "chained exception".

    I undantagsfall (!) kan det vara rimligt att inte använda undantaget. I så fall kan man döpa variabeln till "ignore" istället för "e", så vet IDEA att man medvetet ignorerade den.

  • Unused Declaration – IDEA kan analysera vilka delar av ett program som faktiskt används. Då måste man veta var man egentligen utgår ifrån, dvs. varifrån någon kan "starta" programmet, till exempel från vilken main()-metod som helst i programmet. Sådana startpunkter kallas "entry points". Dessa är alltså inte fel i sig själva utan talar bara om var IDEA börjar leta efter kod som kan användas.

    Efter detta visas de fält, metoder och klasser som inte kan nås från någon entry point via metodanrop med mera. Om det syns något här som faktiskt används, beror det på att IDEA inte var medveten om någon entry point. Till exempel kan det hända att en enum-konstant enbart används indirekt via Enum.valueOf(String). Då står enum-konstanten aldrig i koden, och IDEA kan missa användningen. Tala isåfall om via en kommentar hur ni egentligen använder den kod som felaktigt har markerats som oanvänd.

    I övrigt är det meningen att ni ska ta bort oanvänd kod före inlämning, eftersom den är otestad och odemonstrerad.

  • Utility class is not final – En hjälpklass (utility class) ä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, eftersom dess objekt varken kan lagra eller göra något som man inte kan göra utan objektet. 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.


Sidansvarig: Jonas Kvarnström
Senast uppdaterad: 2018-02-12