Buggone di livello A: stone che non si trasformano

[Jocchan]

Il gioco crasha ad ogni partita perchè le stone non si trasformano in gemme:

OS: Windows XP
Version: 5.1
Architecture: x86

VM Vendor: Sun Microsystems Inc.
Version: 1.6.0_02

Class Path: DiamondCrush.exe
JNI Library Path: lib/win32

Exception: class java.util.ConcurrentModificationException
Message: null
Display Adapter Driver: ati2dvag 6.14.10.6606

Stacktrace:
java.util.ConcurrentModificationException
	at java.util.AbstractList$Itr.checkForComodification(Unknown Source)
	at java.util.AbstractList$Itr.next(Unknown Source)
	at it.diamonds.grid.Grid.runIteration(Unknown Source)
	at it.diamonds.grid.state.StoneTurnState.update(Unknown Source)
	at it.diamonds.grid.state.GemsPairOnControlState.update(Unknown Source)
	at it.diamonds.GameTurn.update(Unknown Source)
	at it.diamonds.grid.GridController.update(Unknown Source)
	at it.diamonds.PlayField.updatePlayField(Unknown Source)
	at it.diamonds.PlayField.update(Unknown Source)
	at it.diamonds.GameLoop.updateState(Unknown Source)
	at it.diamonds.AbstractLoop.loopStep(Unknown Source)
	at it.diamonds.Game.loopStep(Unknown Source)
	at it.diamonds.Game.loop(Unknown Source)
	at it.diamonds.Game.main(Unknown Source)

Chi se ne occupa? Capita ad ogni partita, quindi schizza di diritto alla massima priorità.

[fek]

Dire che stiamo andando a rilento e siamo sbadati e' un eufemismo.
Siamo proprio fermi. Allora?

Questo e' un bug gravissimo che va sistemato ora. Chi se ne occupa?

[cisc]

Originariamente inviato da fek

Dire che stiamo andando a rilento e siamo sbadati e' un eufemismo.
Siamo proprio fermi. Allora?

Questo e' un bug gravissimo che va sistemato ora. Chi se ne occupa?

me ne occupo io...ma devo aggiungere un po di roba a Grid, il refactoring che volevo fare in concomitanza al bug mi risulta un po lunghetto in periodo di esami

[cisc]

ho introdotto un sistema di asynchronousInsertion delle droppable in Grid. Allora, test che evidenzia il bug:

  public void testNoCuncurrentModificationOnStoneTransformation()
  {
      try
      {
          Droppable anotherStone = createStone(DIAMOND);
          stone.getAnimatedSprite().setCurrentFrame(5);
          anotherStone.getAnimatedSprite().setCurrentFrame(5);
          grid.insertDroppable(anotherStone, 5, 0);
          grid.insertDroppable(stone, 5, 2);
          grid.runIteration(iteration);
          grid.garbageCollectDroppables();
          grid.performAsynchronousInsertions();
      }
      catch (ConcurrentModificationException e)
      {
          fail();
      }

  }

per farlo passare ho introdotto i seguenti metodi in grid:

    public void asynchronousInsertDroppable(Droppable droppable, int row, int column)
    {
        canInsertDroppable(droppable, row, column);

        setDroppablePositionInGrid(droppable, row, column);

        droppablesAsynchronousInserted.add(droppable);
    }


    private void setDroppablePositionInGrid(Droppable droppable, int row, int column)
    {
        droppable.getRegion().setRow(row);
        droppable.getRegion().setColumn(column);
        alignDroppableToCellUpperBound(droppable);
    }


    private void canInsertDroppable(Droppable droppable, int row, int column)
    {
        if (droppable == null)
        {
            throw new IllegalArgumentException();
        }
        if (isCellOccupiedWithNoToGarbageCollectDroppable(row, column))
        {
            throw new IllegalArgumentException();
        }
        if (droppableList.contains(droppable))
        {
            throw new IllegalArgumentException();
        }
        if (droppablesAsynchronousInserted.contains(droppable))
        {
            throw new IllegalArgumentException();
        }
    }


    private boolean isCellOccupiedWithNoToGarbageCollectDroppable(int row, int column)
    {
        return !isCellFree(Cell.create(row, column)) && !droppablesToGarbageCollect.contains(getDroppableAt(Cell.create(row, column)));
    }


    public void performAsynchronousInsertions()
    {
        droppableList.addAll(droppablesAsynchronousInserted);
        droppablesAsynchronousInserted.clear();
    }

e relativi test:

