Programma "Refactor This!" [Da leggere]

[Bonfo]

Originariamente inviato da Ufo13

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

L'idea e` usare Cell ovunque.

Azz... ho fatto un macello

Ok... se non l'avete ancora fatto metto tutto a Cell, anche se secondo me ci complica la vita.
Ma se lo dice il God in persona

EDIT:
Io sto ancora cercando di semplificare il GridTestCase, la sto prendeno un po' alla lontana mettendoa posto TestGrid, ma cosi' dovremmo avere molto piu' controllo sui test in Grid, rendendoci piu' semplice il GridTestCase e di conseguenza l'80% dei test fatti fino ad ora.
Sono esentato dal Refactor This oppure prima li faccio fuori tutti e poi posso andare avanti ?!

[AnonimoVeneziano]

Era :

it.diamonds.grid.action.CrushByChestAction.java Linea 62

    private void getAdjacentCrushableGems(Droppable crushSourceDroppable,
        DroppableList adiacentCrushableGems)
    {
        // TODO: REFACTOR THIS

        Region sourceCell = crushSourceDroppable.getRegion();

        if(sourceCell.getLeftColumn() >= 1)
        {
            getLeftOrRightAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_LEFT);
        }

        if(sourceCell.getRightColumn() < getGrid().getNumberOfColumns() - 1)
        {
            getLeftOrRightAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_RIGHT);
        }

        if(sourceCell.getTopRow() >= 1)
        {
            getUpOrDownAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_UP);
        }

        if(sourceCell.getBottomRow() < getGrid().getNumberOfRows() - 1)
        {
            getUpOrDownAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_DOWN);
        }
    }

E'

    private void getAdjacentCrushableGems(Droppable crushSourceDroppable,
        DroppableList adiacentCrushableGems)
    {
            getLeftOrRightAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_LEFT);
            
            getLeftOrRightAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_RIGHT);

            getUpOrDownAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_UP);

            getUpOrDownAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems, GO_DOWN);
    }

E ho spostato i controlli nei metodi getUpOrDownAdjacent.... getLeftOrRight...


ad esempio in getUpOrDown... è stato aggiunto:

        if(row + direction.deltaY() < 0 || row + direction.deltaY() > getGrid().getNumberOfRows() - 1)
        {
            return;
        }

Analogo è stato fatto nella getLeftOrRight.... ma con le colonne al posto delle righe

Ciao

[fek]

Originariamente inviato da Bonfo

Sono esentato dal Refactor This oppure prima li faccio fuori tutti e poi posso andare avanti ?!

La seconda che hai detto

[fek]

Originariamente inviato da AnonimoVeneziano

E ho spostato i controlli nei metodi getUpOrDownAdjacent.... getLeftOrRight...


ad esempio in getUpOrDown... è stato aggiunto:

        if(row + direction.deltaY() < 0 || row + direction.deltaY() > getGrid().getNumberOfRows() - 1)
        {
            return;
        }

Analogo è stato fatto nella getLeftOrRight.... ma con le colonne al posto delle righe

Ciao

Bene, mi piace molto, mi rendi ora piu' chiaro quell'if? E' troppo lunghetto.

[AnonimoVeneziano]

Originariamente inviato da fek

Bene, mi piace molto, mi rendi ora piu' chiaro quell'if? E' troppo lunghetto.

Mmm, mi è venuta in mente questa soluzione :

La funzione da rifattorizzare diventa :

    private void getAdjacentCrushableGems(Droppable crushSourceDroppable,
        DroppableList adiacentCrushableGems)
    {
            getLeftAndRightAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems);

            getUpAndDownAdjacentCrushableGems(crushSourceDroppable,
                adiacentCrushableGems);
    }

