Programma "Refactor This!" [Da leggere]

[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!"



Ho seminato nel codice questo commento:

// TODO: REFACTOR THIS

Il commento indica un metodo che va rifattorizzato semplicemente perche' non si capisce che cosa voglia fare, e' oscuro, o troppo lungo, o non utile. Insomma, va rifattorizzato e ripulito in un metodo piu' chiaro.

Le regole di commit cambiano dunque cosi':

- ad ogni commit dev'essere associata la rimozione di un TODO "Refactor This"
- nella descrizione del commit indicare il file e la riga del TODO eliminato
- scrivere in questo topic il codice prima e dopo il refactoring

Il non seguire questa regola portera' al revert del commit. E' una soluzione temporanea un po' draconiana, ma e' chiaro che serve solo per riportare velocemente la code base ad uno stato piu' usabile, di modo da facilitare i successivi task e renderli piu' divertenti per tutti. Al momento lavorare sulla code base e' un lavoraccio ed e' molto complesso.

Se sono necessarie eventuali deroghe alla regola, magari per un commit veloce, o una serie di commit (per i quali basta un solo TODO eliminato), contattatemi pure in MSN. Senza deroga vale la regola: "Un commit, un TODO eliminato".

Periodicamente faro' un giro della code base per indicare i metodi da rifattorizzare. Sentitevi liberi di aggiungere il TODO ad un metodo che reputate troppo complesso o non chiaro.

[Bonfo]

Regole generali per il refactoring:
- Quando un metodo da privato diventa pubblico bisogna aggiunger il test!!


P.S.: se volete che aggiunga qualcos'altro, ditemelo

[71104]

bella come idea
mi metto a caccia.

[^TiGeRShArK^]

oh porca

meno male che per ora non ho tempo di committare (dopo aver finito di lavorare alle 20 e 30 finalmente mi sto prendendo un pò di riposo )

P.S: il mio amico giustamente aggiunge: "non direi proprio"... dato che fino ad ora ho bestemmiato per settargli l'ADSL cercando di recuperare la password del router che aveva perso

[71104]

it.diamonds.grid.action.CrushByFlashAction.java:53

codice attuale:

    private Droppable getGemToDelete(Droppable falshingGem)
    {
        // TODO: REFACTOR THIS

        Droppable gemToDelete = null;

        Region cell = falshingGem.getRegion();

        if(cell.getTopRow() < getGrid().getNumberOfRows() - 1)
        {
            gemToDelete = getGem(cell.getTopRow() + 1, cell.getLeftColumn());
        }

        if(cell.getLeftColumn() >= 1 && gemToDelete == null)
        {
            gemToDelete = getGem(cell.getTopRow(), cell.getLeftColumn() - 1);
        }

        if(cell.getLeftColumn() < getGrid().getNumberOfColumns() - 1
            && gemToDelete == null)
        {
            gemToDelete = getGem(cell.getTopRow(), cell.getLeftColumn() + 1);
        }

        if(cell.getTopRow() > 1 && gemToDelete == null)
        {
            gemToDelete = getGem(cell.getTopRow() - 1, cell.getLeftColumn());
        }

        return gemToDelete;
    }

questo è quello che ottengo ad una prima revisione, ma non ho concluso granché: è ancora troppo pieno di if per i miei gusti -.-

    private Droppable getGemToDelete(Droppable falshingGem)
    {
        // TODO: REFACTOR THIS

        Region region = falshingGem.getRegion();
        Cell cell = new Cell(region.getTopRow(), region.getLeftColumn());

        Droppable gemToDelete = searchGemToDelete(cell);

        if(gemToDelete == null)
        {
            gemToDelete = searchLeft(cell);

            if(gemToDelete == null)
            {
                gemToDelete = searchRight(cell);

                if(gemToDelete == null)
                {
                    gemToDelete = searchUp(cell);
                }
            }
        }
        return gemToDelete;
    }


    private Droppable searchGemToDelete(Cell cell)
    {
        if((cell.getRow() < getGrid().getNumberOfRows() - 1))
        {
            return getGem(cell.getLower());
        }
        return null;
    }


    private Droppable searchLeft(Cell cell)
    {
        if(cell.getColumn() >= 1)
        {
            return getGem(cell.getLeft());
        }
        return null;
    }


    private Droppable searchRight(Cell cell)
    {
        if(cell.getColumn() < getGrid().getNumberOfColumns() - 1)
        {
            return getGem(cell.getRight());
        }
        return null;
    }


    private Droppable searchUp(Cell cell)
    {
        if(cell.getRow() > 1)
        {
            return getGem(cell.getUpper());
        }
        return null;
    }


    private Droppable getGem(Cell cell)
    {
        Droppable gem = getGrid().getDroppableAt(cell);
        if(gem == null)
        {
            return null;
        }

        if(gem.getFallingObject() != null && gem.getFallingObject().isFalling())
        {
            return null;
        }

        if(gem.getGridObject().getType().isStone())
        {
            return null;
        }

        return gem;
    }

[71104]

mi ci sono scervellato, meglio di così non riesco a fare: non riesco ad eliminare quegli if (PER ORA). committo così com'è.

[71104]

minchia, guardate che roba

it.diamonds.tests.droppable.gems.TestBigGemInGrid:457

    public void testBigGemMoveDownABit()
    {
        // TODO: REFACTOR THIS

        Droppable gem = createGem(EMERALD);
        grid.insertDroppable(gem, 12, 3);
        gem.getFallingObject().drop();
        grid.updateDroppable(gem);

        Droppable gem1 = createGem(EMERALD);
        grid.insertDroppable(gem1, 12, 4);
        gem1.getFallingObject().drop();
        grid.updateDroppable(gem1);

        Droppable gem2 = createGem(EMERALD);
        grid.insertDroppable(gem2, 11, 3);
        gem2.getFallingObject().drop();
        grid.updateDroppable(gem2);

        Droppable gem3 = createGem(EMERALD);
        grid.insertDroppable(gem3, 11, 4);
        gem3.getFallingObject().drop();
        grid.updateDroppable(gem3);

        float gemY = gem2.getAnimatedObject().getSprite().getPosition().getY();

        grid.updateBigGems();
        Droppable bigGem = grid.getDroppableAt(new Cell(12, 3));

        bigGem.getMovingDownObject().moveDown(grid);

        assertEquals("top must be increased", gemY
            + ((float)grid.getActualGravity() / 2),
            bigGem.getAnimatedObject().getSprite().getPosition().getY(), 0.001f);
    }

    public void testBigGemMoveDownABit()
    {
        insert2x2BlockOfGems(EMERALD, 11, 3);
        Droppable topGem = grid.getDroppableAt(new Cell(11, 3));

        grid.updateBigGems();
        Droppable bigGem = grid.getDroppableAt(new Cell(12, 3));

        bigGem.getMovingDownObject().moveDown(grid);

        float top = topGem.getAnimatedObject().getSprite().getPosition().getY();

        assertEquals("top must be increased", top
            + ((float)grid.getActualGravity() / 2),
            bigGem.getAnimatedObject().getSprite().getPosition().getY(), 0.001f);
    }

[Bonfo]

71104, sei stato tu a mettere static le rows e le columns in Grid??

A me va benissimo, ma ciò vuol dire che tutte le grid hanno lo stesso numero di colonne e righe, e anche questo mi va benissimo, ma anche che sono inizializzate a 0 se non specificato diversamente:

    private static int rows;

    private static int columns;

    ...

    public Grid(Environment environment, Point origin)
    {
        ....

        rows = environment.getConfig().getInteger("rows");
        columns = environment.getConfig().getInteger("columns");

        ....
    }

Tu non so se lo sai, ma hai appena aggiunto un bug alla code-base.
Se non viene mai costruita una grid, rows e columns sono entrambi a 0.

Ma poi mi speighi il vantaggio di averli statici?!?

Io no sono lo spezzatore di dita ufficiale, ma poi finisce che chiedo una delega .

[Bonfo]

Mi è venuta in mente una cosa carina...
Vediamo quanti test scoppiano se nel config cambio il valore di columns o di rows ?!?

No provo perchè potrei svenire.

[Bonfo]

Bisogna prendere una decisione per Grid.

Tutti i metodi, secondo me, devono essere omogenei; di conseguenza:
- tutti ricevono come parametro Cell;
- tutti ricevono come parametro row e column;

Piccola grande differenza: Cell lancia una eccezione se viene creata con parametri negativi.

Per isValidCell vuol dire che non c'è bisogno di controllare i valori negativi, ma che può lanciare eccezzionida un momento all'altro.

Io voto per la coppia row e column. ( tanto i metodi isValidColumn e isValidCell ci sono già).

Aspetto decisioni.

EDIT2: Ehm... già che c'ero le ho cambiate tutte, i posto in cui si usavano vermanete le celle erano 2...
in tutto il resto della code base si faceva sempre new. Secondo me ora le cose sono più pulite


EDTI: c'è una sovrapposizione tra il metodo inserDroppable di Grid e la classe InsertDroppableAction. Che si fa?
Io ucciderei la action.

[Ufo13]

Minchia Bonfo avevamo appena iniziato a cambiare tutto in Cell e hai ri-revertato i cambiamenti fatti

L'idea e` usare Cell ovunque.

[fek]

Originariamente inviato da Bonfo

Bisogna prendere una decisione per Grid.

Tutti i metodi, secondo me, devono essere omogenei; di conseguenza:
- tutti ricevono come parametro Cell;
- tutti ricevono come parametro row e column;

Si lavora ovunque solo in termini di Cell e non di column/row.

EDIT2: Ehm... già che c'ero le ho cambiate tutte, i posto in cui si usavano vermanete le celle erano 2...
in tutto il resto della code base si faceva sempre new. Secondo me ora le cose sono più pulite

Puoi fare il revert per favore? Vogliamo cambiare tutto a Cell.
Quale TODO hai rifattorizzato per questo commit? La regola vale per tutti.

[fek]

Voglio aggiungere un'altra cosa: non si entra in produzione fino a che non spariscono i Refactor This dalla code base. Questo ci da' anche una buona metrica per capire a che punto stiamo.

Abbiamo 16 Refactor This al momento.

[fek]

Per rilassare un attimo la regola, che effettivamente e' molto draconiana, non serve che risolviate del tutto un TODO per fare il commit, basta che ci abbiate lavorato in maniera consistente e abbiate migliorato quel pezzo di codice. Poi sta a voi decidere se il codice e' in buono stato e il TODO si puo' eliminare.
Ricordate di indicare nel commit la riga di codice dove era presente il TODO.

[Ufo13]

Quindi se uno deve ancora rifattorizzare un pezzo di codice (sto ancora lavorando ad animation) prima fa commit dei todo da fare e poi comincia ad eliminarli? Nel mio caso bisogna ancora spostare un po' di test da TestGemAnimation a TestAnimatedSprite piu` semplificarli piu` testare roba non testata...

[fek]

Si', e' chiaro che se stai facendo una serie di commit come me in questo momento, basta un solo TODO.

[71104]

Originariamente inviato da Bonfo

71104, sei stato tu a mettere static le rows e le columns in Grid??

si, l'avevo fatto mentre cercavo soluzioni per il primo TODO anche se poi quella soluzione l'avevo revertata (dimenticandomi di revertare quel fatto dei campi static). per quanto riguarda il bug, l'importante è che quei due metodi non vengano mai chiamati quando nessuna griglia è stata ancora creata, semplice

[AnonimoVeneziano]

Prima:

    protected void applyOn(Droppable gem)
    {
        Region cell = gem.getRegion();

        int row = cell.getTopRow();
        int column = cell.getLeftColumn();

        if(isGemNotValidForBigGem(gem))
        {
            return;
        }

        if(isGemNeighbourValidForBigGem(gem, -1, 0)
            && isGemNeighbourValidForBigGem(gem, 0, 1)
            && isGemNeighbourValidForBigGem(gem, -1, 1))
        {
            // TODO: Fran - REFACTOR THIS
            BigGem bigGem = new BigGem(
                row - 1,
                column,
                gem.getGridObject().getColor(),
                engine,
                getGrid().getDroppableAt(row - 1, column).getAnimatedSprite().getSprite().getPosition());

            bigGem.addGem(gem);
            bigGem.addGem(getGrid().getDroppableAt(row - 1, column));
            bigGem.addGem(getGrid().getDroppableAt(row, column + 1));
            bigGem.addGem(getGrid().getDroppableAt(row - 1, column + 1));

            for(Droppable droppable : bigGem.getIncludedGems())
            {
                getGrid().removeDroppable(droppable);
            }

            droppables.add(bigGem);
        }
    }

Ora:

    protected void applyOn(Droppable gem)
    {
     
        if(isGemNotValidForBigGem(gem))
        {
            return;
        }

        if(isGemNeighbourValidForBigGem(gem, -1, 0)
            && isGemNeighbourValidForBigGem(gem, 0, 1)
            && isGemNeighbourValidForBigGem(gem, -1, 1))
        {
            BigGem bigGem = createBigGemWithGemsNear(gem);
            
            removeGemsUsedForBigGem(bigGem);

            droppables.add(bigGem);
        }
    }

    private BigGem createBigGemWithGemsNear(Droppable gem) 
    {
        int row = gem.getRegion().getTopRow();
        int column = gem.getRegion().getLeftColumn();
        DroppableColor bigGemColor = gem.getGridObject().getColor();
        Point spritePosition = getGrid().getDroppableAt(row - 1, column).getAnimatedSprite().getSprite().getPosition();
        
        BigGem bigGem = new BigGem(row - 1, column, bigGemColor, engine, spritePosition);
        
        bigGem.addGem(gem);
        bigGem.addGem(getGrid().getDroppableAt(row - 1, column));
        bigGem.addGem(getGrid().getDroppableAt(row, column + 1));
        bigGem.addGem(getGrid().getDroppableAt(row - 1, column + 1));
        
        return bigGem;
    }
    
    private void removeGemsUsedForBigGem(BigGem bigGem) 
    {
        for(Droppable droppable : bigGem.getIncludedGems())
        {
            getGrid().removeDroppable(droppable);
        }       
    }

Okkei?

(Non sono molto esperto di refactoring )

[71104]

Originariamente inviato da AnonimoVeneziano

Okkei?

(Non sono molto esperto di refactoring )

basta che la build sia ancora verde; tanto non dovevi aggiungere nessun test perché i metodi che hai aggiunto sono tutti privati se non erro. se è tutto verde committa.

[AnonimoVeneziano]

Originariamente inviato da 71104

basta che la build sia ancora verde; tanto non dovevi aggiungere nessun test perché i metodi che hai aggiunto sono tutti privati se non erro. se è tutto verde committa.

Oookkei