Göm menyn

Att fixa problem

På föreläsningarna har vi diskuterat ett antal aspekter av bra objektorienterad (eller allmän) programmering. Fler aspekter diskuteras på olika platser här på websidorna.

Vi har sett under tidigare år att man även har nytta av övning i att åtgärda problematisk kod, annars finns alltför många problem kvar i slutinlämningen av projektet, vilket leder till onödiga kompletteringar och missade poäng. Vi har därför lagt till ett antal övningar på detta. Flera av övningarna är enkla och ibland vet man redan vad resultatet blir, men vår erfarenhet är att man ändå lär sig en hel del mer genom att göra och inte bara läsa.

Övningen är inte obligatorisk och behöver inte redovisas – men om ni gör misstag i det som vi övar på här, behöver ni antagligen komplettera projektet.

Om du genomför den här uppgiften och lägger koden i projektets Git-repo, lägg den då i ett paket som heter projprep (någonstans i namnet) så att våra verktyg kan sortera bort koden från själva projektet. Annars kan t.ex. samma kodkrav ställas även på denna kod.

Övning P.1: Magiska tal

Om magiska tal

Ett vanligt problem vi ser i inlämnad kod är att konstanter, speciellt numeriska konstanter, används som de är i koden utan att de får något namn. Dessa kan kallas magiska tal (magic numbers). Detta gör koden mer svårläst, och ofta behöver man då skriva kommentarer för att beskriva vad talen används till.

Exempel: Blanda en kortlek.

// Shuffle the deck (standard decks have 52 cards)
for (int i = 1; i <= 52; i++) {
    // 53 is the deck size + 1
    deck.swap(i, i + randomInt(53-i) - 1);
} 

Inför vi en namngiven konstant blir 52/53 självdokumenterande. Vi kan använda en lokal konstant (deklarerad final) för detta. Då blir det också enkelt att ändra värdet vid behov (i exemplet ovan kunde man annars ändra alla "52" men glömma "53").