e le due funzioni getUpAndDown... e getLeftAndRight... diventano :

    private void getUpAndDownAdjacentCrushableGems(Droppable source,
        DroppableList adiacentCrushableGems)
    {
        int row = source.getRegion().getTopRow();
        Direction direction = GO_UP;
       
        if(row + direction.deltaY() >= 0)
        {
           for(int column = source.getRegion().getLeftColumn(); column <= source.getRegion().getRightColumn(); column++)
           {
                Droppable toTest = getGrid().getDroppableAt(row + direction.deltaY(), column);
                tryToAddGemToCrushableGems(toTest, source, adiacentCrushableGems);
           }
        }

        row = source.getRegion().getBottomRow();
        direction = GO_DOWN;

        if(row + direction.deltaY() <= getGrid().getNumberOfRows() - 1)
        {
           for(int column = source.getRegion().getLeftColumn(); column <= source.getRegion().getRightColumn(); column++)
           {
                Droppable toTest = getGrid().getDroppableAt(row + direction.deltaY(), column);
                tryToAddGemToCrushableGems(toTest, source, adiacentCrushableGems);
           }
        }
    }


    private void getLeftAndRightAdjacentCrushableGems(Droppable source,
        DroppableList adiacentCrushableGems)
    {
        int column = source.getRegion().getLeftColumn();
        Direction direction = GO_LEFT;
        
        if(column + direction.deltaX() >= 0)
        {
            for(int row = source.getRegion().getTopRow(); row <= source.getRegion().getBottomRow(); row++)
            {
                Droppable toTest = getGrid().getDroppableAt(row, column + direction.deltaX());
                tryToAddGemToCrushableGems(toTest, source, adiacentCrushableGems);
            }
        }
        
        column = source.getRegion().getRightColumn();
        direction = GO_RIGHT;
        
        if(column + direction.deltaX() <= getGrid().getNumberOfColumns() - 1)
        {
            for(int row = source.getRegion().getTopRow(); row <= source.getRegion().getBottomRow(); row++)
            {
                Droppable toTest = getGrid().getDroppableAt(row, column + direction.deltaX());
                tryToAddGemToCrushableGems(toTest, source, adiacentCrushableGems);
            }
        }
    }

Meglio o peggio?

Il numero di righe delle funzioni UpAndDown e LeftAndRight rimane praticamente invariato (2 righe in più)

Ciao

[fek]

Preferivo l'altra. Le due funzione getUpAndDown... e l'altra sono troppo complesse. Non si capisce che cosa stiano facendo solo leggendo il codice.
Lo dice anche il nome secondo stesso secondo me. Io dividerei ognuna in due. Puoi lavorarci un po'?

[AnonimoVeneziano]

Originariamente inviato da fek

Preferivo l'altra. Le due funzione getUpAndDown... e l'altra sono troppo complesse. Non si capisce che cosa stiano facendo solo leggendo il codice.
Lo dice anche il nome secondo stesso secondo me. Io dividerei ognuna in due. Puoi lavorarci un po'?

Quindi ritorno alla vecchia soluzione con l'if semplificato?

EDIT

Così va bene l'if?

CAVOLATA :fagiano:

[fek]

Originariamente inviato da AnonimoVeneziano

Quindi ritorno alla vecchia soluzione con l'if semplificato?

Vedi tu

[AnonimoVeneziano]

Originariamente inviato da fek

Vedi tu

Ok, torniamo al vecchio

Così ? :

        row += direction.deltaY();
        
        if(row < 0 || row > getGrid().getNumberOfRows() - 1)
        {
            return;
        }