public class TestGridAsynchronousInsertions extends GridTestCase
{
    public void setUp()
    {
        super.setUp();
    }

    public void testNoSynchronousInsertion()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);

        grid.asynchronousInsertDroppable(droppable, 5, 0);
        assertNull(grid.getDroppableAt(Cell.create(5, 0)));
    }
    
    public void testAsynchronousInsertion()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);

        grid.asynchronousInsertDroppable(droppable, 5, 0);
        grid.performAsynchronousInsertions();
        assertSame(droppable,grid.getDroppableAt(Cell.create(5, 0)));
    }
    
    public void testNoAsynchronousInsertionOnNonFreeCell()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);
        grid.insertDroppable(droppable, 0, 0);
        
        try
        {
            grid.asynchronousInsertDroppable(droppable, 0, 0);
            fail();
        }
        catch (IllegalArgumentException e)
        {
            ;
        }
    }
    
    public void testNoAsynchronousInsertionOfAlreadyInsertedDroppable()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);
        grid.insertDroppable(droppable, 0, 0);
        
        try
        {
            grid.asynchronousInsertDroppable(droppable, 5, 0);
            fail();
        }
        catch (IllegalArgumentException e)
        {
            ;
        }
    }
    
    public void testNoTwoAsynchronousInsertionOfSameDroppable()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);
        grid.asynchronousInsertDroppable(droppable, 0, 0);
        
        try
        {
            grid.asynchronousInsertDroppable(droppable, 5, 0);
            fail();
        }
        catch (IllegalArgumentException e)
        {
            ;
        }
    }
    
    public void testNoNullDroppableAsynchronousInserted()
    {
        try
        {
            grid.asynchronousInsertDroppable(null, 5, 0);
            fail();
        }
        catch (IllegalArgumentException e)
        {
            ;
        }
    }
    
    public void testNoDroppableInsertionOfAlreadyAsnchronousInsertedDroppable()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);
        grid.asynchronousInsertDroppable(droppable, 0, 0);
        
        try
        {
            grid.insertDroppable(droppable, 5, 0);
            fail();
        }
        catch (IllegalArgumentException e)
        {
            ;
        }
    }
    
    public void testAsynchronousInsertionOnCellBeOccupiedWithToGarbageCollectDroppable()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);
        Droppable droppableToRemove = createGem(DroppableColor.DIAMOND);

        grid.insertDroppable(droppableToRemove, 5, 0);
        grid.addDroppableToGarbage(droppableToRemove);
        try
        {
            grid.asynchronousInsertDroppable(droppable, 5, 0);
        }
        catch (IllegalArgumentException e)
        {
            fail();
        }
    }
    
    public void testNoDoubleAsynchronousInsertion()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);

        grid.asynchronousInsertDroppable(droppable, 5, 0);
        grid.performAsynchronousInsertions();
        grid.performAsynchronousInsertions();
        assertEquals(1, grid.getNumberOfDroppables());
    }
    
    public void testDroppableAsynchronousInsertedInRightPlace()
    {
        Droppable droppable = createGem(DroppableColor.DIAMOND);

        grid.asynchronousInsertDroppable(droppable, 5, 3);
        grid.performAsynchronousInsertions();
        assertEquals(gridPosition.getX()+3*Cell.SIZE_IN_PIXELS, droppable.getAnimatedSprite().getPosition().getX());
        assertEquals(gridPosition.getY()+5*Cell.SIZE_IN_PIXELS, droppable.getAnimatedSprite().getPosition().getY());
    }

}

Qualche nome di metodo forse è un po forzato, ma non m'è venuto in mente altro

[Jocchan]

Il bug è risolto quindi? Con l'ultima build non mi si presenta.

[fek]

Originariamente inviato da cisc

Qualche nome di metodo forse è un po forzato, ma non m'è venuto in mente altro

cisc! Stiamo semplificando Grid, non voglio che gli sbatti dentro altri dieci metodi. Reverta tutto, e crea una classe se necessario.

E poi, non so veramente piu' come dirtelo, gli spazi non costano nulla, metti questi spazi attorno agli operatori e le interlinee. I tuoi metodi sono un wall text spesso incomprensibile.

Inoltre:

   private void canInsertDroppable(Droppable droppable, int row, int column)
    {
        if (droppable == null)
        {
            throw new IllegalArgumentException();
        }
        if (isCellOccupiedWithNoToGarbageCollectDroppable(row, column))
        {
            throw new IllegalArgumentException();
        }
        if (droppableList.contains(droppable))
        {
            throw new IllegalArgumentException();
        }
        if (droppablesAsynchronousInserted.contains(droppable))
        {
            throw new IllegalArgumentException();
        }
    }

