Programma "Refactor This!" [Da leggere]

[AnonimoVeneziano]

Originariamente inviato da 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();
                
            }

Quel metodo in Grid non c'è , bisognerà aggiungerlo

Comunque non so quanto riduce la complessità . In pratica invece di fare il ciclo su due livelli lo fai su uno solo, però hai il vantaggio di togliere dall'iterazione di tutti gli elementi "null" della griglia ...

[Bonfo]

E in piu' non ti devi tenere una lista di supporto per sapere se quell'oggetto l'hai gia' visitato o no.

Mi ricordo che subito dopo la FirstPlayable abbiamo trasformato grid da array di celle a lista di droppable proprio per poter itereare solo sugli oggetti dentro la grid.

Il problema che avevo incontrato era l'iterazione mutlipla perche' usavo il foreach (":") , ma con l'iterator il problema si risolve ( o almeno cosi' dovrebbe essere)

[Baol]

Era esattamente quello che stavo per proporre io, infatti per il refactoring di prima ho risolto così (alle Actions viene già passata la droppableList).
Tuttavia provando a modificarlo in AbstractGridIteration ottengo un bel java.util.ConcurrentModificationException in un test fra l'altro che c'entra poco, ma che richiama la createNewBigGemsAction...
Sto provando a modificare ma sono un attimo in difficoltà.
Intanto committo il refactoring del REFACTOR THIS.

EDIT: cosa intendi per iterazione multipla?

[Bonfo]

Originariamente inviato da Baol

EDIT: cosa intendi per iterazione multipla?

java.util.ConcurrentModificationException

Ovvero stai cercando di modifcare un lista sulla quale qualcun'altro sta iterando.
Quello che fa scoppiare tutto è (ad esempio)

for(Object obj : objects) {
    for(Object obj1 : objects) {
         objects.remove(obj1);
    }

Però ho detto una cavolata
Non è sufficente farsi dare due iteratori diverisi, il problema si pone lo stesso, ovvero e un po' più complicato da risolvere

[Bonfo]

Prima

public void testPositions()
    {
        // TODO: REFACTOR THIS
        // Test positions of what? clarify this test and simplify it
        
        int row1 = 4;
        int column1 = 2;

        Point spritePosition1 = gem.getAnimatedSprite().getPosition();

        grid.insertDroppable(gem, row1, column1);
        
        assertEquals(gridPosition.getX() + Cell.SIZE_IN_PIXELS * column1, spritePosition1.getX());
        assertEquals(gridPosition.getY() + Cell.SIZE_IN_PIXELS * row1, spritePosition1.getY());

        int row2 = 3;
        int column2 = 5;
        
        Droppable anotherGem = createGem(DroppableColor.DIAMOND);

        Point spritePosition2 = anotherGem.getAnimatedSprite().getPosition();

        grid.insertDroppable(anotherGem, row2, column2);
        
        assertEquals(gridPosition.getX() + Cell.SIZE_IN_PIXELS * column2, spritePosition2.getX());
        assertEquals(gridPosition.getY() + Cell.SIZE_IN_PIXELS * row2, spritePosition2.getY());
    }

Dopo

    public void testGemSpritePositions()
    {
        grid.insertDroppable(gem, 4, 2);
        checkSpritePosition(gem.getAnimatedSprite().getPosition(), 4, 2);

        Droppable anotherGem = createGem(DroppableColor.DIAMOND);
        grid.insertDroppable(anotherGem, 3, 5);
        checkSpritePosition(anotherGem.getAnimatedSprite().getPosition(), 3, 5);
    }


    private void checkSpritePosition(Point spritePosition, int row, int column)
    {
        assertEquals(gridPosition.getX() + Cell.SIZE_IN_PIXELS * column,
            spritePosition.getX());
        assertEquals(gridPosition.getY() + Cell.SIZE_IN_PIXELS * row,
            spritePosition.getY());
    }

[Bonfo]

Prima

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

        MockEngine engine = MockEngine.create(800, 600);

        Droppable gem = createGem(DroppableColor.DIAMOND);

        final Sprite sprite = gem.getAnimatedSprite().getSprite();

        sprite.draw(engine);

        assertEquals("gem must be drawn with unbrightened texture(bad left)",
            engine.getImageRect().getLeft(), 0);

        assertEquals("gem must be drawn with unbrightened texture(bad right)",
            engine.getImageRect().getRight(), 31);
    }


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

        MockEngine engine = MockEngine.create(800, 600);

        Droppable gem = createGem(DroppableColor.DIAMOND);
        gem.getAnimatedSprite().getSprite().draw(engine);
        assertEquals(
            "Height of the texture engine differente of height of gem(init)",
            engine.getImageRect().getHeight(),
            (int)gem.getAnimatedSprite().getSprite().getTextureArea().getHeight());
        assertEquals(
            "Width of the texture engine differente of width of gem(init)",
            engine.getImageRect().getWidth(),
            (int)gem.getAnimatedSprite().getSprite().getTextureArea().getWidth());

        gem.getAnimatedSprite().getSprite().draw(engine);
        assertEquals(
            "Height of the texture engine differente of height of gem(bright false)",
            engine.getImageRect().getHeight(),
            (int)gem.getAnimatedSprite().getSprite().getTextureArea().getHeight());
        assertEquals(
            "Width of the texture engine differente of width of gem(bright false)",
            engine.getImageRect().getWidth(),
            (int)gem.getAnimatedSprite().getSprite().getTextureArea().getWidth());
    }