Poi anche sotto nel "for" ovviamente verrà usato "row" anzichè "row + direction.deltaY()" (stessa cosa per l'altra funzione)

Ciao

[Ufo13]

Rifattorizzo questo:

public void testDrawTwoFrames()
    {
        int frameSize = 32;
        
        AnimatedSprite animatedSprite = createAnimatedSprite(3, 100, frameSize);
        
        timer.advance(ANIMATION_DELAY);

        animatedSprite.updateAnimation(timer.getTime());
        animatedSprite.getSprite().draw(engine);

        assertEquals("bad texture drawn", 0,
            ((MockEngine)engine).getImageRect().getLeft());
        assertEquals("bad texture drawn", frameSize,
            ((MockEngine)engine).getImageRect().getTop());

        timer.advance(100);
        animatedSprite.updateAnimation(timer.getTime());
        animatedSprite.getSprite().draw(engine);

        assertEquals("bad texture drawn", 0,
            ((MockEngine)engine).getImageRect().getLeft());
        assertEquals("bad texture drawn", frameSize * 2,
            ((MockEngine)engine).getImageRect().getTop());
    }

Spezzato in 3 test. Ho aggiunto il test per il frame iniziale.

    public void testRenderingOfFirstFrame()
    {
        AnimatedSprite animatedSprite = createAnimatedSprite(2, 100, FRAME_SIZE);
        
        animatedSprite.updateAnimation(timer.getTime());
        animatedSprite.getSprite().draw(engine);
        
        Rectangle expectedImageRectangle = new Rectangle(0, 0, FRAME_SIZE, FRAME_SIZE);
        assertEquals(expectedImageRectangle, engine.getImageRect());
    }
    
    public void testRenderingOfSecondFrame()
    {
        AnimatedSprite animatedSprite = createAnimatedSprite(2, 100, FRAME_SIZE);
        
        timer.advance(ANIMATION_DELAY);
        animatedSprite.updateAnimation(timer.getTime());
        animatedSprite.getSprite().draw(engine);
        
        Rectangle expectedImageRectangle = new Rectangle(0, FRAME_SIZE, FRAME_SIZE, FRAME_SIZE);
        assertEquals(expectedImageRectangle, engine.getImageRect());
    }
    
    public void testRenderingOfThirdFrame()
    {
        final int animationUpdateRate = 100;
        AnimatedSprite animatedSprite = createAnimatedSprite(3, animationUpdateRate, FRAME_SIZE);
        
        timer.advance(ANIMATION_DELAY);
        timer.advance(animationUpdateRate);
        
        animatedSprite.updateAnimation(timer.getTime());
        animatedSprite.getSprite().draw(engine);
        
        Rectangle expectedImageRectangle = new Rectangle(0, FRAME_SIZE * 2, FRAME_SIZE, FRAME_SIZE);
        assertEquals(expectedImageRectangle, engine.getImageRect());
    }

Si potrebbe fare ancora qualcosina ma mi accontento, e` abbastanza chiaro penso :o

[Ufo13]

Prima:

    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;
    }

Dopo:

    private Droppable getGemToDelete(Droppable falshingGem)
    {        
        Region region = falshingGem.getRegion();
        Cell cell = new Cell(region.getTopRow(), region.getLeftColumn());
        
        Direction directions[] = {Direction.GO_DOWN, Direction.GO_LEFT, Direction.GO_RIGHT, Direction.GO_UP};

        for (Direction direction : directions)
        {
            Droppable gemToDelete = searchGemToDelete(cell, direction);
            
            if (gemToDelete != null)
            {
                return gemToDelete;
            }
        }
        
        return null;
    }


    private Droppable searchGemToDelete(Cell cell, Direction direction)
    {
        final int rowToSearch = cell.getRow() + direction.deltaY();
        final int columnToSearch = cell.getColumn() + direction.deltaX();
        
        final boolean isValidCell = getGrid().isValidCell(rowToSearch, columnToSearch);
        if (!isValidCell)
        {
            return null;
        }
        
        return getGem(new Cell(rowToSearch, columnToSearch));
    }

[Bonfo]

[Baol]

Originariamente inviato da Ufo13

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

Per evitare cose del genere o sovrapposizioni di lavoro non si potrebbe creare un topic "Io sto lavorando su..." in cui inserire cosa si sta facendo? Ad esempio su quale Refactor This ci si sta impegnando? (questo eventualmente si può fare anche qua, postando prima di modificarlo ed editando a lavoro completato)

[Bonfo]

Io sto rimettendo tutti i cell che avevo tolto!

Mi ci vorra' un po', anche perche' nel mentre lavoro :P

[fek]

Originariamente inviato da Baol

Per evitare cose del genere o sovrapposizioni di lavoro non si potrebbe creare un topic "Io sto lavorando su..." in cui inserire cosa si sta facendo? Ad esempio su quale Refactor This ci si sta impegnando? (questo eventualmente si può fare anche qua, postando prima di modificarlo ed editando a lavoro completato)

Si' buona idea. Scrivete qui il Refactor This sul quale state lavorando e poi editatelo con la soluzione.

Fede, refactoring stilosissimo

[Baol]

Lavorando in CrushByFlashAction.java su getAllGemsSameOf.
Prima:

    //TODO: REFACTOR THIS
    private DroppableList getAllGemsSameOf(Droppable gemToDelete)
    {
        DroppableList gemList = new DroppableList();
        Droppable gem;
        
        for(int row = getGrid().getNumberOfRows() - 1; row >= 0; row--)
        {
            for(int column = 0; column < getGrid().getNumberOfColumns(); column++)
            {
                gem = getGrid().getDroppableAt(row, column);
                if(gem == null)
                {
                    continue;
                }

                if(gem.getGridObject().getColor() == gemToDelete.getGridObject().getColor())
                {
                    if(!gemList.contains(gem))
                    {
                        gemList.add(gem);
                    }
                }
            }
        }
        return gemList;
    }

Dopo...

    public void apply(DroppableList gridElements)
    {
        super.apply(gridElements);
        applyToEachDroppable();
    }
    ...

    private DroppableList getAllGemsSameOf(Droppable gemToDelete)
    {
        DroppableList gridElements = getGridElements();
        DroppableList gemSameList = new DroppableList();

        for(Droppable droppable : gridElements)
        {
            if(droppable.getGridObject().getColor() == gemToDelete.getGridObject().getColor())
            {
                gemSameList.add(droppable);
            }
        }

        return gemSameList;
    }

[Bonfo]

Rimettendo tutti i new Cell() dentro codice e test, mi sono reso conto chesi usanbo moltissimo la coppia 0, 4 e 1, 4, ovvero qulle delle gemsPair.

Che ne di te di un bel campo statico in GemsPair??

[AnonimoVeneziano]

Originariamente inviato da Baol

Lavorando in CrushByFlashAction.java su getAllGemsSameOf.
Prima:

    //TODO: REFACTOR THIS
    private DroppableList getAllGemsSameOf(Droppable gemToDelete)
    {
        DroppableList gemList = new DroppableList();
        Droppable gem;
        
        for(int row = getGrid().getNumberOfRows() - 1; row >= 0; row--)
        {
            for(int column = 0; column < getGrid().getNumberOfColumns(); column++)
            {
                gem = getGrid().getDroppableAt(row, column);
                if(gem == null)
                {
                    continue;
                }

                if(gem.getGridObject().getColor() == gemToDelete.getGridObject().getColor())
                {
                    if(!gemList.contains(gem))
                    {
                        gemList.add(gem);
                    }
                }
            }
        }
        return gemList;
    }

...

Mmm, qua ci vorrebbe un modo facile per ottenere tutte le gemme dello stesso tipo nella griglia ...

[Bonfo]

Scusate, ma perche' invece di andare per riga e per colonna non andiamo con l'iteratore sull'elenco di droppabales??

            for(Droppable droppabel : grid.getDroppableList()) {
            }

e se poi abbiamo problemi di iterazione doppia basta fare:

            for(Iterator iterator = grid.getDroppableList().iterator(); iterator.hasNext();)
            {
                Droppable droppable = (Droppable)iterator.next();
                
            }

[Bonfo]

Piccolo problemi che ritorna prepotente...

Quando creo una cella con valori negativi mi becco una exception.
Che faccio: aggiungo controlli ovunque??

in realta' basta che isValidCell continui aragionare a row e column. Che si fa?