Le eccezioni vanno usate solo in casi, appunto, eccezionali, non per comunicare condizioni d'errore come in questo caso. Questa catena di if e' da eliminare.

In futuro sto seriamente pensando a importi alcune regole:

- se scrivi un metodo in versione wall-text faccio il revert
- se scrivi anche un solo if faccio il revert
- se aggiungi un metodo alla classe Grid faccio il revert

E poi ti spezzo le ginocchia

[cisc]

Originariamente inviato da fek

cisc! Stiamo semplificando Grid, non voglio che gli sbatti dentro altri dieci metodi. Reverta tutto, e crea una classe se necessario.

beh dai, faceva parte del refactoring che avevo in mente di fare togliere sti metodi che ho aggiunto e il garbage collector da grid

E poi, non so veramente piu' come dirtelo, gli spazi non costano nulla, metti questi spazi attorno agli operatori e le interlinee. I tuoi metodi sono un wall text spesso incomprensibile.

così incomprensibile? mea culpa, cerco almeno di fare un sorce -> format prima di committare

Inoltre:

   private void canInsertDroppable(Droppable droppable, int row, int column)
    {
        if (droppable == null)
        {
            throw new IllegalArgumentException();
        }
        if (isCellOccupiedWithNoToGarbageCollectDroppable(row, column))
        {
            throw new IllegalArgumentException();
        }
        if (droppableList.contains(droppable))
        {
            throw new IllegalArgumentException();
        }
        if (droppablesAsynchronousInserted.contains(droppable))
        {
            throw new IllegalArgumentException();
        }
    }

Le eccezioni vanno usate solo in casi, appunto, eccezionali, non per comunicare condizioni d'errore come in questo caso. Questa catena di if e' da eliminare.

questa catena d'if già c'era in grid, l'ho solo estratta in un metodo, notando una certa urgenza nel risolvere il bug, e non avendo io molto tempo per pensare ad un refactoring più corposo, ho preferito committare la risoluzione del bug per poi rimandare il refactoring a dopo

In futuro sto seriamente pensando a importi alcune regole:

- se scrivi un metodo in versione wall-text faccio il revert
- se scrivi anche un solo if faccio il revert
- se aggiungi un metodo alla classe Grid faccio il revert

E poi ti spezzo le ginocchia

no le ginocchia noooo

[jappilas]

Originariamente inviato da cisc

ho introdotto un sistema di asynchronousInsertion delle droppable in Grid.
<...>

ma... ma... passare a qualcosa del tipo "for(...) droppable = droppable.transform()" senza complicarsi la vita con inserimenti asincroni ( e soprattutto eliminando una iteration ) ?

non fare l' errore che faceva (e qualche volta ancora fa ) il sottoscritto, non cedere alla tentazione di aggiungere 1000 righe di codice per qualcosa che magari si può fare con una o due...

[thebol]

Originariamente inviato da jappilas

ma... ma... passare a qualcosa del tipo "for(...) droppable = droppable.transform()" senza complicarsi la vita con inserimenti asincroni ( e soprattutto eliminando una iteration ) ?

il problema e che il metodo che dovrebbe inserirle, riceve le gemme da esaminare da un iterator. L'iterator è sulla griglia.


Ops. Ho appena avuto un idea geniale . Quando si fa l'iterator, si fa la clone della lista e poi si cicla su quella. Le insert, remove, sticazzi, saranno sulla lista originale.

E' un idea che mi è venuta durante sto post, ma a braccio dovrebbe funzionare.

E dovrebbe eliminare sia l'inserimento asincrono, sia il garbage collector delle gemme.

[fek]

Originariamente inviato da cisc

beh dai, faceva parte del refactoring che avevo in mente di fare togliere sti metodi che ho aggiunto e il garbage collector da grid

Ok, toglitelo dalla mente ora, non voglio vedere nulla con asynchronous.
Va benissimo togliere il garbage collector. Prova con il suggerimenti di jappi.

così incomprensibile? mea culpa, cerco almeno di fare un sorce -> format prima di committare

Si', e' cosi' incomprensibile
Non basta fare un format, devi proprio imparare a raggruppare il codice per blocchi logici (che poi estrarrai in metodi ). E metti quegli spazi fra gli operatori.