Dopo

    private Gem gem;

    private MockEngine engine;


    @Override
    public void setUp()
    {
        super.setUp();
        engine = (MockEngine)environment.getEngine();
        gem = createGem(DroppableColor.DIAMOND);
    }
    
    public void testGemIsDrawnCorrectly()
    {
        gem.getAnimatedSprite().getSprite().draw(engine);
        
        assertEquals("Wrong left texture position", engine.getImageRect().getLeft(), 0);
        assertEquals("Wrong right texture position", engine.getImageRect().getRight(), 31);
    }


    public void testGemViewSize()
    {
        Sprite sprite = gem.getAnimatedSprite().getSprite();
        sprite.draw(engine);

        assertEquals("Wrong textureArea height", engine.getImageRect().getHeight(), sprite.getTextureArea().getHeight());
        assertEquals("Wrong textureArea Width", engine.getImageRect().getWidth(), sprite.getTextureArea().getWidth());
    }

[Bonfo]

Questo è più difficile....
Prima

    //TODO: REFACTOR THIS
    public void testUpdateWithThreeFrames()
    {
        AnimatedSprite animatedSprite = createAnimatedSprite(3, 100);
        
        timer.advance(ANIMATION_DELAY);

        animatedSprite.updateAnimation(timer.getTime());

        timer.advance(1);
        animatedSprite.updateAnimation(timer.getTime());

        assertEquals("current frame must be 1", 1,
            animatedSprite.getCurrentFrame());

        timer.advance(99);
        animatedSprite.updateAnimation(timer.getTime());

        assertEquals("current frame must be 2", 2,
            animatedSprite.getCurrentFrame());

    }

Dopo:

    public void testUpdateWithThreeFrames()
    {
        AnimatedSprite animatedSprite = createAnimatedSprite(3, 100);
        
        timer.advance(ANIMATION_DELAY + 1);
        animatedSprite.updateAnimation(timer.getTime());

        assertEquals("current frame must be 1", 1,
            animatedSprite.getCurrentFrame());

        timer.advance(99);
        animatedSprite.updateAnimation(timer.getTime());

        assertEquals("current frame must be 2", 2,
            animatedSprite.getCurrentFrame());

    }

Di più non mi è venuto in mente...
... questo però non lo committo

[fek]

Originariamente inviato da Bonfo

E in piu' non ti devi tenere una lista di supporto per sapere se quell'oggetto l'hai gia' visitato o no.

Mi ricordo che subito dopo la FirstPlayable abbiamo trasformato grid da array di celle a lista di droppable proprio per poter itereare solo sugli oggetti dentro la grid.

Il problema che avevo incontrato era l'iterazione mutlipla perche' usavo il foreach (":") , ma con l'iterator il problema si risolve ( o almeno cosi' dovrebbe essere)

Buona idea, fai pure.

[fek]

Originariamente inviato da Bonfo

Dopo

    public void testGemSpritePositions()
    {
        grid.insertDroppable(gem, 4, 2);
        checkSpritePosition(gem.getAnimatedSprite().getPosition(), 4, 2);

        Droppable anotherGem = createGem(DroppableColor.DIAMOND);
        grid.insertDroppable(anotherGem, 3, 5);
        checkSpritePosition(anotherGem.getAnimatedSprite().getPosition(), 3, 5);
    }


    private void checkSpritePosition(Point spritePosition, int row, int column)
    {
        assertEquals(gridPosition.getX() + Cell.SIZE_IN_PIXELS * column,
            spritePosition.getX());
        assertEquals(gridPosition.getY() + Cell.SIZE_IN_PIXELS * row,
            spritePosition.getY());
    }

Mi piace. Rinomina solo checkSpritePosition() in assertSpritePosition() e poi spezza il test in due test separati, eventualmente nel loro TestCase.

[Ufo13]

Bonfo per l'eccezione su Cell usa un factory method magari che ritorna null se la cell non e` valida...

Per quant origuarda la lista di droppable: per me dovrebbe essere privata ed inerente all'implementazione interna di Grid. Per questo volevo creare un command per iterare le celle..

[71104]

i due "refactor this" che stanno in CrushByFlashAction sono una goduria
me ne occupo io

PS: dite addio a CrushByFlashAction perché temo che stia per scomparire