// Shuffle the deck
final int deckSize = 52;
for (int i = 1; i <= deckSize; i++) {
    deck.swap(i, i + randomInt(deckSize+1-i) - 1;
}

Alternativt kan vi deklarera konstanten i klassen (till exempel private final static int DECK_SIZE = 52;) och sedan använda den där den behövs. Detta är särskilt användbart när samma konstant behövs i flera metoder.

// Shuffle the deck
for (int i = 1; i <= DECK_SIZE; i++) {
    deck.swap(i, i + randomInt(DECK_SIZE+1-i) - 1;
}

Uppgift: Magiska konstanter

  1. Bestäm dig för var du vill lägga din kod – på en egen plats (OK eftersom detta inte måste lämnas in) eller i projektet och dess repo. Om du lägger det i samma repo som projektet, skapa då ett paket som heter projprep för din kod, så att vi kan skilja ut förberedelserna från det faktiska projektet.

    Skapa en ny klass i ditt paket, med namnet TextViewer. Låt följande kod ersätta klassdeklarationen (men låt paketdeklarationen överst vara kvar):

    import javax.swing.*;
    import java.awt.*;
    
    public class TextViewer extends JComponent {
    
        public TextViewer() throws HeadlessException {
    	setPreferredSize(new Dimension(600, 300));
        }
    
        public static void main(String[] args) {
    	JFrame frame = new JFrame("TextViewer");
    	frame.add(new TextViewer());
    	frame.pack();
    	frame.setVisible(true);
        }
    
        @Override protected void paintComponent(final Graphics g) {
    	g.setColor(Color.BLACK);
    	g.setFont(new Font("serif", Font.PLAIN, 20));
    	g.drawString("This is the first line of the first paragraph of text.", 0, 20);
    	g.drawString("It is followed by the second line of text.", 0, 42);
    	g.drawString("Which is followed by the third line of text.", 0, 64);
    
    	g.drawString("Start of the second paragraph of text.", 0, 94);
    	g.drawString("Continuation of the second paragraph of text.", 0, 116);
    	g.drawString("Conclusion of the second paragraph of text.", 0, 138);
    
    	g.drawString("Start of the third paragraph of text.", 0, 168);
    	g.drawString("Continuation of the third paragraph of text.", 0, 190);
    	g.drawString("Conclusion of the third paragraph of text.", 0, 212);
        }
    } 

    Detta kunde till exempel vara en del av ett spel som ska skriva ut vissa värden på skärmen efter att det har ritat ut spelplanen. Därför använder vi inte en vanlig enkel textkomponent utan måste rita texten "manuellt".

  2. Testkör programmet. Som du ser ritas formatterad text i fönster. Storleken på texten är 20 pixlar, radavståndet mellan textraderna är 2 pixlar och mellan stycken läggs ytterligare 8 pixlar. Utritningen styrs genom anropen till drawString där sista parametrarna är x- resp. y-koordinaterna för texten.

  3. Ändra nu layout så att fontstorleken är 12 pixlar, radavståndet 1 pixel och avståndet mellan stycken är 4 pixlar. Gör detta genom att ändra koordinaterna manuellt.

  4. Vi inser att det är ganska jobbigt att arbeta med magiska konstanter som förekommer på många ställen. Vi börjar därför med att ta bort dessa successivt. Ersätt de magiska värdena 600 och 300 med konstanter som heter WIDTH och HEIGHT samt x-koordinaten 0 med LEFT_ALIGNED.

    Tips: Ställ markören i "600" och använd Refactor | Extract | Constant: Ctrl-Alt-C för att skapa en konstant på klassnivå.

  5. Textradernas y-koordinater är ju olika för varje anrop, så det kan verka meningslöst att ta ut dem som enskilda konstanter. Å andra sidan har de en viss struktur som vi kan använda för att göra det mycket enklare att göra förändringar: Y-värdena ökas hela tiden med textstorlek, radavstånd och eventuellt även styckeavstånd. Vi kan alltså göra det möjligt att räkna ut Y-koordinat genom att veta radnummer och styckenummer och sedan multiplicera dem med rätt värden.

    Skapa först konstanterna FONT_SIZE, ROW_DISTANCE och PARAGRAPH_DISTANCE och ge dem rätt värden enligt ovan.

  6. Skapa sedan int-fälten startX, paragraph och row och initialisera dem till LEFT_ALIGNED, 0 och 0.

  7. Implementera hjälpfunktionen drawRow(Graphics g, String text) så att den målar ut text på x-koordinaten startX och y-koordinaten (row+1)*FONT_SIZE+row*ROW_DISTANCE+paragraph*PARAGRAPH_DISTANCE. Låt även funktionen öka värdet på row så att nästa gång den anropas, hamnar texten en rad ner.

  8. Skriv om paintComponent så att den anropar hjälpfunktionen radvis och ökar paragraph när så behövs. Vi ändrar aldrig på startX i detta exempel.

Övning P.2: Redundant kod

Bakgrund P.2: Redundant kod

Det är också ganska vanligt att inlämnad kod innehåller mycket redundans. I kodexemplet nedan, en spelplan med metoder för att flytta nuvarande position åt fyra håll (så länge som inget är i vägen), finns flera exempel på det. Nu ska vi identifiera dessa exempel och prova några olika sätt att bli av med mycket av redundansen. Det ger effektivare kod som ofta blir lättare att underhålla.


public class GameBoard
{
    private enum CellType
    {
        EMPTY, TREE, BUILDING, ROCK, POWERUP, BORDER
    }

    private CellType[][] cells;
    private int currentX, currentY;

    public GameBoard(int width, int height) {
        this.cells = new CellType[height][width];
        this.currentX = width / 2;
        this.currentY = height / 2;
    }

    public void moveDown() {
        if (canMoveDown()) {
            currentY++;
        }
    }

    public void moveUp() {
        if (canMoveUp()) {
            currentY--;
        }
    }

    public void moveRight() {
        if (canMoveRight()) {
            currentX++;
        }
    }

    public void moveLeft() {
        if (canMoveLeft()) {
            currentX--;
        }
    }

    public boolean canMoveRight() {
        return !(cells[currentY][currentX + 1] == CellType.BORDER ||
                cells[currentY][currentX + 1] == CellType.TREE ||
                cells[currentY][currentX + 1] == CellType.BUILDING ||
                cells[currentY][currentX + 1] == CellType.ROCK);
    }

    public boolean canMoveLeft() {
        return !(cells[currentY][currentX - 1] == CellType.BORDER ||
                cells[currentY][currentX - 1] == CellType.TREE ||
                cells[currentY][currentX - 1] == CellType.BUILDING ||
                cells[currentY][currentX - 1] == CellType.ROCK);
    }

    public boolean canMoveDown() {
        return !(cells[currentY + 1][currentX] == CellType.BORDER ||
                cells[currentY + 1][currentX] == CellType.TREE ||
                cells[currentY + 1][currentX] == CellType.BUILDING ||
                cells[currentY + 1][currentX] == CellType.ROCK);
    }

    public boolean canMoveUp() {
        return !(cells[currentY - 1][currentX] == CellType.BORDER ||
                cells[currentY - 1][currentX] == CellType.TREE ||
                cells[currentY - 1][currentX] == CellType.BUILDING ||
                cells[currentY - 1][currentX] == CellType.ROCK);
    }
}

Uppgift: Redundant kod

  1. Skapa en ny klass i det paket som du skapade tidigare, med namnet GameBoard. Låt koden ovan ersätta klassdeklarationen.

  2. I första steget märker vi att det finns fyra metoder som kontrollerar om en specifik cell är av typ BORDER, TREE, BUILDING eller ROCK – ett hinder som gör att det inte går att flytta sig till just den angivna cellen. Det här är väldigt repetitivt och man borde definitivt göra något åt detta. Det finns flera olika åtgärder att välja mellan, t.ex. att skapa metoden boolean hasObstacle(int x, int y) som skulle kunna utföra själva testet.

    Den åtgärd vi väljer är att ändra modelleringen så att celltypen själv håller reda på om den är ett hinder eller inte. På det sättet blir det också enklare att lägga till nya "hindertyper", eftersom all information om dem anges på samma ställe (i enum-deklarationen). Här drar vi nytta av att enum-konstanter är objekt, och att enum-typer därför kan ha fält och konstruktorer. Eftersom vi inte har diskuterat detta så mycket ger vi lite extra hjälp och visar den nya definitionen av CellType som ska ersätta den gamla:

    private enum CellType
    {
        EMPTY(false), TREE(true), BUILDING(true), ROCK(true),
        POWERUP(false), BORDER(true);
    
        public final boolean isObstacle;
    
        CellType(final boolean isObstacle) {
            this.isObstacle = isObstacle;
        }
    }

    Notera att det är just BORDER, TREE, BUILDING och ROCK som konstrueras med parametern true, och som får ett sant värde på isObstacle.

    Din uppgift blir nu att anpassa de fyra canMoveXYZ()-metoderna så att de utnyttjar det nya fältet. De ska alltså inte titta på fyra specifika celltyper utan bara se om isObstacle är sann i just denna cell. Detta sparar ett antal rader för varje canMoveXYZ()-metod.

  3. De fyra canMoveXYZ()-metoderna är nu relativt korta, och frågan är om de verkligen behöver vara enskilda metoder. Samtidigt är det bra att ha ett namn på kontrollen om man kan flytta åt ett visst håll, och det namnet försvinner om vi t.ex. bara flyttar in all kod från canMoveRight() till moveRight(). Så vi provar ett annat grepp istället.

    Första steget är att vi inser att alla canMoveXYZ()-metoderna trots allt är väldigt lika. Behöver vi verkligen olika metoder för varje riktning vi kan flytta oss i? Nej, inte om riktningen representeras med parametrar istället! Vi inför därför metoden

    public boolean canMove(int deltaX, int deltaY) {
        return !cells[currentY + deltaY][currentX + deltaX].isObstacle;
    }

    Skriv om samtliga canMoveXYZ() så att de anropar canMove() med rätt parametrar. (Om man bara vill ändra x, så är deltaY lika med 0, och vice versa.)

    Ta sedan nästa steg och gör dig av med canMoveXYZ(). Detta kan IDEA hjälpa till med: Ställ markören i moveDown(), i anropet till canMoveDown(), och välj Refactor | Inline: Ctrl-Alt-N. Välj "Inline all invocations and remove the method". Gör samma för de övriga tre metoderna. Nu har vi bara kvar moveXYZ() och canMove(deltaX,deltaY).

  4. Om vi nu kan göra alla canMoveXYZ() till en enda metod, kan vi inte göra samma sak för moveXYZ()? Jo, självklart. Och då kan vi ta ett steg till genom att införa namn på förflyttningarna, så att andra klasser inte behöver jobba direkt med koordinater. Inför den här enum-typen under (inte inuti) CellType:

    public enum Move {
        DOWN(0,1), UP(0,-1), RIGHT(1,0), LEFT(-1,0);
    
        public final int deltaX;
        public final int deltaY;
    
        Move(final int deltaX, final int deltaY) {
            this.deltaX = deltaX;
            this.deltaY = deltaY;
        }
    }

    Nu har du fria händer att se till att det blir en move()-metod som tar en Move som parameter, och en canMove()-metod som också tar en Move som parameter.

    Notera att detta fungerar utmärkt oavsett antalet förflyttningar man kan flytta sig i. Även om man lägger till fyra nya diagonala förflyttningar, eller möjligheten att hoppa flera steg i någon riktning, behöver man inte lägga till nya metoder. Det har vi åstadkommit genom att bland annat göra en förflyttning till en sak, ett objekt.

Övning P.3: Felaktig initialisering

Bakgrund P.3: Felaktig initialisering

Som vi har diskuterat på föreläsningarna är det tänkt att varje klass ska ha kontroll över sina egna objekt, inklusive initialiseringen av alla fält som ingår i objekten. Eftersom vi ofta får projekt där detta har missats kommer nu en enkel övning i att fixa till det problemet. Fält i klass A ska initialiseras i klass A:s konstruktor. Om vi har "class B extends A" så ska inte klass B:s konstruktor peta på dessa fält direkt. Istället ska den skicka informationen vidare till superklassens konstruktor med super(...), eftersom A själv ska vara ansvarigt för att initialisera sina egna medlemmar.

Uppgift: Felaktig initialisering

Skapa följande klasser (i varsin fil):

/**
 * A movable object on the screen, with current x and y coordinates.
 */
public class MovableObject
{
    protected int x, y;

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    public void moveTo(int x, int y) {
        this.x = x;
        this.y = y;
    }
}
public class PlayerSprite extends MovableObject
{
    private final String name;

    public PlayerSprite(final String name, final int x, final int y) {
        this.name = name;
        this.x = x;
        this.y = y;
    }
}

Som ni ser ärver PlayerSprite ner två fält, x och y, från MovableObject. Konstruktorn för PlayerSprite vill initialisera de fälten, men gör detta på fel sätt: Genom att själv tilldela värden till x och y. Se till att det istället finns en konstruktor i MovableObject som tar x och y som parameter, och att konstruktorn för PlayerSprite anropar denna genom "super(...)" så som vi har diskuterat på föreläsningarna.

Övning P.4: Använd enum

Bakgrund P.4: Använd enum

Om en variabel bara ska kunna anta ett litet antal förutbestämda värden bör den normalt vara en enum, inte en sträng eller ett heltal. På det sättet kan Java garantera att variabeln faktiskt bara kan anta just dessa värden.

Uppgift: Använd enum

Skapa följande klass:

public class Player
{
    public static final int SPEED_SLOW = 0;
    public static final int SPEED_MEDIUM = 1;
    public static final int SPEED_FAST = 2;

    public static final String MODE_NORMAL = "Normal";
    public static final String MODE_GHOST = "Ghost";
    public static final String MODE_INVULNERABLE = "Invulnerable";

    private int x, y;
    private int speed = SPEED_MEDIUM;
    private String mode = MODE_NORMAL;

    public int getSpeed() {
        return speed;
    }

    public void setSpeed(final int speed) {
        this.speed = speed;
    }

    public String getMode() {
        return mode;
    }

    public void setMode(final String mode) {
        this.mode = mode;
    }

    public void moveRight() {
        switch (speed) {
            case SPEED_SLOW:
                x += 5;
                break;
            case SPEED_MEDIUM:
                x += 10;
                break;
            case SPEED_FAST:
                x += 20;
                break;
        }
    }

    /**
     * Describe current speed and mode -- used for a status display
     */
    public String getDescription() {
        StringBuilder buf = new StringBuilder();
        buf.append("Player is ");

        switch (speed) {
            case SPEED_SLOW:
                buf.append("SLOW");
                break;
            case SPEED_MEDIUM:
                buf.append("MEDIUM");
                break;
            case SPEED_FAST:
                buf.append("FAST");
                break;
        }

        buf.append(" and ");
        
        switch (mode) {
            case MODE_NORMAL:
                buf.append("NORMAL");
                break;
            case MODE_GHOST:
                buf.append("GHOST");
                break;
            case MODE_INVULNERABLE:
                buf.append("INVULNERABLE");
                break;
        }
        
        return buf.toString();
    }

    public static void main(String[] args) {
        final Player player = new Player();
        System.out.println(player.getDescription());
    }
}

Testkör.

Här ser du att hastigheter representeras som heltal, trots att det egentligen bara är tre värden som accepteras. Set-metoden kontrollerar inte att värdet är korrekt (man kan sätta speed till 42 om man vill), och den som anropar getSpeed() måste jämföra returvärdet med konstanterna SPEED_SLOW, SPEED_MEDIUM och SPEED_FAST för att se vad värdet egentligen betyder.

Skriv om detta genom att införa en enum-typ för hastigheter, med tre värden (SLOW, MEDIUM, FAST). Nästla inte denna inuti Player, utan gör den till en självständig typ som ligger i en egen fil.

Dra nytta av att ett enum-värde har en toString()-metod som returnerar namnet på värdet, så att getDescription() kan förenklas: Du behöver inte längre en switch-sats för speed, utan kan direkt lägga in enum-värdets namn i strängbufferten.

Gör samma sak för mode-värdena.

Testkör igen och se att du får rätt resultat.

Övning P.5: Wrapper-typer i onödan

Bakgrund P.5: Wrapper-typer i onödan

Java har en primitiv datatyp som heter boolean. Det finns också en objekttyp som heter Boolean, med stor bokstav. Eftersom denna innehåller en primitiv boolean kan den kallas för en wrapper (ett "omslag", "hölje").

Det finns några specifika fall där objekttypen behöver användas, men som vi kommer att diskutera på föreläsningarna vill man i nästan alla fall använda de primitiva datatyperna, som är snabbare och använder mindre minne.

Uppgift: Wrapper-typer i onödan

Skapa följande klass:

public class NumberTester
{
    private NumberTester() {}

    public static Boolean isEven(Integer number) {
        Integer remainder = number % 2;
        return remainder == 0;
    }

    public static void main(String[] args) {
        System.out.println(isEven(17));
        System.out.println(isEven(42));
    }
}

Testkör klassen. Den fungerar, men vid anropet till isEven(17) måste värdet 17 konverteras till ett objekt. Vid uträkningen av "number % 2" måste Integer-objektet konverteras tillbaka till ett primitivt heltal, och resultatet måste sedan konverteras till ett objekt (remainder). Jämförelsen med 0 resulterar i en boolean, som sedan måste konverteras till ett Boolean-objekt och returneras.

Ändra sedan Boolean och Integer till motsvarande primitiva typer. Testkör igen. Resultatet ska bli samma, men inga onödiga konverteringar sker.

(Ja, detta är en extremt simpel övning -- tanken är som sagt att man minns detta mycket bättre om man faktiskt har testat i praktiken, hur enkelt testet än är!)

Övning P.6: Instanceof-kedjor

Bakgrund P.6: Instanceof-kedjor

Javas instanceof-operator låter oss testa om en variabels nuvarande värde tillhör en viss klass eller ett visst gränssnitt. Detta är ett kraftfullt verktyg, men ofta är det fel verktyg. En av de stora styrkorna hos objektorientering är ju just att man inte behöver ta reda på vilken typ ett objekt har för att kunna göra något typ-specifikt: Man använder istället subtypspolymorfism och dynamisk bindning. Nu ska vi testa detta i praktiken.

Uppgift: Instanceof-kedjor

Lägg till följande klasser. Eftersom Game återanvänder klassen Player, från tidigare uppgifter, behöver ändra parametrarna till setSpeed() och setMode() i Game till att använda de enum-typer du införde där.

import java.util.ArrayList;
import java.util.List;

public class Game
{
    private Player player = new Player();
    private List<Powerup> activePowerups = new ArrayList<>();

    public void playerHitPowerup(Powerup power) {
        activePowerups.add(power);
        if (power instanceof SpeedPowerup) {
            player.setSpeed(Player.SPEED_FAST);
        } else if (power instanceof GhostPowerup) {
            player.setMode(Player.MODE_GHOST);
        } else if (power instanceof InvulnerablePowerup) {
            player.setMode(Player.MODE_INVULNERABLE);
        } else {
            throw new IllegalArgumentException("Unknown powerup: " + power);
        }
    }
}
import java.awt.*;

public interface Powerup
{
    String getDescription();

    public void paint(Graphics g, int x, int y);
}
import java.awt.*;

public class SpeedPowerup implements Powerup
{
    @Override public String getDescription() {
        return "Makes a player faster";
    }

    public void paint(Graphics g, int x, int y) {
        g.fillRect(x, y, 10, 10);
    }
}
import java.awt.*;

public class GhostPowerup implements Powerup
{
    public void paint(Graphics g, int x, int y) {
        g.fillOval(x, y, 10, 10);
    }

    @Override public String getDescription() {
        return "Makes a player into a ghost that can travel through walls";
    }
}
import java.awt.*;

public class InvulnerablePowerup implements Powerup
{
    public void paint(Graphics g, int x, int y) {
        g.fillOval(x, y, 10, 10);
    }

    @Override public String getDescription() {
        return "Makes a player invulnerable for a while";
    }
}

Problemet med denna kod är att vi har samlat centraliserad information om alla existerande Powerup-typer i Game-klassen. Om man lägger till en ny Powerup-typ måste man alltså även ändra i playerHitPowerup(), som är tänkt att anropas när spelaren träffade på en powerup på spelplanen. I ett fullständigt projekt skulle detta så klart också kunna inträffa i flera andra metoder. I stället vill vi göra koden så modulär så möjligt!

Lösningen är enkel: Inför en ny metod i Powerup-gränssnittet, som vi kan kalla för playerHitMe(). Den behöver så klart inte ta en Powerup som parameter, eftersom den finns i en Powerup. Däremot behöver den ta spelaren som parameter. Se sedan till att det finns en konkret implementation av playerHitMe() i var och en av Powerup-klasserna. Där vet ju klassen vilken typ av Powerup det gäller (SpeedPowerup vet att den ska sätta hastigheten), så inga instanceof behövs.

Den långa kedjan av instanceof-tester i playerHitPowerup() kan nu reduceras till en enda rad: power.playerHitMe(player). Anropet är möjligt, eftersom metoden finns i Powerup-gränssnittet, och rätt implementation anropas, på grund av dynamisk bindning.

När detta är fixat är det möjligt att lägga till nya Powerups utan att ändra Game-klassen.

Vissa undantag kan finnas där det verkligen är mycket svårt att undvika kedjor av instanceof, men i de flesta fall vi har sett (minst 90%) fungerar polymorfism utmärkt och SKA användas. Fråga examinatorn om ni är tveksamma.

Labb av Jonas Kvarnström 2016–2024.


Sidansvarig: Jonas Kvarnström
Senast uppdaterad: 2024-02-04