questa catena d'if già c'era in grid, l'ho solo estratta in un metodo, notando una certa urgenza nel risolvere il bug, e non avendo io molto tempo per pensare ad un refactoring più corposo, ho preferito committare la risoluzione del bug per poi rimandare il refactoring a dopo

Ok, la prossima volta metti un REFACTOR THIS e scrivi come fare il refactoring se non hai il tempo di farlo tu.

no le ginocchia noooo

Dita o ginocchia.

[fek]

Originariamente inviato da thebol

il problema e che il metodo che dovrebbe inserirle, riceve le gemme da esaminare da un iterator. L'iterator è sulla griglia.


Ops. Ho appena avuto un idea geniale . Quando si fa l'iterator, si fa la clone della lista e poi si cicla su quella. Le insert, remove, sticazzi, saranno sulla lista originale.

E' un idea che mi è venuta durante sto post, ma a braccio dovrebbe funzionare.

E dovrebbe eliminare sia l'inserimento asincrono, sia il garbage collector delle gemme.

Si'!

[fek]

Originariamente inviato da jappilas

non fare l' errore che faceva (e qualche volta ancora fa ) il sottoscritto, non cedere alla tentazione di aggiungere 1000 righe di codice per qualcosa che magari si può fare con una o due...

Ho le lacrime agli occhi.

[Jocchan]

Originariamente inviato da jappilas

non fare l' errore che faceva (e qualche volta ancora fa ) il sottoscritto, non cedere alla tentazione di aggiungere 1000 righe di codice per qualcosa che magari si può fare con una o due...

[thebol]

Originariamente inviato da fek

Si'!

ho provato a rotta di collo in treno e funziona.

Ora va fatta un po meglio,e messi a posto anche dei test(alcuni nn compilavano).

io nn posso, ho un altro refactoring in corso e stasera sono occupato a soffrire

[fek]

A che punto siamo con questo bug?

[Ufo13]

Originariamente inviato da jappilas

ma... ma... passare a qualcosa del tipo "for(...) droppable = droppable.transform()" senza complicarsi la vita con inserimenti asincroni ( e soprattutto eliminando una iteration ) ?

Questa non e` la soluzione al problema. Non hai piu` l'eccezione ma ottieni altri bug peggiori.

La soluzione e` il clone della lista IMHO

[thebol]

Originariamente inviato da Ufo13

Questa non e` la soluzione al problema. Non hai piu` l'eccezione ma ottieni altri bug peggiori.

La soluzione e` il clone della lista IMHO

per il clone basta fare new LinkedList(droppables). Va fatto in 2 punti del codice(quello che itera e da un altra parte, basta cercare dove viene usata la garbageDeletion).

poi si toglie la garbage deletione e si rimette quella normale. Uguale per l'inserimento

[jappilas]

Originariamente inviato da Ufo13

Questa non e` la soluzione al problema. Non hai piu` l'eccezione ma ottieni altri bug peggiori.

quali?
comunque il senso non era che *quella* fosse una soluzione perfetta, ma suggerire di cercare una soluzione (qualsiasi) rapida, che sfrutti quello che già abbiamo e il polimorfismo quanto più possibile

La soluzione e` il clone della lista IMHO

che infatti mi piace perchè semplifica il codice attuale

[Bonfo]

Originariamente inviato da thebol

per il clone basta fare new LinkedList(droppables). Va fatto in 2 punti del codice(quello che itera e da un altra parte, basta cercare dove viene usata la garbageDeletion).

poi si toglie la garbage deletione e si rimette quella normale. Uguale per l'inserimento

Ehm... perche' LinkedList e non ArrayList? o addirittura aggiungere il costruttore DroppableList(DroppableList list)?

E poi non c'e' gia' il metodo clone() che fa tutto?

[thebol]

Originariamente inviato da Bonfo

Ehm... perche' LinkedList e non ArrayList?

perchè per iterare è più efficiente una linkedList, e perchè manco avevo visto che era un arrayList(alla fine è una lista temporanea, può essere quello che vuole)

o addirittura aggiungere il costruttore DroppableList(DroppableList list)?

manco avevo visto che era una droppableList(ma che serve poi sta classe? a risparmiarsi List<Droppable> nel codice???). Perchè aggiungere un costruttore quando si può fare senza? La lista sarebbe temporanea per il for each e basta.

E poi non c'e' gia' il metodo clone() che fa tutto?

il metodo clone lo eviterei(vai a sapere come lo implementa una classe..), se cè il costruttore per copia imho e meglio usare quello.


btw provo a fare il refactoring