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]

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 :muro:

[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:stordita:

[Jocchan]

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

[fek]

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

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]

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


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


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?:stordita: mea culpa, cerco almeno di fare un sorce -> format prima di committare :stordita:



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:stordita:

[jappilas]

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

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

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]

:eek:

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


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:sofico: . 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]

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


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?:stordita: mea culpa, cerco almeno di fare un sorce -> format prima di committare :stordita:

Si', e' cosi' incomprensibile :D
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:stordita:

Dita o ginocchia.

[fek]

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:sofico: . 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]

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]

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...
http://i28.tinypic.com/5nlq46.jpg
;)

[thebol]

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]

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

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]

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]

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 IMHOche infatti mi piace perchè semplifica il codice attuale :)

[Bonfo]

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]

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

[thebol]

refactoring fatto, manca solo da mettere a posto i test.

Fra l'altro gia che c'ero ho messo anche a posto il fatto che le crushByFlesh contassero come crush. Questo perchè ho cambiato la firma di Droppable.crushGemOn, facendole restituire un booleano. True se c'è stata una crush, false altrimenti.

Il metodo di chest restituisce true e quello di flashingGem false :)

Il tutto però ancora da testare.......

[Bonfo]

Anch'io sulla esistenza di DroppableList ho qualch dubbio...sicuramente sara' un requisito Fekkiano di leggibilita'.

Per il resto ok, era solo per capire ;)

[Ufo13]

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

Non potremmo farlo sempre di default durante una runIteration?

[thebol]

Non potremmo farlo sempre di default durante una runIteration?

è quello che ho fatto ;)

anche se va fatto anche altri 2 punti (updateBigGem e updateCrush sempre di Grid)

[thebol]

committato