[fek]

Originariamente inviato da Ufo13

Bonfo per l'eccezione su Cell usa un factory method magari che ritorna null se la cell non e` valida...

Per quant origuarda la lista di droppable: per me dovrebbe essere privata ed inerente all'implementazione interna di Grid. Per questo volevo creare un command per iterare le celle..

Hmmm... sono combattuto. Ci penso su e vi dico. Perche' Iterator e' un Pattern che e' comunque slegato dall'implementazione interna di una classe ma serve a iterare qualunque struttura dati. Ed ha molto senso iterare una griglia, piu' semplice che creare un Command Pattern solo per questo.

[Ufo13]

Originariamente inviato da fek

Hmmm... sono combattuto. Ci penso su e vi dico. Perche' Iterator e' un Pattern che e' comunque slegato dall'implementazione interna di una classe ma serve a iterare qualunque struttura dati. Ed ha molto senso iterare una griglia, piu' semplice che creare un Command Pattern solo per questo.

un metodo pubblico come getDroppableList() rompe completamente l'incapsulazione di Grid. A questo punto potrei implementarmi qualunque metodo di grid tipo getDroppableAt() al di fuori di essa. Un iterator preserverebbe l'incapsulazione e sarebbe facile cambiare l'implementazione interna di Grid senza dover cambiare 300 file

[AnonimoVeneziano]

Originariamente inviato da 71104

i due "refactor this" che stanno in CrushByFlashAction sono una goduria
me ne occupo io

PS: dite addio a CrushByFlashAction perché temo che stia per scomparire

Tutto sto sistema degli action è da eliminare .

Cioè, è assurdo che ogni volta che deve essere fatta una iterazione di una Action parte un ciclo che si fa tutta la griglia e poi all'esecuzione dell'azione stessa su un solo droppable parte un altro ciclo che si rifà tutta la griglia (come nel caso di FlashAction).

Ma un sistema ad eventi? Con dei listener ad esempio (come in Swing) . Quando una gemma smette di cadere si può fare che lancia un evento che ad esempio fa il controllo se ci sono gemme da crushare o che la fanno crushare attorno a se e cose del genere. Era già stato pensato?

Ciao

[fek]

Originariamente inviato da Ufo13

un metodo pubblico come getDroppableList() rompe completamente l'incapsulazione di Grid. A questo punto potrei implementarmi qualunque metodo di grid tipo getDroppableAt() al di fuori di essa. Un iterator preserverebbe l'incapsulazione e sarebbe facile cambiare l'implementazione interna di Grid senza dover cambiare 300 file

Infatti getDroppableList() deve sparire in favore di un iterator.

[fek]

Originariamente inviato da AnonimoVeneziano

Ma un sistema ad eventi? Con dei listener ad esempio (come in Swing) . Quando una gemma smette di cadere si può fare che lancia un evento che ad esempio fa il controllo se ci sono gemme da crushare o che la fanno crushare attorno a se e cose del genere. Era già stato pensato?

Ciao

No, ma pian piano ci stiamo muovendo verso qualcosa di simile attraverso il refactoring. Le Action devono sparire. Stiamo facendo un gran lavoro per semplificare Droppable e disaccoppiarla da tutto cio' che non le compete (animazioni e sprite ad esempio). Ma esistono non chiare dipendente con le Action e questo si sta rivelando un piccolo scoglio prima della rimozione completa delle Action. Comunque siamo a discreto punto: AnimatedSprite e' gia' quasi del tutto sparito ed e' stata creata una classe DroppableRenderer per gestire animazioni e sprite. Ora la si deve solo tirare fuori da li' definitivamente (e non e' facile). E' il mio compito del prossimo finesettimana.

[AnonimoVeneziano]

Originariamente inviato da fek

Le Action devono sparire.

Questo mi compiace assai

[Ufo13]

Originariamente inviato da fek

Infatti getDroppableList() deve sparire in favore di un iterator.

Faccio io stasera, ho gia` in mente tutti i test

Porca trota mentre postavo qui e` crashato visual studio :o

Tutta colpa del mio uber shader subsurface scattering + raytraced reflections da 10 istruzioni!!111

[fek]

Ahaha gli uber shader di subsurface scattering sono sempre i migliori: flip della normale, NdotL rovesciato con triplo carpiato e passa la paura del subsurface scattering
E nessuno se ne accorge mai.

[71104]

è normale che per il refactor this di CrushByFlashAction io debba aggiungere miliardi di test?

il fatto è che sto spostando CrushByFlashAction.updateCrushesOnFlash in un nuovo metodo FlashingGem.makeCrush, e con esso se ne stanno andando anche tutti i metodi privati "di servizio" che c'erano nella action; alcuni di questi metodi sono andati in FlashingGem, altri in Grid, e siccome quelli finiti in Grid sono pubblici li ho dovuti riscrivere test-driven