[CICLO 6] Test Driven Development task 6.1.4 (DanieleC88 vs 71104)

[71104]

come da titolo

la build per ora potrebbe essere precaria, ma cercheremo di lasciarla sempre verde: chi scrive i test non committa, mentre committa solo chi li implementa

dunque, fatemi pensare una test list:
- verificare che Grid si accorga se una gemma sta alla sinistra di una del suo stesso tipo
- verificare bla bla bla destra bla bla bla
- verificare bla bla sopra
- verificare sotto
- verificare che Grid illumini la gemma se si accorge che sopra, sotto, a destra o a sinistra c'è una gemma dello stesso tipo

[DanieleC88]

bla bla bla destra bla bla bla? come sei esplicativo...

[DanieleC88]

Dimenticavo: ovviamente vanno testate anche le condizioni in cui la gemma NON deve essere illuminata.

[71104]

giusto, quindi la test list diventerebbe così:

- verificare che Grid si accorga se una gemma sta alla sinistra di un'altra dello stesso tipo
- verificare a destra
- verificare sopra
- verificare sotto
- verificare che Grid si accorga quando una gemma non sta a destra, sinistra, sopra o sotto una dello stesso tipo (sempre test separati: infatti mi sa che è meglio metterli in una classe apposta perché sono tanti)
- verificare che quando una gemma sta accanto ad almeno una dello stesso tipo si illumini
- verificare che questo non accada se una gemma non sta accanto a nessuna dello stesso tipo

ecco il primo test:

    public void testSameAsLeft()
    {
        grid.insertGem(5, 3, gem1);
        grid.insertGem(5, 4, gem2);
        assertTrue(grid.sameAsLeft(gem2));
    }

si trova in TestGrid ma è meglio che lo sposto in una nuova classe TestGemBrightnessInGrid.

[71104]

