Programma "Refactor This!" [Da leggere]

[71104]

Bonfo, i metodi isStoneTransforming e isStoneTransformed in realtà agiscono solamente sulla stone, non sulla action; io li sposterei in Stone rinominandoli rispettivamente isTransforming e isTransformed.

[Bonfo]

C'ho pensato pure io, ma, non so se l'hai notato, c'e' un cast di mezzo.
Io aspetteri di vedere come risolviamo il problema del getType e isXXX.

Se valutate che conviene farlo subito...
... cambialo subito, ma poi devi aggiungere i test

[thebol]

spianato il primo TODO: REFACTOR THIS di TestGameRestartOnGameOver

ho rinominato la classe di test in TestGameOnGameOver (visto che il metodo testava il gameOver a partire da loop).

Da:

   public void testPassToGameOverState() throws IOException
    {
        // TODO: REFACTOR THIS

        LayerManager oldLayerManager = loop.getLayerManager();

        fillFourthColumn();

        environment.getTimer().advance(newGemDelay);
        loop.doOneStep();

        // verifico che il restart nn sia ancora stato fatto
        assertEquals(oldLayerManager, loop.getLayerManager());
        assertEquals(field1, loop.getPlayFieldOne());
        assertEquals(field2, loop.getPlayFieldTwo());

        environment.getTimer().advance(restartGameDelay - 2);
        loop.doOneStep();

        // verifico che il restart nn sia ancora stato fatto
        assertEquals(oldLayerManager, loop.getLayerManager());
        assertEquals(field1, loop.getPlayFieldOne());
        assertEquals(field2, loop.getPlayFieldTwo());

        environment.getTimer().advance(1);
        loop.doOneStep();

        assertNotSame("The layer must be different from the old one",
            oldLayerManager, loop.getLayerManager());
        // TODO manca test che � stato settato il background(come si fa?)
        assertNotSame("field1 must be different from the old one", field1,
            loop.getPlayFieldOne());
        assertNotSame("field2 must be different from the old one", field2,
            loop.getPlayFieldTwo());
    }

A:

    public void testPassToGameOverOnColumnFull() throws IOException
    {
        fillFourthColumn();
        environment.getTimer().advance(newGemDelay);
        loop.doOneStep();
        assertTrue(loop.getPlayFieldOne().getGridController().isGameOver());
    }

    
    public void testPassToGameOverNotRestartGame() throws IOException
    {
        fillFourthColumn();
        environment.getTimer().advance(newGemDelay);
        loop.doOneStep();
        
        assertEquals(layerManager, loop.getLayerManager());
        assertEquals(field1, loop.getPlayFieldOne());
        assertEquals(field2, loop.getPlayFieldTwo());
    }
    
    
    public void testDelayBeforeRestart() throws IOException
    {
        fillFourthColumn();
        environment.getTimer().advance(newGemDelay);
        loop.doOneStep();
        
        environment.getTimer().advance(restartGameDelay - 2);
        loop.doOneStep();

        assertEquals(layerManager, loop.getLayerManager());
        assertEquals(field1, loop.getPlayFieldOne());
        assertEquals(field2, loop.getPlayFieldTwo());
    }
    
    
    public void testRestartAfterDelay() throws IOException
    {
        fillFourthColumn();
        environment.getTimer().advance(newGemDelay);
        loop.doOneStep();
        
        environment.getTimer().advance(restartGameDelay-1);
        loop.doOneStep();

        assertNotSame("The layer must be different from the old one",
            layerManager, loop.getLayerManager());
        assertNotSame("field1 must be different from the old one", field1,
            loop.getPlayFieldOne());
        assertNotSame("field2 must be different from the old one", field2,
            loop.getPlayFieldTwo());
    }

non chiedetemi perchè basta far avanzare il timer di restartGameDelay(preso dal config) -1 per far scattare il gameOver, su questo non ho indagato, ma ricordo che "era così".

