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!

Många inspektionsvarningar förklaras också i den utökade kodanalysen som ni automatiskt får i issues i era projekt!

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.

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

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

  • 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 '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". Denna inspektion ska vara avstängd.

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

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

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

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

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

  • 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: 2020-05-15