primo commit, perfetto; Daniele ha risolto questo test col seguente codice (un po' modificato da me per far felice Ant e per correggere un erroretto minimo ^^):

    public boolean gemIsSameAsLeft(Gem gem)
    {
        int column = gem.getCellColumn();
        int row = gem.getCellRow();
        
        if(column < 1)
        {
            return false;
        }
        
        if(getGemAt(row, column).getName().equals(getGemAt(row, column - 1).getName()))
        {
            return true;
        }
        
        return false;
    }

[71104]

ok, allora, ho fatto una nuova classe TestGemBrightnessInGrid e spostato il mio test in quest'ultima:

public class TestGemBrightnessInGrid extends TestCase
{
    private Grid grid;
    
    private Gem gem1;
    private Gem gem2;
    
    
    public void setUp()
    {
        grid = Grid.createForTesting();
        
        gem1 = new Gem("diamond");
        gem2 = new Gem("diamond");
        
    }
    
    
    public void testSameAsLeft()
    {
        grid.insertGem(5, 3, gem1);
        grid.insertGem(5, 4, gem2);
        assertTrue(grid.gemIsSameAsLeft(gem2));
    }
    
}

nel metodo setUp non ho usato Gem.createForTesting perché ho preferito esplicitare il tipo di gemma nella creazione; infatti tra poco quando faremo i test opposti dovremo anche creare una terza Gem nbella classe di test che dovrà essere di tipo diverso

[DanieleC88]

Benissimo, ora non ci resta che testare anche gli altri lati, ci vorrà poco.

    public void testSameAsRight()
    {
        grid.insertGem(5, 3, gem1);
        grid.insertGem(5, 4, gem2);
        assertTrue(grid.gemIsSameAsRight(gem1));
    }


    public void testSameAsTop()
    {
        grid.insertGem(5, 4, gem1);
        grid.insertGem(6, 4, gem2);
        assertTrue(grid.gemIsSameAsTop(gem2));
    }


    public void testSameAsBottom()
    {
        grid.insertGem(5, 4, gem1);
        grid.insertGem(6, 4, gem2);
        assertTrue(grid.gemIsSameAsBottom(gem1));
    }

[71104]

et voilà:

    public boolean gemIsSameAsRight(Gem gem)
    {
        int column = gem.getCellColumn();
        int row = gem.getCellRow();
        
        if(column >= grid[0].length)
        {
            return false;
        }
        
        if(getGemAt(row, column).getName().equals(getGemAt(row, column + 1).getName()))
        {
            return true;
        }
        
        return false;
    }

    public boolean gemIsSameAsDown(Gem gem)
    {
        int column = gem.getCellColumn();
        int row = gem.getCellRow();
        
        if(row > grid.length)
        {
            return false;
        }
        
        if(getGemAt(row, column).getName().equals(getGemAt(row + 1, column).getName()))
        {
            return true;
        }
        
        return false;
    }

    public boolean gemIsSameAsUp(Gem gem)
    {
        int column = gem.getCellColumn();
        int row = gem.getCellRow();
        
        if(row < 1)
        {
            return false;
        }
        
        if(getGemAt(row, column).getName().equals(getGemAt(row - 1, column).getName()))
        {
            return true;
        }
        
        return false;
    }

a questo punto uno pensa che stiamo già a metà del task e invece MANCOPENNIENTE!!!
carissimo Daniele, prima di continuare seguendo la test list ti sottopongo questo problema:

    public void testAffinityCheckWithEmtpyCell()
    {
        grid.insertGem(5, 5, gem1);
        assertFalse(grid.gemIsSameAsLeft(gem1));
    }

[71104]

a proposito, sarebbe bene anche riflettere su qualche modo per eliminare quella mostruosa quadruplicazione di codice, ma si tratta di un refactoring e ci penseremo a fine task.

EDIT: anzi, è talmente semplice che ci ho già pensato

[DanieleC88]

Per la quadruplicazione di codice non credo ci voglia molto... per il problema delle caselle vuote, be', cosa dire? Il problema è grande ma la soluzione è piccola. Un banalissimo 'if'.

    public boolean gemIsSameAsLeft(Gem gem)
    {
        int column = gem.getCellColumn();
        int row = gem.getCellRow();

        if(column < 1)
        {
            return false;
        }

        if(getGemAt(row, column - 1) != null)
        {
            if(getGemAt(row, column).getName().equals(
                getGemAt(row, column - 1).getName()))
            {
                return true;
            }
        }
        return false;
    }

E così anche per gli altri.

[Ufo13]

hmmm puoi ancora eliminare un return

[cover]

Raga, mettete i test sotto it.diamonds.tests.ignore, almeno non ci sono problemi con la build....e fek non si arrabbia

[71104]

allora, DanieleC88 si è dovuto disconnettere percui il task dovrò essere completato domani; ho eliminato la quadruplicazione sperando (come suggerito da VICIUS) in un miracoloso fix ma Spartacus mi ha detto sorry, oggi niente miracoli
ora provo ad aggiustare la build; nel frattempo ecco il codice dei 4 metodi riassunti in un unico metodo:

    public boolean gemsAreSame(Gem gem, int xStep, int yStep)
    {
        int column = gem.getCellColumn();
        int row = gem.getCellRow();

        int otherColumn = column + xStep;
        int otherRow = row + yStep;

        if((otherColumn < 0) || (otherColumn >= grid[0].length))
        {
            return false;
        }

        if((otherRow < 1) || (otherRow >= grid.length))
        {
            return false;
        }

        if(getGemAt(otherRow, otherColumn) != null)
        {
            if(gem.getName().equals(getGemAt(otherRow, otherColumn).getName()))
            {
                return true;
            }
        }

        return false;
    }

ed ecco invece come è diventata la classe dei test:

public class TestGemBrightnessInGrid extends TestCase
{
    private Grid grid;

    private Gem gem1;

    private Gem gem2;


    public void setUp()
    {
        grid = Grid.createForTesting();

        gem1 = new Gem("diamond");
        gem2 = new Gem("diamond");

    }


    public void testSameAsLeft()
    {
        grid.insertGem(5, 3, gem1);
        grid.insertGem(5, 4, gem2);
        assertTrue(grid.gemsAreSame(gem2, -1, 0));
    }


    public void testSameAsRight()
    {
        grid.insertGem(5, 3, gem1);
        grid.insertGem(5, 4, gem2);
        assertTrue(grid.gemsAreSame(gem1, 1, 0));
    }


    public void testSameAsUp()
    {
        grid.insertGem(5, 4, gem1);
        grid.insertGem(6, 4, gem2);
        assertTrue(grid.gemsAreSame(gem2, 0, -1));
    }


    public void testSameAsDown()
    {
        grid.insertGem(5, 4, gem1);
        grid.insertGem(6, 4, gem2);
        assertTrue(grid.gemsAreSame(gem1, 0, 1));
    }


    public void testAffinityCheckWithEmtpyCell()
    {
        grid.insertGem(5, 5, gem1);
        assertFalse(grid.gemsAreSame(gem1, 1, 0));
        assertFalse(grid.gemsAreSame(gem1, 0, 1));
    }

}

[71104]

magia, si è fixata da sola
tanto meglio

ci vediamo domani per la seconda puntata; byez

[fek]

In questo metodo:

    public boolean gemsAroundOfSameType(Gem gem, int column, int row)
    {
        int otherColumn = gem.getCellColumn() + column;
        int otherRow = gem.getCellRow() + row;

        if((otherColumn < 0) || (otherColumn >= grid[0].length))
        {
            return false;
        }

        if((otherRow < 1) || (otherRow >= grid.length))
        {
            return false;
        }

        if(getGemAt(otherRow, otherColumn) != null)
        {
            if(gem.isSameType(getGemAt(otherRow, otherColumn)))
            {
                return true;
            }
        }

        return false;
    }

I primi due branch non sono testati da alcun test. Aggiungete i test relativi ragazzi. E, soprattutto, state scrivendo troppo codice fra un test e l'altro. Fate passi piu' piccoli.

(Ho fatto un po' di refactoring)

[Ufo13]

non è meglio unire le condizioni dei 2 if annidati con un &&?

[fek]

Originariamente inviato da Ufo13

non è meglio unire le condizioni dei 2 if annidati con un &&?

In genere preferisco svolgere gli if per chiarezza:

    public boolean gemsAroundOfSameType(Gem gem, int column, int row)
    {
        int otherColumn = gem.getCellColumn() + column;
        int otherRow = gem.getCellRow() + row;

        if((otherColumn < 0) || (otherColumn >= grid[0].length))
        {
            return false;
        }

        if((otherRow < 1) || (otherRow >= grid.length))
        {
            return false;
        }

        if(getGemAt(otherRow, otherColumn) == null)
        {
           return false;
        }
        
        if(gem.isSameType(getGemAt(otherRow, otherColumn)))
        {
            return true;
        }

        return false;
    }

[71104]

Originariamente inviato da fek

I primi due branch non sono testati da alcun test. Aggiungete i test relativi ragazzi.

hai ragione, ci sarei arrivato alla fine ^^
dobbiamo aggiungere un test che verifichi che venga lanciata eccezione se vengono passati una riga e una colonna fuori dalla griglia; per ora ritorna false, ma è un comportamento "temporaneo".

(Ho fatto un po' di refactoring)

ho visto e concordo

[DanieleC88]

Originariamente inviato da Ufo13

non è meglio unire le condizioni dei 2 if annidati con un &&?

Si, si potrebbe, ma in genere credo sia meglio spezzettare il codice per renderlo quanto più leggibile si può.

Colgo l'occasione per scusarmi del casino che ho fato ieri, ma purtroppo andavo di fretta, oggi spero di avere più tempo.

[71104]

Originariamente inviato da 71104

hai ragione, ci sarei arrivato alla fine ^^
dobbiamo aggiungere un test che verifichi che venga lanciata eccezione se vengono passati una riga e una colonna fuori dalla griglia; per ora ritorna false, ma è un comportamento "temporaneo".

ma cos'ero, fuso ieri??
non deve mica lanciare eccezione: se le coordinate vanno fuori significa semplicemente che la gemma che ho dato come parametro sta sui bordi
per testare gli if basta semplicemente creare dei test che testino il metodo con una gemma messa ai bordi
Daniele, tocca provvedere