ho anche eliminato il todo che diceva che si doveva testare l'apparizione dell'immagine del game over, ma nn si sapeva come(ho la strana sensazione di averlo scritto io..fra l'altro....

    public void testNotShowGameOverImageBeforeGameOver() throws IOException
    {
        Image gameOverImage = environment.getEngine().createImage(GameOverBox.texturePath);
        assertFalse(mockEngine.wasImageDrawn(gameOverImage));
    }
    
    
    public void testShowGameOverImageOnGameOver() throws IOException
    {
        fillFourthColumn();
        environment.getTimer().advance(newGemDelay);
        loop.doOneStep();
        
        Image gameOverImage = environment.getEngine().createImage(GameOverBox.texturePath);
        assertTrue(mockEngine.wasImageDrawn(gameOverImage));
    }
    
    public void testNotShowGameOverAfterRestart() throws IOException
    {
        fillFourthColumn();
        environment.getTimer().advance(newGemDelay);
        loop.doOneStep();
        
        environment.getTimer().advance(restartGameDelay-1);
        loop.doOneStep();
        
        Image gameOverImage = environment.getEngine().createImage(GameOverBox.texturePath);
        assertFalse(mockEngine.wasImageDrawn(gameOverImage));
    }

unici accorgimenti per questo test, è stato cambiare il metodo di MockEngine clearDisplay in

    public void clearDisplay()
    {
        drawInfoList = new ArrayList<DrawInfo>();
        numberOfQuadsDrawn = 0;
    }

prima non veniva pulita la drawInfoList, per cui mi ritrovavo il gameover anche dopo il restart...

In piu ho messo public static final GameOverBox.texturePath, per avere accesso all'nome texture/immagine.

C'è un altro test che usa la tecnica environment.getEngine().createImage(NOME_TEXTURE) e che hanno il nome texture hardcoded nel test(come nel mio caso il nome texture era private static). Non so quale modo sia migliore...(anche se io voto mettere i nomi texture public static final)

[Bonfo]

Originariamente inviato da thebol

ho anche eliminato il todo che diceva che si doveva testare l'apparizione dell'immagine del game over, ma nn si sapeva come(ho la strana sensazione di averlo scritto io..fra l'altro....

Sei sicuro che non ci sia gia' qualcos'altro di simile in giro??
Mi ricordo che avevamo trovato un doppione... controlla, altrimenti signifca che cosi' va benissimo

[thebol]

Originariamente inviato da Bonfo

Prima:
Dopo:

    protected void applyOn(Droppable gem)
    {
        if(!gem.getGridObject().getType().isStone() || !isStoneTransforming(gem))
        {
            return;
        }

        turningStonesCounter++;

        if(isStoneTransformed(gem))
        {
            turningStonesCounter--;
            turnStoneInToGem(gem);
        }

        gem.update(timer.getTime());
    }


    private void turnStoneInToGem(Droppable stone)
    {
        getGridElements().remove(stone);

        Droppable gem = gemFactory.create(GEM, stone.getGridObject().getColor());
        gem.getFallingObject().drop();

        int row = stone.getRegion().getTopRow();
        int column = stone.getRegion().getLeftColumn();
        getGrid().insertDroppable(gem, row, column);
    }


    private boolean isStoneTransforming(Droppable stone)
    {
        int frame = stone.getAnimatedSprite().getCurrentFrame();
        return frame >= 5 && frame < 7;
    }


    private boolean isStoneTransformed(Droppable stone)
    {
        int frame = stone.getAnimatedSprite().getCurrentFrame();
        return frame >= 7;
    }

ho trovato un bug(vedi allegato)

non ne sono sicuro, ma potrebbe essere questa modifica a farlo saltare fuori.

ps.si lo so c'è un volontario per il test del bug...però se va bene domani
mattina..

pps.mancava l'allegato

[Bonfo]

Il bug penso di aver capito dov'è.
Prima con frame = 7 riusciva comunque a passare il primo if, ovvero

if(frame < 5 ) {
return;
}

mentre invece adesso ci casca dentro in pieno.

if(frame < 5 || frame >= 7) {
return;
}

Dovrei scrivere il test, ma vorrei mazzare un po' bigGem.

Spostiamo la discussione di questo nel thread dei Bug

[Bonfo]

Perchè AbstractDroppable non si tiene una reference al DroppableDescription invece di usarlo solo per inizzializzare i membri??

[fek]

Originariamente inviato da Bonfo

Perchè AbstractDroppable non si tiene una reference al DroppableDescription invece di usarlo solo per inizzializzare i membri??

Perche' l'ho dimenticato
Se vedi una cosa cosi' semplice, fallo senza problemi. Ieri ho rifattorizzato 20 classi a mezzanotte, qualcosa puo' scappare.

[Bonfo]

Prima:

    protected boolean canMoveButNotWithFullGravity(Grid grid)
    {
        // TODO: REFACTOR THIS

        float gridBottom = grid.getRowUpperBound(grid.getNumberOfRows() - 1);

        int numberOfRowsOverOne = region.getBottomRow() - region.getTopRow();

        if (getSprite().getPosition().getY() + numberOfRowsOverOne
            * Cell.SIZE_IN_PIXELS + ((float)grid.getActualGravity()) / 2 > gridBottom)
        {
            return true;
        }

        if (getSprite().getPosition().getY() + ((float)grid.getActualGravity())
            / 2 <= grid.getRowUpperBound(getRegion().getTopRow()))
        {
            return false;
        }

        Cell cell = new Cell(getRegion().getBottomRow() + 1, getRegion().getLeftColumn());

        return grid.isDroppableAt(cell);
    }

Dopo:

      protected boolean canMoveButNotWithFullGravity(Grid grid)
    {
        float nextPositionY = getSprite().getPosition().getY() + grid.getActualGravity() * 0.5f;
        float limit = grid.getRowUpperBound(grid.getNumberOfRows() - getRegion().getHeight());

        if (nextPositionY > limit)
        {
            return true;
        }

        float currentRowLimit = grid.getRowUpperBound(getRegion().getTopRow());

        if (nextPositionY <= currentRowLimit)
        {
            return false;
        }

        Cell cell = new Cell(getRegion().getBottomRow() + 1, getRegion().getLeftColumn());

        return grid.isDroppableAt(cell);
    }

Ricordo i due problemi, entrambi da testare.
- il supporto a gravita' che producano step solo <= 1 cella ( visibile nella costruzione della cella)
- il fatto che si controlli solo la 1 column, probabilemnte causa del bug 6.

[^TiGeRShArK^]

Originariamente inviato da fek

La code base sta pian piano migliorando, soprattutto Droppable, a seguito di un po' di colpi di refactoring. Ma durante il refactoring giornaliero ho avuto molti problemi sia con BigGem sia con vari test.

Visto che affidarsi al vostro bon cuore non sta sortendo effetti troppo visibile, la soluzione e' il programma "Refactor This!"

cazz.. avevo in cameretta proprio quel poster...
ma la scritta non me la ricordavo..

[Baol]

Comincio a lavorare sul refactoring della CrushByChestAction

EDIT: ho committato, anche se ho lasciato il REFACTOR THIS perchè c'è ancora lavoro da fare.
In particolare pensavo di mettere in Droppable un metodo getAdjacentDroppable che restituisse una DroppableList con le droppable adiacenti (e un flag se sono richieste le droppable dello stesso colore, oppure un altro metodo apposito). Con un metodo simile il codice della CrushByChestAction si dimezzerebbe e saremmo più vicini alla sua eliminazione. Si può andare in questa direzione?

[Bonfo]

Mi sono preso la libertà di aggiungere un paio di Refactor this

Poi ho aggiunto il metodo create() a MockSingleDroppable, che vedo sostiuirsi benissimo al createGem() di GridTestCase, poi, con il refactoring che ho fatto su BigGem, anche la loro creazione dovrebbe essere molto più semplice, facendo si che si possano eliminare tutti qui metodi strani da GridTestCase... che secondo me dimagrisce un bel po'.

Inoltre stavo pensando di valutare bene cosa stanno a fare li in nostri TestCase custom.
Ovvero a parte il GridTestCase che fa tutto, gli altri li vedo un po' "inutili", il che vuol dire 2 cose:
- o li stiamo usando male,
- o non servono a un granchè.

In ogni caso bisogna "omogeneizzare" i test

Io un po' alla volta sto cercando di eliminare le dipendenze. Ad esempio è inutile usare GridTestCase se l'unica cosa che mi serve è un MockEngine.

[fek]

Giusto, togli le dipendenze dai TestCase che non servono e aggiungi i Refactor This che reputi opportuni.

[Baol]

Uhmmm, ieri sera il forum è andato in palla e non mi ha più modificato il post...

Dunque: ho committato il lavoro sulla CrushByChestAction ma si può fare ancora qualcosa: pensavo di aggiungere un metodo getAdjacentDroppables ai vari Droppables (implementato magari in AbstractDroppable e in BigGem) che restituisca tutte le gemme adiacenti (ovvero con un lato in comune) di qualsiasi colore (pensavo con un flag che indichi se devono o meno essere dello stesso colore, oppure creare un altro metodo apposito). Questo semplificherebbe notevolmente la Action e ci metterebbe più vicini alla sua eliminazione.
Posso procedere in quella direzione?

[fek]

Originariamente inviato da Baol

Uhmmm, ieri sera il forum è andato in palla e non mi ha più modificato il post...

Dunque: ho committato il lavoro sulla CrushByChestAction ma si può fare ancora qualcosa: pensavo di aggiungere un metodo getAdjacentDroppables ai vari Droppables (implementato magari in AbstractDroppable e in BigGem) che restituisca tutte le gemme adiacenti (ovvero con un lato in comune) di qualsiasi colore (pensavo con un flag che indichi se devono o meno essere dello stesso colore, oppure creare un altro metodo apposito). Questo semplificherebbe notevolmente la Action e ci metterebbe più vicini alla sua eliminazione.
Posso procedere in quella direzione?

Flag?! Flag che modifica il comportamento di un metodo?!?!?! FLAG?!?!?!

[Baol]

Chiedo scusa, volevo dire due metodi separati

[71104]

ho una domanda circa la regola 1 commit ==> 1 refactor this che se ne va: è possibile committare aggiungendo dei refactor this nel codice ma senza risolverne nessuno? oppure per committare qualunque cosa (anche l'aggiunta di nuovi refactor this) bisogna per forza risolverne almeno uno?

[fek]

Originariamente inviato da 71104

ho una domanda circa la regola 1 commit ==> 1 refactor this che se ne va: è possibile committare aggiungendo dei refactor this nel codice ma senza risolverne nessuno? oppure per committare qualunque cosa (anche l'aggiunta di nuovi refactor this) bisogna per forza risolverne almeno uno?

La seconda che hai detto.

[71104]

ok.

ora sto lavorando sul refactor this in it.diamonds.grid.action.CrushByChestAction:11

[71104]

ot: ho notato solo oggi l'immagine di ken shiro in prima pagina
credo che abbiamo trovato un'ispirazione per il codename della prossima release
(il codename precedente era YAGNI )