View Full Version : Programma "Refactor This!" [Da leggere]
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!"
http://img137.imageshack.us/img137/6697/kenpq9.jpg
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.
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 ;)
bella come idea :D
mi metto a caccia.
^TiGeRShArK^
03-02-2008, 22:35
oh porca :mbe:
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 :Prrr: )
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 :asd:
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;
}
mi ci sono scervellato, meglio di così non riesco a fare: non riesco ad eliminare quegli if (PER ORA). committo così com'è.
minchia, guardate che roba :asd:
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);
}
:sofico:
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. :muro:
Ma poi mi speighi il vantaggio di averli statici?!?
Io no sono lo spezzatore di dita ufficiale, ma poi finisce che chiedo una delega . :Perfido: :Perfido:
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. :lamer:
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. ;)
Minchia Bonfo avevamo appena iniziato a cambiare tutto in Cell e hai ri-revertato i cambiamenti fatti :D
L'idea e` usare Cell ovunque.
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.
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.
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.
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...
Si', e' chiaro che se stai facendo una serie di commit come me in questo momento, basta un solo TODO.
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 :O
AnonimoVeneziano
04-02-2008, 19:08
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 :stordita: )
Okkei?
(Non sono molto esperto di refactoring :stordita: ) 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
04-02-2008, 19:15
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 :)
Minchia Bonfo avevamo appena iniziato a cambiare tutto in Cell e hai ri-revertato i cambiamenti fatti :D
L'idea e` usare Cell ovunque.
Azz... ho fatto un macello :doh:
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 ?! :D :D
AnonimoVeneziano
04-02-2008, 20:40
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
Sono esentato dal Refactor This oppure prima li faccio fuori tutti e poi posso andare avanti ?! :D :D
La seconda che hai detto :D
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
04-02-2008, 21:02
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
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
04-02-2008, 21:10
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:
Quindi ritorno alla vecchia soluzione con l'if semplificato?
Vedi tu :)
AnonimoVeneziano
04-02-2008, 21:21
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
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
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));
}
Minchia Bonfo avevamo appena iniziato a cambiare tutto in Cell e hai ri-revertato i cambiamenti fatti :D
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)
Io sto rimettendo tutti i cell che avevo tolto! :muro: :D
Mi ci vorra' un po', anche perche' nel mentre lavoro :P
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 :D
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;
}
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
05-02-2008, 00:06
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 ...
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();
}
Piccolo problemi che ritorna prepotente...
Quando creo una cella con valori negativi mi becco una exception.
Che faccio: aggiungo controlli ovunque?? :muro:
in realta' basta che isValidCell continui aragionare a row e column. Che si fa?
AnonimoVeneziano
05-02-2008, 01:15
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 :stordita:
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 ...
E in piu' non ti devi tenere una lista di supporto per sapere se quell'oggetto l'hai gia' visitato o no. :D
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)
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?
EDIT: cosa intendi per iterazione multipla?
java.util.ConcurrentModificationException :D :D
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 :nonio:
Non è sufficente farsi dare due iteratori diverisi, il problema si pone lo stesso, ovvero e un po' più complicato da risolvere :(
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());
}
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());
}
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 :stordita: :stordita:
E in piu' non ti devi tenere una lista di supporto per sapere se quell'oggetto l'hai gia' visitato o no. :D
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.
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.
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..
i due "refactor this" che stanno in CrushByFlashAction sono una goduria :uh: :Perfido:
me ne occupo io :boxe: :huh:
PS: dite addio a CrushByFlashAction perché temo che stia per scomparire :asd:
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.
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
05-02-2008, 10:42
i due "refactor this" che stanno in CrushByFlashAction sono una goduria :uh: :Perfido:
me ne occupo io :boxe: :huh:
PS: dite addio a CrushByFlashAction perché temo che stia per scomparire :asd:
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
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.
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
05-02-2008, 10:48
Le Action devono sparire.
Questo mi compiace assai :D
Infatti getDroppableList() deve sparire in favore di un iterator.
Faccio io stasera, ho gia` in mente tutti i test :D
Porca trota mentre postavo qui e` crashato visual studio :o
Tutta colpa del mio uber shader subsurface scattering + raytraced reflections da 10 istruzioni!!111
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 :D
E nessuno se ne accorge mai.
è normale che per il refactor this di CrushByFlashAction io debba aggiungere miliardi di test? :mc:
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 :huh:
makeCrush? Mi sembra un nome criptico, ne puoi trovare uno piu' esplicito?
Ci posti anche il codice dei metodi che stai aggiungendo a Grid?
Per tutti. Ricordate che al seguente indirizzo trovate TUTTO il codice di Diamonds con relativa complessita' e copertura dei test:
http://fcarucci.homeip.net:8080/cruisecontrol/buildresults/diamonds-nightly?tab=Cobertura
Questo e' CrushByFlashAction:
http://fcarucci.homeip.net:8080/cruisecontrol/artifacts/diamonds-nightly/20080205010028/cobertura/it.diamonds.grid.action.CrushByFlashAction.html
CrushByFlashAction.getGem diventa Grid.getCrushableByFlashAt:
public Droppable getCrushableByFlashAt(Cell cell)
{
Droppable gem = 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;
}
relativi tests:
public void testCrushableAt()
{
Droppable gem = createGem(EMERALD);
grid.insertDroppable(gem, 4, 2);
gem.getFallingObject().drop();
assertSame(gem, grid.getCrushableByFlashAt(new Cell(4, 2)));
}
public void testNoCrushableAt()
{
assertNull(grid.getCrushableByFlashAt(new Cell(4, 2)));
}
public void testNotDroppedNotCrushable()
{
Droppable gem = createGem(EMERALD);
grid.insertDroppable(gem, 4, 2);
assertNull(grid.getCrushableByFlashAt(new Cell(4, 2)));
}
public void testStoneIsNotCrushable()
{
Droppable stone = createStone(EMERALD);
grid.insertDroppable(stone, 4, 2);
stone.getFallingObject().drop();
assertNull(grid.getCrushableByFlashAt(new Cell(4, 2)));
}
altro metodo spostato in Grid, getAllGemsSameOf:
public DroppableList getAllGemsSameOf(Droppable gemToDelete)
{
DroppableList gemSameList = new DroppableList();
for(Droppable droppable : droppableList)
{
if(droppable.getGridObject().getColor() == gemToDelete.getGridObject().getColor())
{
gemSameList.add(droppable);
}
}
return gemSameList;
}
tests:
public void testOneGemSameOf()
{
Droppable emerald = createGem(EMERALD);
grid.insertDroppable(emerald, 1, 2);
assertTrue(grid.getAllGemsSameOf(emerald).contains(emerald));
}
public void testTwoGemsSameOf()
{
Droppable oneEmerald = createGem(EMERALD);
Droppable anotherEmerald = createGem(EMERALD);
grid.insertDroppable(oneEmerald, 1, 2);
grid.insertDroppable(anotherEmerald, 3, 4);
assertTrue(grid.getAllGemsSameOf(oneEmerald).contains(anotherEmerald));
}
public void testGemNotSameOf()
{
Droppable emerald = createGem(EMERALD);
Droppable diamond = createGem(DIAMOND);
grid.insertDroppable(emerald, 1, 2);
grid.insertDroppable(diamond, 3, 4);
assertFalse(grid.getAllGemsSameOf(emerald).contains(diamond));
}
per finire, in AbstractCrushAction c'era un metodo che crushava una lista di droppables; questo metodo si basava su un flag "addToScore" che determinava se i droppables crushati dovevano contare nel punteggio (il flag è falso quando il crush viene fatto da una flashing gem, vero quando viene fatto da un chest), quindi oltre che spostare quel metodo in Grid mi son detto: piuttosto che usare un flag ad indicare quando il metodo viene chiamato da una parte e quando dall'altra, lo divido in due metodi (uno chiamato da una parte e uno dall'altra :D).
ecco il risultato:
public void crushDroppablesWithScore(DroppableList crushableGems)
{
for(Droppable gemToCrush : crushableGems)
{
getScoreCalculator().addGem(gemToCrush);
crushedGemsCounter += gemToCrush.crush(this);
}
}
public void crushDroppablesWithoutScore(DroppableList crushableGems)
{
for(Droppable gemToCrush : crushableGems)
{
gemToCrush.crush(this);
}
}
ed i tests:
public void testCrushOneGemWithoutScore()
{
MockGem gem = new MockGem(environment.getEngine());
DroppableList list = new DroppableList();
list.add(gem);
grid.crushDroppablesWithoutScore(list);
assertTrue(gem.isCrushed());
}
public void testCrushTwoGemsWithoutScore()
{
MockGem oneGem = new MockGem(environment.getEngine());
MockGem anotherGem = new MockGem(environment.getEngine());
DroppableList list = new DroppableList();
list.add(oneGem);
list.add(anotherGem);
grid.crushDroppablesWithoutScore(list);
assertTrue(oneGem.isCrushed());
assertTrue(anotherGem.isCrushed());
}
public void testCrushOneGemWithScore()
{
MockGem gem = new MockGem(environment.getEngine());
DroppableList list = new DroppableList();
list.add(gem);
grid.crushDroppablesWithScore(list);
assertTrue(gem.isCrushed());
}
public void testOneCrushedGemWithScoreCounter()
{
MockGem gem = new MockGem(environment.getEngine());
DroppableList list = new DroppableList();
list.add(gem);
grid.crushDroppablesWithScore(list);
assertEquals(1, grid.getCrushedGemsCounter());
}
public void testCrushTwoGemsWithScore()
{
MockGem oneGem = new MockGem(environment.getEngine());
MockGem anotherGem = new MockGem(environment.getEngine());
DroppableList list = new DroppableList();
list.add(oneGem);
list.add(anotherGem);
grid.crushDroppablesWithScore(list);
assertTrue(oneGem.isCrushed());
assertTrue(anotherGem.isCrushed());
}
public void testTwoCrushedGemsWithScoreCounter()
{
MockGem oneGem = new MockGem(environment.getEngine());
MockGem anotherGem = new MockGem(environment.getEngine());
DroppableList list = new DroppableList();
list.add(oneGem);
list.add(anotherGem);
grid.crushDroppablesWithScore(list);
assertEquals(2, grid.getCrushedGemsCounter());
}
public void testScoreOfOneCrushedGem()
{
MockGem gem = new MockGem(environment.getEngine(), 10);
DroppableList list = new DroppableList();
list.add(gem);
grid.crushDroppablesWithScore(list);
assertEquals(10, grid.getScoreCalculator().getTempScore());
}
public void testScoreOfAnotherCrushedGem()
{
MockGem gem = new MockGem(environment.getEngine(), 5);
DroppableList list = new DroppableList();
list.add(gem);
grid.crushDroppablesWithScore(list);
assertEquals(5, grid.getScoreCalculator().getTempScore());
}
public void testScoreOfTwoCrushedGems()
{
MockGem oneGem = new MockGem(environment.getEngine(), 10);
MockGem anotherGem = new MockGem(environment.getEngine(), 20);
DroppableList list = new DroppableList();
list.add(oneGem);
list.add(anotherGem);
grid.crushDroppablesWithScore(list);
assertEquals(30, grid.getScoreCalculator().getTempScore());
}
makeCrush? Mi sembra un nome criptico, ne puoi trovare uno piu' esplicito? più esplicito di così... quel metodo serve esattamente a "fare il crush"; io ho un oggetto (nello specifico una flashing gem) in grado di effettuare un crush su altri droppables e voglio fare sto crush: chiamo il metodo "makeCrush" e gli passo la lista dei droppables da crushare :D
Questo e' CrushByFlashAction:
http://fcarucci.homeip.net:8080/cruisecontrol/artifacts/diamonds-nightly/20080205010028/cobertura/it.diamonds.grid.action.CrushByFlashAction.html ti faccio notare che i miei nuovi tests coprono quella riga che era rimasta scoperta :asd:
vedi testNotDroppedNotCrushable
ok, committato uno dei due REFACTOR THIS di CrushByFlashAction. ho il fiatone :mc:
manca l'altro refactor this prima di ammazzare la action; se qualcuno non lo fa entro domani sera ci penso io :D
CrushByFlashAction.getGem diventa Grid.getCrushableByFlashAt:
public Droppable getCrushableByFlashAt(Cell cell)
{
Droppable gem = 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;
}
Ok, ora faccio il precisino. Non mi piace per vari motivi:
1- E' un metodo troppo specifico e Grid non dovrebbe avere una tale conoscenza di concetti come Stone, Flash, etc. Dovrebbe limitarsi a fornire strumenti piu' generici ad altre entita' che poi si occupano di queste ricerche specifiche.
2- Ci sono una marea di if e non mi piacciono. Il primo si elimina facilmente con un NullObject su Gem e mi sembra il caso di aggiungerlo ormai.
3- gem.getGridObject().getType().isStone(). Regola di Demeter, ovvero cose come getThis().getItFromThere().getItActuallyHere().getWellItWasThere() sono orribili. La regola dice che nessun metodo dev'essere invocato su un oggetto ritornato da un altro oggetto. L'oggetto che usi deve avere quel metodo che invoca l'oggetto al suo interno.
4- Ma in questo caso non possiamo aggiungere isStone() a Gem, quindi dobbiamo togliere quel comportamento basato su un getType() mascherato. Puoi farlo tu?
Al lavoro sfaticato! :D
Al lavoro sfaticato! :D
SEEEEE, al lavoro cosaa :mc:
io devo studià oooooo :D
Diamond Crush lo rivedrò domani sera :Prrr:
per ora il massimo che posso fare è mettere un refactor this su getAllGemsSameOf.
1- E' un metodo troppo specifico e Grid non dovrebbe avere una tale conoscenza di concetti come Stone, Flash, etc. Dovrebbe limitarsi a fornire strumenti piu' generici ad altre entita' che poi si occupano di queste ricerche specifiche. il motivo percui ho deciso di metterlo in Grid era perché quel metodo getGem che prima era nella action non faceva minimamente uso di ciò che gli forniva il "this" di turno tranne che per il metodo getGrid: lo chiamava e lavorava su ciò che otteneva dalla griglia; cioè lavorava sulla griglia; di conseguenza l'ho spostato. però giustamente a guardare tutti i controlli che fa direi che lavora più sulla gemma che sulla griglia, quindi potrei pensare al modo di dividerlo e/o spostarlo ancora.
2- Ci sono una marea di if e non mi piacciono. Il primo si elimina facilmente con un NullObject su Gem e mi sembra il caso di aggiungerlo ormai. quegli if non li ho mica scritti io :) non ho fatto altro che spostare quel metodo, e sono il primo ad essere felice dell'eliminazione di qualsivoglia if. per il NullObject ci risentiamo domani sera :D
3- gem.getGridObject().getType().isStone(). Regola di Demeter, ovvero cose come getThis().getItFromThere().getItActuallyHere().getWellItWasThere() sono orribili. La regola dice che nessun metodo dev'essere invocato su un oggetto ritornato da un altro oggetto. L'oggetto che usi deve avere quel metodo che invoca l'oggetto al suo interno. non sapevo questa regola, ora ho imparato qualcos'altro :O
comunque la mia ambizione va ben oltre: isStone e tutti gli isXxx che gli fanno compagnia in AbstractDroppableType se ne devono andare :read:
4- Ma in questo caso non possiamo aggiungere isStone() a Gem, quindi dobbiamo togliere quel comportamento basato su un getType() mascherato. Puoi farlo tu? scrivendo il post a mano a mano non avevo letto il 4 prima di rispondere al 3 :D
vale come sopra: domani sera. sorry :cry:
il motivo percui ho deciso di metterlo in Grid era perché quel metodo getGem che prima era nella action non faceva minimamente uso di ciò che gli forniva il "this" di turno tranne che per il metodo getGrid: lo chiamava e lavorava su ciò che otteneva dalla griglia; cioè lavorava sulla griglia; di conseguenza l'ho spostato. però giustamente a guardare tutti i controlli che fa direi che lavora più sulla gemma che sulla griglia, quindi potrei pensare al modo di dividerlo e/o spostarlo ancora.
Si', per favore, dividi e sposta.
quegli if non li ho mica scritti io :) non ho fatto altro che spostare quel metodo, e sono il primo ad essere felice dell'eliminazione di qualsivoglia if. per il NullObject ci risentiamo domani sera :D
La code ownership e' di tutti, quindi hai la responsabilita' di quegli if, come ce l'ho io, come chiunque altro. Se li vedi, hai la responsabilita' di toglierli, se non li togli e fai il commit, la responsabilita' e' anche tua ;)
non sapevo questa regola, ora ho imparato qualcos'altro :O
comunque la mia ambizione va ben oltre: isStone e tutti gli isXxx che gli fanno compagnia in AbstractDroppableType se ne devono andare :read:
Assolutamente si'. Inizia da questo.
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 :D
E nessuno se ne accorge mai.
Si infatti e per sta cagata pensano tutti che sia uber skilled!!!11oneone
per ora il massimo che posso fare è mettere un refactor this su getAllGemsSameOf. pardon... volevo dire su getCrushableByFlashAt :mc:
(a pancia piena adesso ragiono meglio)
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 secondo me la getDroppableList doveva essere una soluzione provvisoria da far usare solo alle Action per facilitare il loro inglobamento in Grid, ma poi ho visto che alle Action viene già automaticamente passata la lista di Droppable :D
In pratica le Action funzionavano a metà fra un Command e un Iterator, se non ho capito male.
Stasera prendo il codice delle Flash e sfodero il calcio rotante rovesciato di Ken Shiro vs Chuck Norris ed i predatori dell'Arca Perduta.
Infatti secondo me la getDroppableList doveva essere una soluzione provvisoria da far usare solo alle Action per facilitare il loro inglobamento in Grid, ma poi ho visto che alle Action viene già automaticamente passata la lista di Droppable :D
In pratica le Action funzionavano a metà fra un Command e un Iterator, se non ho capito male.
Infatti se implementiamo correttamente i pattern Command ed Iterator una volta messi insieme saranno un'arma di distruzione di massa :D
Mi piace. Rinomina solo checkSpritePosition() in assertSpritePosition() e poi spezza il test in due test separati, eventualmente nel loro TestCase.
Agli ordini capo!
Ma addirittura un'altro TestCase?? Io li ho spostati da TestGrid, dove erano prima, in TestGridDrawing perche' questo, o meglio questi 2 ;) , test riguardano come le gemme vengono disegnate dalla griglia.
Ora spezzo e rinomino, aspetto la conferma per creare il nuovo TestCase.
A proposito di iterator.... :mano: :mano:
Ora spezzo e rinomino, aspetto la conferma per creare il nuovo TestCase.
Si' per favore.
ok ;)
EDIT:
Fatto, anche se il nome del TestCase non mi convince.
Altra cosa, ho elimintao questo test:
public void testBottomBound()
{
grid.insertDroppable(gem, grid.getNumberOfRows() - 1, 1);
final Sprite sprite = gem.getAnimatedSprite().getSprite();
assertFalse("Gem moved beyond bottom bound!",
sprite.getBottom() - gridPosition.getY() > grid.getHeight());
}
Perche' a questo punto non so piu' cosa testa :wtf:
it.diamonds.tests.engine.video.AnimatedSprite:103
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());
}
Durante:
public void testUpdateWithThreeFrames()
{
final int ANIMATION_RATE = 100;
AnimatedSprite animatedSprite = createAnimatedSprite(3, ANIMATION_RATE);
animatedSprite.updateAnimation(ANIMATION_DELAY + 1);
assertEquals("current frame must be 1", 1,
animatedSprite.getCurrentFrame());
animatedSprite.updateAnimation(ANIMATION_DELAY + ANIMATION_RATE);
assertEquals("current frame must be 2", 2,
animatedSprite.getCurrentFrame());
}
Dopo:
public void testUpdateFirstOfThreeFrames()
{
AnimatedSprite animatedSprite = createAnimatedSprite(3, ANIMATION_RATE);
animatedSprite.updateAnimation(ANIMATION_DELAY + 1);
assertEquals("current frame must be 1", 1,
animatedSprite.getCurrentFrame());
}
public void testUpdateSecondOfThreeFrames()
{
AnimatedSprite animatedSprite = createAnimatedSprite(3, ANIMATION_RATE);
animatedSprite.updateAnimation(ANIMATION_DELAY + ANIMATION_RATE);
assertEquals("current frame must be 2", 2,
animatedSprite.getCurrentFrame());
}
Prima:
protected void applyOn(Droppable gem)
{
if(!gem.getGridObject().getType().isStone())
{
return;
}
if(gem.getAnimatedSprite().getCurrentFrame() < 5)
{
return;
}
turningStonesCounter++;
if(gem.getAnimatedSprite().getCurrentFrame() >= 7)
{
// TODO: REFACTOR THIS
turningStonesCounter--;
getGridElements().remove(gem);
Droppable newGem = gemFactory.create(GEM,
gem.getGridObject().getColor());
newGem.getFallingObject().drop();
getGrid().insertDroppable(newGem, gem.getRegion().getTopRow(),
gem.getRegion().getLeftColumn());
}
gem.update(timer.getTime());
}
Dopo:
protected void applyOn(Droppable gem)
{
if(!gem.getGridObject().getType().isStone() || !isStoneTransforming(gem))
{
return;
}
turningStonesCounter++;
if(isStoneTransformed(gem))
{
turningStonesCounter--;
turnStoneInToGem(gem);
}
gem.update(timer.getTime());
}
private void turnStoneInToGem(Droppable stone)
{
getGridElements().remove(stone);
Droppable gem = gemFactory.create(GEM, stone.getGridObject().getColor());
gem.getFallingObject().drop();
int row = stone.getRegion().getTopRow();
int column = stone.getRegion().getLeftColumn();
getGrid().insertDroppable(gem, row, column);
}
private boolean isStoneTransforming(Droppable stone)
{
int frame = stone.getAnimatedSprite().getCurrentFrame();
return frame >= 5 && frame < 7;
}
private boolean isStoneTransformed(Droppable stone)
{
int frame = stone.getAnimatedSprite().getCurrentFrame();
return frame >= 7;
}
Bonfo, i metodi isStoneTransforming e isStoneTransformed in realtà agiscono solamente sulla stone, non sulla action; io li sposterei in Stone rinominandoli rispettivamente isTransforming e isTransformed.
C'ho pensato pure io, ma, non so se l'hai notato, c'e' un cast di mezzo.
Io aspetteri di vedere come risolviamo il problema del getType e isXXX.
Se valutate che conviene farlo subito...
... cambialo subito, ma poi devi aggiungere i test :D :D
spianato il primo TODO: REFACTOR THIS di TestGameRestartOnGameOver
ho rinominato la classe di test in TestGameOnGameOver (visto che il metodo testava il gameOver a partire da loop).
Da:
public void testPassToGameOverState() throws IOException
{
// TODO: REFACTOR THIS
LayerManager oldLayerManager = loop.getLayerManager();
fillFourthColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
// verifico che il restart nn sia ancora stato fatto
assertEquals(oldLayerManager, loop.getLayerManager());
assertEquals(field1, loop.getPlayFieldOne());
assertEquals(field2, loop.getPlayFieldTwo());
environment.getTimer().advance(restartGameDelay - 2);
loop.doOneStep();
// verifico che il restart nn sia ancora stato fatto
assertEquals(oldLayerManager, loop.getLayerManager());
assertEquals(field1, loop.getPlayFieldOne());
assertEquals(field2, loop.getPlayFieldTwo());
environment.getTimer().advance(1);
loop.doOneStep();
assertNotSame("The layer must be different from the old one",
oldLayerManager, loop.getLayerManager());
// TODO manca test che � stato settato il background(come si fa?)
assertNotSame("field1 must be different from the old one", field1,
loop.getPlayFieldOne());
assertNotSame("field2 must be different from the old one", field2,
loop.getPlayFieldTwo());
}
A:
public void testPassToGameOverOnColumnFull() throws IOException
{
fillFourthColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
assertTrue(loop.getPlayFieldOne().getGridController().isGameOver());
}
public void testPassToGameOverNotRestartGame() throws IOException
{
fillFourthColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
assertEquals(layerManager, loop.getLayerManager());
assertEquals(field1, loop.getPlayFieldOne());
assertEquals(field2, loop.getPlayFieldTwo());
}
public void testDelayBeforeRestart() throws IOException
{
fillFourthColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
environment.getTimer().advance(restartGameDelay - 2);
loop.doOneStep();
assertEquals(layerManager, loop.getLayerManager());
assertEquals(field1, loop.getPlayFieldOne());
assertEquals(field2, loop.getPlayFieldTwo());
}
public void testRestartAfterDelay() throws IOException
{
fillFourthColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
environment.getTimer().advance(restartGameDelay-1);
loop.doOneStep();
assertNotSame("The layer must be different from the old one",
layerManager, loop.getLayerManager());
assertNotSame("field1 must be different from the old one", field1,
loop.getPlayFieldOne());
assertNotSame("field2 must be different from the old one", field2,
loop.getPlayFieldTwo());
}
non chiedetemi perchè basta far avanzare il timer di restartGameDelay(preso dal config) -1 per far scattare il gameOver, su questo non ho indagato, ma ricordo che "era così".
ho anche eliminato il todo che diceva che si doveva testare l'apparizione dell'immagine del game over, ma nn si sapeva come(ho la strana sensazione di averlo scritto io..fra l'altro....
public void testNotShowGameOverImageBeforeGameOver() throws IOException
{
Image gameOverImage = environment.getEngine().createImage(GameOverBox.texturePath);
assertFalse(mockEngine.wasImageDrawn(gameOverImage));
}
public void testShowGameOverImageOnGameOver() throws IOException
{
fillFourthColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
Image gameOverImage = environment.getEngine().createImage(GameOverBox.texturePath);
assertTrue(mockEngine.wasImageDrawn(gameOverImage));
}
public void testNotShowGameOverAfterRestart() throws IOException
{
fillFourthColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
environment.getTimer().advance(restartGameDelay-1);
loop.doOneStep();
Image gameOverImage = environment.getEngine().createImage(GameOverBox.texturePath);
assertFalse(mockEngine.wasImageDrawn(gameOverImage));
}
unici accorgimenti per questo test, è stato cambiare il metodo di MockEngine clearDisplay in
public void clearDisplay()
{
drawInfoList = new ArrayList<DrawInfo>();
numberOfQuadsDrawn = 0;
}
prima non veniva pulita la drawInfoList, per cui mi ritrovavo il gameover anche dopo il restart...
In piu ho messo public static final GameOverBox.texturePath, per avere accesso all'nome texture/immagine.
C'è un altro test che usa la tecnica environment.getEngine().createImage(NOME_TEXTURE) e che hanno il nome texture hardcoded nel test(come nel mio caso il nome texture era private static). Non so quale modo sia migliore...(anche se io voto mettere i nomi texture public static final)
ho anche eliminato il todo che diceva che si doveva testare l'apparizione dell'immagine del game over, ma nn si sapeva come(ho la strana sensazione di averlo scritto io..fra l'altro....
Sei sicuro che non ci sia gia' qualcos'altro di simile in giro??
Mi ricordo che avevamo trovato un doppione... controlla, altrimenti signifca che cosi' va benissimo ;)
Prima:
Dopo:
protected void applyOn(Droppable gem)
{
if(!gem.getGridObject().getType().isStone() || !isStoneTransforming(gem))
{
return;
}
turningStonesCounter++;
if(isStoneTransformed(gem))
{
turningStonesCounter--;
turnStoneInToGem(gem);
}
gem.update(timer.getTime());
}
private void turnStoneInToGem(Droppable stone)
{
getGridElements().remove(stone);
Droppable gem = gemFactory.create(GEM, stone.getGridObject().getColor());
gem.getFallingObject().drop();
int row = stone.getRegion().getTopRow();
int column = stone.getRegion().getLeftColumn();
getGrid().insertDroppable(gem, row, column);
}
private boolean isStoneTransforming(Droppable stone)
{
int frame = stone.getAnimatedSprite().getCurrentFrame();
return frame >= 5 && frame < 7;
}
private boolean isStoneTransformed(Droppable stone)
{
int frame = stone.getAnimatedSprite().getCurrentFrame();
return frame >= 7;
}
ho trovato un bug(vedi allegato)
non ne sono sicuro, ma potrebbe essere questa modifica a farlo saltare fuori.
ps.si lo so c'è un volontario per il test del bug...però se va bene domani
mattina..
pps.mancava l'allegato
Il bug penso di aver capito dov'è.
Prima con frame = 7 riusciva comunque a passare il primo if, ovvero
if(frame < 5 ) {
return;
}
mentre invece adesso ci casca dentro in pieno.
if(frame < 5 || frame >= 7) {
return;
}
Dovrei scrivere il test, ma vorrei mazzare un po' bigGem.
Spostiamo la discussione di questo nel thread dei Bug ;)
Perchè AbstractDroppable non si tiene una reference al DroppableDescription invece di usarlo solo per inizzializzare i membri??
Perchè AbstractDroppable non si tiene una reference al DroppableDescription invece di usarlo solo per inizzializzare i membri??
Perche' l'ho dimenticato :)
Se vedi una cosa cosi' semplice, fallo senza problemi. Ieri ho rifattorizzato 20 classi a mezzanotte, qualcosa puo' scappare.
Prima:
protected boolean canMoveButNotWithFullGravity(Grid grid)
{
// TODO: REFACTOR THIS
float gridBottom = grid.getRowUpperBound(grid.getNumberOfRows() - 1);
int numberOfRowsOverOne = region.getBottomRow() - region.getTopRow();
if (getSprite().getPosition().getY() + numberOfRowsOverOne
* Cell.SIZE_IN_PIXELS + ((float)grid.getActualGravity()) / 2 > gridBottom)
{
return true;
}
if (getSprite().getPosition().getY() + ((float)grid.getActualGravity())
/ 2 <= grid.getRowUpperBound(getRegion().getTopRow()))
{
return false;
}
Cell cell = new Cell(getRegion().getBottomRow() + 1, getRegion().getLeftColumn());
return grid.isDroppableAt(cell);
}
Dopo:
protected boolean canMoveButNotWithFullGravity(Grid grid)
{
float nextPositionY = getSprite().getPosition().getY() + grid.getActualGravity() * 0.5f;
float limit = grid.getRowUpperBound(grid.getNumberOfRows() - getRegion().getHeight());
if (nextPositionY > limit)
{
return true;
}
float currentRowLimit = grid.getRowUpperBound(getRegion().getTopRow());
if (nextPositionY <= currentRowLimit)
{
return false;
}
Cell cell = new Cell(getRegion().getBottomRow() + 1, getRegion().getLeftColumn());
return grid.isDroppableAt(cell);
}
Ricordo i due problemi, entrambi da testare.
- il supporto a gravita' che producano step solo <= 1 cella ( visibile nella costruzione della cella)
- il fatto che si controlli solo la 1 column, probabilemnte causa del bug 6.
^TiGeRShArK^
06-02-2008, 23:08
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!"
http://img137.imageshack.us/img137/6697/kenpq9.jpg
:eek:
cazz.. avevo in cameretta proprio quel poster...
ma la scritta non me la ricordavo.. :stordita:
:asd:
Comincio a lavorare sul refactoring della CrushByChestAction
EDIT: ho committato, anche se ho lasciato il REFACTOR THIS perchè c'è ancora lavoro da fare.
In particolare pensavo di mettere in Droppable un metodo getAdjacentDroppable che restituisse una DroppableList con le droppable adiacenti (e un flag se sono richieste le droppable dello stesso colore, oppure un altro metodo apposito). Con un metodo simile il codice della CrushByChestAction si dimezzerebbe e saremmo più vicini alla sua eliminazione. Si può andare in questa direzione?
Mi sono preso la libertà di aggiungere un paio di Refactor this :flower: :flower:
Poi ho aggiunto il metodo create() a MockSingleDroppable, che vedo sostiuirsi benissimo al createGem() di GridTestCase, poi, con il refactoring che ho fatto su BigGem, anche la loro creazione dovrebbe essere molto più semplice, facendo si che si possano eliminare tutti qui metodi strani da GridTestCase... che secondo me dimagrisce un bel po'. :D :D
Inoltre stavo pensando di valutare bene cosa stanno a fare li in nostri TestCase custom.
Ovvero a parte il GridTestCase che fa tutto, gli altri li vedo un po' "inutili", il che vuol dire 2 cose:
- o li stiamo usando male,
- o non servono a un granchè.
In ogni caso bisogna "omogeneizzare" i test ;)
Io un po' alla volta sto cercando di eliminare le dipendenze. Ad esempio è inutile usare GridTestCase se l'unica cosa che mi serve è un MockEngine.
Giusto, togli le dipendenze dai TestCase che non servono e aggiungi i Refactor This che reputi opportuni.
Uhmmm, ieri sera il forum è andato in palla e non mi ha più modificato il post...
Dunque: ho committato il lavoro sulla CrushByChestAction ma si può fare ancora qualcosa: pensavo di aggiungere un metodo getAdjacentDroppables ai vari Droppables (implementato magari in AbstractDroppable e in BigGem) che restituisca tutte le gemme adiacenti (ovvero con un lato in comune) di qualsiasi colore (pensavo con un flag che indichi se devono o meno essere dello stesso colore, oppure creare un altro metodo apposito). Questo semplificherebbe notevolmente la Action e ci metterebbe più vicini alla sua eliminazione.
Posso procedere in quella direzione?
Uhmmm, ieri sera il forum è andato in palla e non mi ha più modificato il post...
Dunque: ho committato il lavoro sulla CrushByChestAction ma si può fare ancora qualcosa: pensavo di aggiungere un metodo getAdjacentDroppables ai vari Droppables (implementato magari in AbstractDroppable e in BigGem) che restituisca tutte le gemme adiacenti (ovvero con un lato in comune) di qualsiasi colore (pensavo con un flag che indichi se devono o meno essere dello stesso colore, oppure creare un altro metodo apposito). Questo semplificherebbe notevolmente la Action e ci metterebbe più vicini alla sua eliminazione.
Posso procedere in quella direzione?
Flag?! Flag che modifica il comportamento di un metodo?!?!?! FLAG?!?!?!
Chiedo scusa, volevo dire due metodi separati :D
ho una domanda circa la regola 1 commit ==> 1 refactor this che se ne va: è possibile committare aggiungendo dei refactor this nel codice ma senza risolverne nessuno? oppure per committare qualunque cosa (anche l'aggiunta di nuovi refactor this) bisogna per forza risolverne almeno uno?
ho una domanda circa la regola 1 commit ==> 1 refactor this che se ne va: è possibile committare aggiungendo dei refactor this nel codice ma senza risolverne nessuno? oppure per committare qualunque cosa (anche l'aggiunta di nuovi refactor this) bisogna per forza risolverne almeno uno?
La seconda che hai detto.
ok.
ora sto lavorando sul refactor this in it.diamonds.grid.action.CrushByChestAction:11
ot: ho notato solo oggi l'immagine di ken shiro in prima pagina :asd:
credo che abbiamo trovato un'ispirazione per il codename della prossima release :rotfl:
(il codename precedente era YAGNI :D)
from
public void testNotMoveRightOnRestartWaitState() throws IOException
{
// TODO: REFACTOR THIS
Droppable slaveGem;
Droppable pivotGem;
fillFourthColumn();
loop.getPlayerTwoInput().notify(Event.create(Code.RIGHT, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.DOWN, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.BUTTON1, State.PRESSED));
environment.getTimer().advance(newGemDelay);
slaveGem = field2.getGridController().getGemsPair().getSlave();
int slaveRow = slaveGem.getRegion().getTopRow();
int slaveColumn = slaveGem.getRegion().getLeftColumn();
pivotGem = field2.getGridController().getGemsPair().getPivot();
int pivotRow = pivotGem.getRegion().getTopRow();
int pivotColumn = pivotGem.getRegion().getLeftColumn();
environment.getTimer().advance(restartGameDelay - 1);
loop.doOneStep();
assertEquals(field2.getGridController().getGemsPair().getSlave().getRegion().getLeftColumn(), slaveColumn);
// slaveRow +1 perche la gravit� sulla gemsPair viene testata prima del check
// del
// gameOver(in gridController.update)
assertEquals(field2.getGridController().getGemsPair().getSlave().getRegion().getTopRow(), slaveRow + 1);
assertEquals(field2.getGridController().getGemsPair().getPivot().getRegion().getLeftColumn(), pivotColumn);
// slaveRow +1 perche la gravit� sulla gemsPair viene testata prima del check
// del
// gameOver(in gridController.update)
assertEquals(field2.getGridController().getGemsPair().getPivot().getRegion().getTopRow(), pivotRow + 1);
}
to
public void testGemsPairOnFieldTwoMoveWhenGameOverOccour() throws IOException
{
Droppable slaveGem = field2.getGridController().getGemsPair().getSlave();
Droppable pivotGem = field2.getGridController().getGemsPair().getPivot();
fillFourthColumn();
int slaveRow = slaveGem.getRegion().getTopRow();
int slaveColumn = slaveGem.getRegion().getLeftColumn();
int pivotRow = pivotGem.getRegion().getTopRow();
int pivotColumn = pivotGem.getRegion().getLeftColumn();
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
assertEquals(field2.getGridController().getGemsPair().getSlave().getRegion().getLeftColumn(), slaveColumn);
assertEquals(field2.getGridController().getGemsPair().getSlave().getRegion().getTopRow(), slaveRow + 1);
assertEquals(field2.getGridController().getGemsPair().getPivot().getRegion().getLeftColumn(), pivotColumn);
assertEquals(field2.getGridController().getGemsPair().getPivot().getRegion().getTopRow(), pivotRow + 1);
}
public void testGemsPairOnFieldTwoNotMoveOnWaitRestart() throws IOException
{
Droppable slaveGem = field2.getGridController().getGemsPair().getSlave();
Droppable pivotGem = field2.getGridController().getGemsPair().getPivot();
fillFourthColumn();
loop.getPlayerTwoInput().notify(Event.create(Code.RIGHT, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.DOWN, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.BUTTON1, State.PRESSED));
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
Region slaveRegion = cloneRegion(slaveGem.getRegion());
Region pivotRegion = cloneRegion(pivotGem.getRegion());
environment.getTimer().advance(restartGameDelay - 1);
loop.doOneStep();
assertEquals(slaveGem.getRegion(), slaveRegion);
assertEquals(pivotGem.getRegion(), pivotRegion);
}
notare la creazione in ComponentHelperForTest di cloneRegion per evitare di tirarsi fuori top e left ogni volta e confrontarle una per una..
from
public void testControllerDontReactForKeyPressedOnDelay() throws IOException
{
// TODO: REFACTOR THIS
int inputRate = environment.getConfig().getInteger("InputRate");
fillFourthColumn();
loop.getPlayerTwoInput().notify(Event.create(Code.RIGHT, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.DOWN, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.BUTTON1, State.PRESSED));
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
environment.getTimer().advance(restartGameDelay);
loop.doOneStep();
environment.getTimer().advance(inputRate);
loop.doOneStep();
DroppablePair newGemsPairOne = loop.getPlayFieldOne().getGridController().getGemsPair();
DroppablePair newGemsPairTwo = loop.getPlayFieldTwo().getGridController().getGemsPair();
assertEquals("New pivot of field One is misplaced", 4, newGemsPairOne.getPivot().getRegion().getLeftColumn());
assertEquals("New pivot of field One is misplaced", 2, newGemsPairOne.getPivot().getRegion().getTopRow());
assertEquals("New slave of field One is misplaced", 4, newGemsPairOne.getSlave().getRegion().getLeftColumn());
assertEquals("New slave of field One is misplaced", 1, newGemsPairOne.getSlave().getRegion().getTopRow());
assertEquals("New pivot of field Two is misplaced", 4, newGemsPairTwo.getPivot().getRegion().getLeftColumn());
assertEquals("New pivot of field Two is misplaced", 2, newGemsPairTwo.getPivot().getRegion().getTopRow());
assertEquals("New slave of field Two is misplaced", 4, newGemsPairTwo.getSlave().getRegion().getLeftColumn());
assertEquals("New slave of field Two is misplaced", 1, newGemsPairTwo.getSlave().getRegion().getTopRow());
}
to
public void testControllerDontReactForKeyPressedOnDelay() throws IOException
{
int inputRate = environment.getConfig().getInteger("InputRate");
fillFourthColumn();
loop.getPlayerTwoInput().notify(Event.create(Code.RIGHT, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.DOWN, State.PRESSED));
loop.getPlayerTwoInput().notify(Event.create(Code.BUTTON1, State.PRESSED));
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
environment.getTimer().advance(restartGameDelay);
loop.doOneStep();
environment.getTimer().advance(inputRate);
loop.doOneStep();
DroppablePair newGemsPairOne = loop.getPlayFieldOne().getGridController().getGemsPair();
DroppablePair newGemsPairTwo = loop.getPlayFieldTwo().getGridController().getGemsPair();
assertEquals("New pivot of field One is misplaced", PIVOT_START_REGION, newGemsPairOne.getPivot().getRegion());
assertEquals("New slave of field One is misplaced", SLAVE_START_REGION, newGemsPairOne.getSlave().getRegion());
assertEquals("New pivot of field Two is misplaced", PIVOT_START_REGION, newGemsPairTwo.getPivot().getRegion());
assertEquals("New slave of field Two is misplaced", SLAVE_START_REGION, newGemsPairTwo.getSlave().getRegion());
}
qua non ho potuto fare molto...oltre che creare in ComponentHelperForTest PIVOT_START_REGION e SLAVE_START_REGION per semplificare gli assert. Quando arrivo a lavoro metto in config i punti di partenza della slave e della pivot (come da TODO) e poi committo
Si ma i test non devono assolutamente dipendere dai valori nel config..
Si ma i test non devono assolutamente dipendere dai valori nel config..
non sono d'accordo. Si rendono i test più resistenti al cambiamento. Domani cambiamo il tempo di reactToInput e se i valori sono hardcoded tutto si rompe. Usando i valori nel config invece tutto continua a funzionare(o almeno si rompe meno roba).
Poi si potrebbe anche fare un test per testare solo che la gems pair sia in una hardcoded posizione di partenza, ma secondo me è eccessivo.
ps. i valori da config per la posizione della gems pair li ho utilizzati solo in quel test. Se poi si vuol portare l'idea anche il altri test si può fare.
pps. dove si pensa che un test dipenda fortemente dal comportamento di default del config(il test ha senso solo se la gems pair è sulla colonna 4) allora si possono usare valori hardcoded nel test
Ah ok pensavo dicessi test tipo:
int updateRate = config.getInteger("updateRate");
assert(updateRate == 300);
Se e` per rendere i test a prova di cambiamento sono d'accordissimo
Ah ok pensavo dicessi test tipo:
int updateRate = config.getInteger("updateRate");
assert(updateRate == 300);
Se e` per rendere i test a prova di cambiamento sono d'accordissimo
ah ok, sono anche io contro quel tipo di test :)
Due cosine a cui vorrei che deste un occhiata perché mi hanno confuso non poco.
Prima di tutto non sarebbe meglio limitare l'argomento di ScoreCalculator.closeChain() a valori tra 2 e 10? In questo momento è possibile passargli qualsiasi cosa ci passi per la mente e la funzione è persino usata per resettare il tempScore usando 0.
Negli ultimi due test di TestGemCollisionSound il valore per la gravità presente nella configurazione viene moltiplicato per 2. Togliendo questa moltiplicazione i test passano comunque, quindi mi chiedevo a cosa serve? C'è qualche ragione più profonda che ho mancato?
^TiGeRShArK^
08-02-2008, 21:36
sono finiti tutti i refactor this? :stordita:
ho fatto l'update alla head e non ne vedo...
che posso fare di breve stasera che posso? :O
Forse ci sono i "REFACTOR THIS" che non hai visto? :stordita:
In ogni caso se vai in Windows->showView->Tasks ti trovi tutti i TODO saprsi per Diamonds ;)
^TiGeRShArK^
08-02-2008, 21:56
Forse ci sono i "REFACTOR THIS" che non hai visto? :stordita:
In ogni caso se vai in Windows->showView->Tasks ti trovi tutti i TODO saprsi per Diamonds ;)
no, ho guardato proprio in tasks :D
lo so dove si trovano i TODO :D
..però non ne vedo REFACTOR THIS... :stordita:
P.S. mi da 26 items in tasks per la precisione....
no, ho guardato proprio in tasks :D
lo so dove si trovano i TODO :D
..però non ne vedo REFACTOR THIS... :stordita:
P.S. mi da 26 items in tasks per la precisione....
Io ne ho 41, di cui 11 sono REFACTOR THIS.
^TiGeRShArK^
08-02-2008, 22:16
Io ne ho 41, di cui 11 sono REFACTOR THIS.
:mbe:
sto rifacendo l'update....
e mi sta riupdatando tutto ora..
non ci sto capendo + niente.. :fagiano:
P.S. il primo update l'avevo fatto con tortoise e ora questo lo sto facendo con subclipse..
^TiGeRShArK^
08-02-2008, 22:20
ok ci siamo..
41 todo e 11 refactor this..
sarà stato tortoise ad impallarsi :muro:
EDIT:
scoperto il motivo :asd:
in realtà ho due copie di diamonds una si I: e una in i:\workspace\eclipse :asd:
^TiGeRShArK^
08-02-2008, 22:25
Prima:
// TODO: REFACTOR THIS!!! I smell a bad test. Insanely long name and 5 assert.
public void testRepeatitionActivatedAndHandlerExecutedOnInputReaction()
{
setupInput();
input.notify(Event.create(Code.ENTER, State.PRESSED));
reactor.reactToInput(environment.getTimer().getTime());
assertFalse(handler.isRepeated(0));
environment.getTimer().advance(201);
handler.resetStatus();
reactor.reactToInput(environment.getTimer().getTime());
assertTrue(handler.isRepeated(0));
assertEquals(handler.getFastRepeatDelay(), handler.getCurrentRepeatDelay());
assertTrue(handler.executed());
assertTrue(handler.pressed());
}
^TiGeRShArK^
08-02-2008, 23:24
rettifico..
Ho dovuto sistemare TUTTO dato che era qualcosa di trascendentale.
Prima:
private void setupInput()
{
input = Input.create(environment.getKeyboard(), environment.getTimer());
input.setEventMappings(createEventMappings());
reactor = new InputReactor(input, 200, 100);
reactor.addHandler(Code.ENTER, handler);
}
public void setUp()
{
environment = MockEnvironment.create();
handler = MockEventHandler.create();
}
public void testNormalRepeatDelaySet()
{
handler.setNormalRepeatDelay(100);
assertEquals(100, handler.getNormalRepeatDelay());
}
public void testFastRepeatDelaySet()
{
handler.setFastRepeatDelay(100);
assertEquals(100, handler.getFastRepeatDelay());
}
public void testPressed()
{
setupInput();
handler.handleEvent(reactor, Event.create(Code.ENTER, State.PRESSED));
assertTrue(handler.isPressed());
}
public void testReleased()
{
setupInput();
handler.handleEvent(reactor, Event.create(Code.ENTER, State.PRESSED));
handler.handleEvent(reactor, Event.create(Code.ENTER, State.RELEASED));
assertFalse(handler.isPressed());
}
public void testRepeatitionOnTimeDelay()
{
setupInput();
input.notify(Event.create(Code.ENTER, State.PRESSED));
reactor.reactToInput(environment.getTimer().getTime());
assertFalse(handler.isRepeated(0)); // TODO: 3 assert? why?
assertTrue(handler.isRepeated(201));
assertEquals(handler.getNormalRepeatDelay(), handler.getCurrentRepeatDelay());
}
// TODO: REFACTOR THIS!!! I smell a bad test. Insanely long name and 5 assert.
public void testRepeatitionActivatedAndHandlerExecutedOnInputReaction()
{
setupInput();
input.notify(Event.create(Code.ENTER, State.PRESSED));
reactor.reactToInput(environment.getTimer().getTime());
assertFalse(handler.isRepeated(0));
environment.getTimer().advance(201);
handler.resetStatus();
reactor.reactToInput(environment.getTimer().getTime());
assertTrue(handler.isRepeated(0));
assertEquals(handler.getFastRepeatDelay(), handler.getCurrentRepeatDelay());
assertTrue(handler.executed());
assertTrue(handler.pressed());
}
// TODO: Why all this assert?
public void testRepeatitionDisactivatedOnEventReleased()
{
setupInput();
input.notify(Event.create(Code.ENTER, State.PRESSED));
reactor.reactToInput(environment.getTimer().getTime());
assertFalse(handler.isRepeated(0));
environment.getTimer().advance(201);
reactor.reactToInput(environment.getTimer().getTime());
assertTrue(handler.isRepeated(0));
environment.getTimer().advance(201);
input.notify(Event.create(Code.ENTER, State.RELEASED));
reactor.reactToInput(environment.getTimer().getTime());
assertFalse(handler.isRepeated(0));
}
}
Dopo il colpo dell'uccello d'acqua di nanto:
public void setUp()
{
environment = MockEnvironment.create();
int normalRepeatDelay = environment.getConfig().getInteger("NormalRepeatDelay");
int fastRepeatDelay = environment.getConfig().getInteger("FastRepeatDelay");
timer = environment.getTimer();
handler = MockEventHandler.create();
input = Input.create(environment.getKeyboard(), environment.getTimer());
input.setEventMappings(createEventMappings());
reactor = new InputReactor(input, normalRepeatDelay, fastRepeatDelay);
reactor.addHandler(Code.ENTER, handler);
input.notify(Event.create(Code.ENTER, State.PRESSED));
}
public void testNormalRepeatDelaySet()
{
handler.setNormalRepeatDelay(100);
assertEquals(100, handler.getNormalRepeatDelay());
}
public void testFastRepeatDelaySet()
{
handler.setFastRepeatDelay(100);
assertEquals(100, handler.getFastRepeatDelay());
}
public void testPressed()
{
handler.handleEvent(reactor, Event.create(Code.ENTER, State.PRESSED));
assertTrue(handler.isPressed());
}
public void testReleased()
{
handler.handleEvent(reactor, Event.create(Code.ENTER, State.PRESSED));
handler.handleEvent(reactor, Event.create(Code.ENTER, State.RELEASED));
assertFalse(handler.isPressed());
}
public void testHandlerNotRepeated()
{
reactToInput();
assertFalse(handler.isRepeated(0));
}
private void reactToInput()
{
reactor.reactToInput(timer.getTime());
}
public void testHandlerIsRepeatedAfterDelay()
{
reactToInput();
assertTrue(handler.isRepeated(handler.getNormalRepeatDelay() + 1));
}
public void testRepeatDelayIsNotChanging()
{
reactToInput();
assertEquals(handler.getNormalRepeatDelay(), handler.getCurrentRepeatDelay());
}
public void testHandlerIsRepeatedAfterTwoInputReaction()
{
reactTwiceToInput();
assertTrue(handler.isRepeated(0));
}
public void testFastRepetitionSetAfterTwoInputReaction()
{
reactTwiceToInput();
assertEquals(handler.getFastRepeatDelay(), handler.getCurrentRepeatDelay());
}
public void testHandlerExecutedAfterTwoInputReaction()
{
reactTwiceToInput();
assertTrue(handler.executed());
}
public void testHandlerPressedAfterTwoInputReaction()
{
reactTwiceToInput();
assertTrue(handler.pressed());
}
public void testRepetitionDeactivatedWhenReleased()
{
reactTwiceToInput();
timer.advance(handler.getNormalRepeatDelay() + 1);
input.notify(Event.create(Code.ENTER, State.RELEASED));
reactToInput();
assertFalse(handler.isRepeated(0));
}
private void reactTwiceToInput()
{
reactToInput();
timer.advance(handler.getNormalRepeatDelay() + 1);
reactToInput();
}
}
ebbene si :O
2000 anni di scuola Hokuto ci hanno insegnato la vittoria del refactoring sul bad design e sui bug :cool:
:asd:
Ottimo lavoro :) ma perche il setUp e` diventato piu` incasinato?
In CanMoveDownQuery ho trovato questo codice
protected void applyOn(Droppable gem)
{
if (pair != null)
{
if (gem == pair.getPivot() || gem == pair.getSlave())
{
return;
}
}
result |= gem.getMovingDownObject().canMoveDown(getGrid());
}
Anche questo direi che e` un rimasuglio delle dynamite. Cancello l'if. Se avete qualcosa in contrario dite pure :)
protected void applyOn(Droppable gem)
{
result |= gem.getMovingDownObject().canMoveDown(getGrid());
}
Rimuovo i due test che falliscono che testano appunto questo comportamento (tral'altro in maniera oscura)
^TiGeRShArK^
08-02-2008, 23:57
Ottimo lavoro :) ma perche il setUp e` diventato piu` incasinato?
perchè ho aggiunto lì tutto quello che era ripetuto nei vari metodi :asd:
EDIT: volendo si potrebbe reintrodurre l'extract method con setupInput... fai pure se ti va :p
perchè ho aggiunto lì tutto quello che era ripetuto nei vari metodi :asd:
EDIT: volendo si potrebbe reintrodurre l'extract method con setupInput... fai pure se ti va :p
Si' per favore :)
ho committare il refactor this del mio precedente post....
ho però aggiunto un todo a questo test
//TODO This test check a behavior not necessary for the game. Remove it?
public void testGemsPairOnFieldTwoMoveWhenGameOverOccour() throws IOException
{
Droppable slaveGem = field2.getGridController().getGemsPair().getSlave();
Droppable pivotGem = field2.getGridController().getGemsPair().getPivot();
fillFourthColumn();
Region slaveNextRegion = cloneRegion(slaveGem.getRegion(), 0, 1);
Region pivotNextRegion = cloneRegion(pivotGem.getRegion(), 0, 1);
environment.getTimer().advance(newGemDelay);
loop.doOneStep();
assertEquals(field2.getGridController().getGemsPair().getSlave().getRegion(), slaveNextRegion);
assertEquals(field2.getGridController().getGemsPair().getPivot().getRegion(), pivotNextRegion);
}
praticamente questo test verifica un comportamento di diamonds, che però non è una specifica.
Praticamente quando capita il gameover sul field1 la pair sul field2 fà un movimento verso il basso.
Questo perchè viene fatto un check se cè gameover, poi viene fatto l'update sul field 1, viene generato il gameover, poi viene fatto l'update del field2(e la pair si muove verso il basso). Al giro successivo si troverà il gameover, e la pair non si muove più(verificato da un altro test).
Questo test è da tenere?
ps.C'è un simpatico plugin per eclipse di checkstyle. Una volta caricato il file di configurazione di checkstyle, e abilitato nel progetto di diamonds, vi ritroverete i warning in eclipse. Consiglio comunque sempre una build di ant prima di committare. C'è solo una modifica da fare dopo aver caricato il file, andare in Class design->Final class e deselezionare Hide Utility Class Constructor, che genera dei warning nel codice non segnalati dal task ant(non so se dipende da diverse versioni di checkstyle).
Puoi controllare con Jocchan se questo comportamento e' necessario o meno?
Se non e' necessario rimuovi pure il test e il codice di produzione relativo.
Hai un link del plugin di checkstyle?
eclipse-cs.sourceforge.net
non cè nessun codice di produzione relativo. Semplicemente per come è scritto l'update delle griglie succede che in caso di gameover su un field, l'altro non si fermi subito(e sarebbe impossibile evitarlo, a meno di simulare prima su entrambi i field se avviene un gameover).
Questo comportamento è errato, non appena scatta il game over dovrebbero fermarsi entrambe le griglie.
Direi di prendere il tutto a calci rotanti.
^TiGeRShArK^
09-02-2008, 10:49
Si' per favore :)
fatto :O
public void setUp()
{
Environment environment = MockEnvironment.create();
initFields(environment);
initReactor(environment);
input.setEventMappings(createEventMappings());
input.notify(Event.create(Code.ENTER, State.PRESSED));
}
private void initReactor(Environment environment)
{
int normalRepeatDelay = environment.getConfig().getInteger("NormalRepeatDelay");
int fastRepeatDelay = environment.getConfig().getInteger("FastRepeatDelay");
reactor = new InputReactor(input, normalRepeatDelay, fastRepeatDelay);
reactor.addHandler(Code.ENTER, handler);
}
private void initFields(Environment environment)
{
timer = environment.getTimer();
handler = MockEventHandler.create();
input = Input.create(environment.getKeyboard(), environment.getTimer());
}
Questo comportamento è errato, non appena scatta il game over dovrebbero fermarsi entrambe le griglie.
Direi di prendere il tutto a calci rotanti.
è un problema complicato.
Ora come ora abbiamo 2 occasioni in cui si può verificare un gameover:
-Stiamo aspettando che venga fuori una nuova gemsPair e la colonna centrale è piena
-Scendono le stone e ci ritroviamo la colonna centrale piena
Il secondo caso si potrebbe ridurre al primo, visto che dopo la caduta delle stone si va nello stato in cui aspettiamo che venga fuori una nuova gemspair(ho fatto la modifica e fallisce solo un test locale allo stato in cui scendono le stone, il che è normale).
Ora provo un po a giocare col secondo caso ridotto al primo a vedere se ci sono problemi grossi.
Questo comportamento è errato, non appena scatta il game over dovrebbero fermarsi entrambe le griglie.
Direi di prendere il tutto a calci rotanti.
Forse mi sono spiegato male.
Mettiamo che siamo al tempo t1. Devono essere aggiornate la griglia 1 e la griglia 2.
Nel t1 la griglia1 va in gameover. Nel tempo t1 viene fatto l'update della griglia 2 e la gemspair scende.
Al t11 viene verificato che il gioco è in gameover e tutte e 2 le griglie non si muovono.
Questo comportamente è difficilmente eliminabile, perchè anche se aggiornassi la griglia 1 nel t1, e poi controllassi che se è andata in gameover per poi bloccare la griglia 2 non risolverebbe il problema. Infatti a parti invertite il problema si ripresenterebbe.
L'unico sistema che mi viene in mente, sarebbe quello di simulare il t1 sia sulla griglia 1 che sulla griglia 2, e poi vedere se una ha generato gameover. Ma mi sembra una complicazione assurda.
Secondo me spiegata cosi la situazione è accettabile.
Questo comportamento è errato, non appena scatta il game over dovrebbero fermarsi entrambe le griglie.
Direi di prendere il tutto a calci rotanti.
Perfetto, serve un bug e il relativo test che lo esercita.
Forse mi sono spiegato male.
Mettiamo che siamo al tempo t1. Devono essere aggiornate la griglia 1 e la griglia 2.
Nel t1 la griglia1 va in gameover. Nel tempo t1 viene fatto l'update della griglia 2 e la gemspair scende.
Al t11 viene verificato che il gioco è in gameover e tutte e 2 le griglie non si muovono.
Questo comportamente è difficilmente eliminabile, perchè anche se aggiornassi la griglia 1 nel t1, e poi controllassi che se è andata in gameover per poi bloccare la griglia 2 non risolverebbe il problema. Infatti a parti invertite il problema si ripresenterebbe.
L'unico sistema che mi viene in mente, sarebbe quello di simulare il t1 sia sulla griglia 1 che sulla griglia 2, e poi vedere se una ha generato gameover. Ma mi sembra una complicazione assurda.
Secondo me spiegata cosi la situazione è accettabile.
Perche' non calcoliamo il gameover alla fine del turno dopo che entrambe le griglie sono state aggiornate? Oppure all'inizio del turno precedente. Dovrebbe automaticamente risolvere il problema. Joc, che dici?
è un problema complicato.
Ora come ora abbiamo 2 occasioni in cui si può verificare un gameover:
-Stiamo aspettando che venga fuori una nuova gemsPair e la colonna centrale è piena
-Scendono le stone e ci ritroviamo la colonna centrale piena
Il secondo caso si potrebbe ridurre al primo, visto che dopo la caduta delle stone si va nello stato in cui aspettiamo che venga fuori una nuova gemspair(ho fatto la modifica e fallisce solo un test locale allo stato in cui scendono le stone, il che è normale).
Ora provo un po a giocare col secondo caso ridotto al primo a vedere se ci sono problemi grossi.
questa considerazione merita una menzione a prescindere se il problema riportato è veramente da considerarsi tardi.
Ora come ora succede questo:
al t1 aggiorno la griglia 1 che viene riempita di stone. Se la colonna centrale è piena la griglia va in gameover. Poi aggiorna la griglia 2. Al giro successivo tutto è bloccato.
Si potrebbe eliminare questo controllo(ridurre il secondo caso al primo) e si avrebbe questa situazione:
al t1 aggiorno la griglia 1 che viene riempita di stone. Poi aggiorna la griglia 2. Nei prossimi tx la griglia 1 aspetta che sia passato abbastanza tempo per generare la nuova gemspair(e intanto la griglia 2 va avanti). Quanto arriva il tx in cui la nuova gemspair della griglia 1 può essere generata, allora si ha il gameover della griglia 1.
La situazione è differente, perchè in un caso(la caduta di stone) viene dato game over quando la colonna centrale è piena, mentre in tutti gli altri(che per ora possono essere solo la banale colonna di gemspair se nn vado errato) il game over viene dato quando la nuova gemspair viene generata(e si verifica che non si può inserire).
Forse sono un po seghe mentali, però volevo fare un quadro preciso della situazione(sempre che abbia capito come gira il tutto :mc: )
Perche' non calcoliamo il gameover alla fine del turno dopo che entrambe le griglie sono state aggiornate? Oppure all'inizio del turno precedente. Dovrebbe automaticamente risolvere il problema. Joc, che dici?
Dico che mi sembra la cosa più sensata. Funzionerebbe?
questa considerazione merita una menzione a prescindere se il problema riportato è veramente da considerarsi tardi.
Ora come ora succede questo:
al t1 aggiorno la griglia 1 che viene riempita di stone. Se la colonna centrale è piena la griglia va in gameover. Poi aggiorna la griglia 2. Al giro successivo tutto è bloccato.
Si potrebbe eliminare questo controllo(ridurre il secondo caso al primo) e si avrebbe questa situazione:
al t1 aggiorno la griglia 1 che viene riempita di stone. Poi aggiorna la griglia 2. Nei prossimi tx la griglia 1 aspetta che sia passato abbastanza tempo per generare la nuova gemspair(e intanto la griglia 2 va avanti). Quanto arriva il tx in cui la nuova gemspair della griglia 1 può essere generata, allora si ha il gameover della griglia 1.
La situazione è differente, perchè in un caso(la caduta di stone) viene dato game over quando la colonna centrale è piena, mentre in tutti gli altri(che per ora possono essere solo la banale colonna di gemspair se nn vado errato) il game over viene dato quando la nuova gemspair viene generata(e si verifica che non si può inserire).
Forse sono un po seghe mentali, però volevo fare un quadro preciso della situazione(sempre che abbia capito come gira il tutto :mc: )
Non conta tanto l'istante esatto in cui il game over viene generato, purchè però - una volta che una griglia va in game over - anche l'altra venga bloccata immediatamente. Dico questo perchè, anche se si tratta di un solo turno, potremmo avere situazioni inconsistenti.
Se troviamo un modo per farlo in maniera semplice, allora lo segno come bug e vediamo di risolverlo... altrimenti, se dovesse richiedere per forza dei refactoring mastodontici lo farò slittare indietro nella lista delle nostre priorità ;)
Perche' non calcoliamo il gameover alla fine del turno dopo che entrambe le griglie sono state aggiornate? Oppure all'inizio del turno precedente. Dovrebbe automaticamente risolvere il problema. Joc, che dici?
è quello che succede adesso.
if (!gameOver)
{
playFieldOne.update(loopTimestamp);
playFieldTwo.update(loopTimestamp);
checkAndShowGameOverMessage(playFieldOne);
checkAndShowGameOverMessage(playFieldTwo);
}
il problema (che secondo me non è un problema, come da altro post) e che se la prima da gameover, la seconda comunque fa la update. E mettere il checkGameover in mezzzo alle 2 update risolve il problema solo da un "lato"
E che mi ero spiegato male nel primo post...
Ci ho pensato, e lo segno come bug di livello D.
Si tratta di un delay di un solo update, e quindi assolutamente impercettibile per il giocatore (parliamo di un ritardo dell'ordine del centesimo di secondo, se non erro).
Se si trova un modo per risolverlo facilmente, allora provvediamo... ma scatenare altri refactoring complessi nella situazione attuale sarebbe decisamente sproporzionato al problema.
Quindi, teniamo presente la sua esistenza ma al momento - sempre ovviamente a meno che non sia poi così complicato risolverlo - NON ce ne occupiamo.
EDIT: alla fine dei conti non serve complicare i livelli dei bug, mi limito a specificare meglio un paio di dettagli.
jappilas
09-02-2008, 11:52
Non conta tanto l'istante esatto in cui il game over viene generato, purchè però - una volta che una griglia va in game over - anche l'altra venga bloccata immediatamente.
il problema (che secondo me non è un problema, come da altro post) e che se la prima da gameover, la seconda comunque fa la update. E mettere il checkGameover in mezzzo alle 2 update risolve il problema solo da un "lato"
E che mi ero spiegato male nel primo post...che la seconda faccia la update è giusto - in fondo il controllo è effettuato sequenzialmente sulle due griglie, e non prevedere che anche la seconda griglia possa trovarsi nella stessa situazione ( e potenzialmente generare il gameover allo stesso ciclo della prima, instaurando una situazione di parità ) imho toglierebbe fairness al gameplay
per questo, tempo fa pensavo a un protocollo di controllo del gameover basato sui timestamp (ogni grid terrebbe conto del tempo trascorso dall' inizio della partita "dalla propria parte") e una variante dell handshake a 3 vie ( la prima griglia in gameover avrebbe notificato all' altra comunicandole il deltaT, la seconda avrebbe dato l' OK al gameover dopo un tempo pari alla differenza tra il deltaT ricevuto e il proprio - se in quel tempo avesse rilevato a propria volta il gameover si sarebbe avuta la parità)
ma in effetti mi rendo conto che basterebbe assicurare che entrambe le Grid siano passate attraverso lo stesso numero di cicli, quindi un contatore intero...
Per la gestione dei game over contemporanei ci sarà bisogno di un qualche task, visto che non basta cambiare il comportamento attuale (si dovrà gestire correttamente il caso di pareggio). E' per questo che per ora è meglio non toccare nulla ;)
jappilas
09-02-2008, 12:06
Per la gestione dei game over contemporanei ci sarà bisogno di un qualche task, visto che non basta cambiare il comportamento attuale (si dovrà gestire correttamente il caso di pareggio).infatti... peraltro sarebbe anche un task grafico visto che quantomeno occorreranno delle nuove texture per i messaggi di vittoria / sconfitta / pareggio :p
E' per questo che per ora è meglio non toccare nulla ;)certo, ma è comunque importante analizzare il problema per vedere quando affrontarlo :)
infatti... peraltro sarebbe anche un task grafico visto che quantomeno occorreranno delle nuove texture per i messaggi di vittoria / sconfitta / pareggio :p
E non solo quello, ci sono diversi ritocchi da fare prima della release finale.
certo, ma è comunque importante analizzare il problema per vedere quando affrontarlo :)
Sì, ed è per questo che va tenuto presente (e infatti è nella lista dei bug).
Non conta tanto l'istante esatto in cui il game over viene generato, purchè però - una volta che una griglia va in game over - anche l'altra venga bloccata immediatamente. Dico questo perchè, anche se si tratta di un solo turno, potremmo avere situazioni inconsistenti.
Se troviamo un modo per farlo in maniera semplice, allora lo segno come bug e vediamo di risolverlo... altrimenti, se dovesse richiedere per forza dei refactoring mastodontici lo farò slittare indietro nella lista delle nostre priorità ;)
va stabilito bene, se è gameover quando la colonna centrale si riempie(aka ogni volta che si è inserito una o piu gemme/stone/etc va controllato se c'è gameover, oppure quando di dovrebbe inserire la gemspair ma la colonna centrale è piena). Fare un refactoring per questo tipo di casistiche non è mastodontico.
va stabilito bene, se è gameover quando la colonna centrale si riempie(aka ogni volta che si è inserito una o piu gemme/stone/etc va controllato se c'è gameover, oppure quando di dovrebbe inserire la gemspair ma la colonna centrale è piena). Fare un refactoring per questo tipo di casistiche non è mastodontico.
Beh, pensandoci bene (e parlandone con Fek) è emerso che c'è comunque da considerare il caso di un pareggio (ai tempi il pareggio era previsto, avremmo dovuto gestirlo una volta implementato il sistema dei round). Quindi presumibilmente, una volta che ci lavoreremo, invece di azzerare il numero di update della seconda griglia quando la prima fa game over vedremo addirittura di aumentarli, in modo da avere un briciolo di tolleranza che non renda impossibile un pareggio... ma bilanciando il tutto in modo che al contempo non si noti troppo.
Quindi, per ora - visto che ancora non esiste una partita vera e propria, ed il concetto di pareggio non avrebbe senso - consideriamolo un bug di cui non ci occupiamo (e per questo va nel livello D, ovvero la wish list). Quando sarà il momento gestiremo tutto :)
certo, ma è comunque importante analizzare il problema per vedere quando affrontarlo :)
Visto che il tempo e' limitato, non e' meglio spenderlo per analizzare uno dei problemi che abbiamo adesso piuttosto che analizzare un problema che molto probabilmente non dovremo mai risolvere?
che la seconda faccia la update è giusto - in fondo il controllo è effettuato sequenzialmente sulle due griglie, e non prevedere che anche la seconda griglia possa trovarsi nella stessa situazione ( e potenzialmente generare il gameover allo stesso ciclo della prima, instaurando una situazione di parità ) imho toglierebbe fairness al gameplay
per questo, tempo fa pensavo a un protocollo di controllo del gameover basato sui timestamp (ogni grid terrebbe conto del tempo trascorso dall' inizio della partita "dalla propria parte") e una variante dell handshake a 3 vie ( la prima griglia in gameover avrebbe notificato all' altra comunicandole il deltaT, la seconda avrebbe dato l' OK al gameover dopo un tempo pari alla differenza tra il deltaT ricevuto e il proprio - se in quel tempo avesse rilevato a propria volta il gameover si sarebbe avuta la parità)
ma in effetti mi rendo conto che basterebbe assicurare che entrambe le Grid siano passate attraverso lo stesso numero di cicli, quindi un contatore intero...
YAGNI
va stabilito bene, se è gameover quando la colonna centrale si riempie(aka ogni volta che si è inserito una o piu gemme/stone/etc va controllato se c'è gameover, oppure quando di dovrebbe inserire la gemspair ma la colonna centrale è piena). Fare un refactoring per questo tipo di casistiche non è mastodontico.
Non e' mastodontico, ma e' YAGNI. Abbiamo una montagna di refactoring ancora da fare, e alcuni bug importanti da risolvere. Per quanto ci riguarda, molto probabilmente, non affronteremo mai questo problema, a meno che Jocchan in futuro non gli alzi la priorita'.
Ricordiamoci che la priorita' dei task e dei bug e' stabilita dal customer, non dai noi.
Non e' mastodontico, ma e' YAGNI. Abbiamo una montagna di refactoring ancora da fare, e alcuni bug importanti da risolvere. Per quanto ci riguarda, molto probabilmente, non affronteremo mai questo problema, a meno che Jocchan in futuro non gli alzi la priorita'.
Ricordiamoci che la priorita' dei task e dei bug e' stabilita dal customer, non dai noi.
mettere in evidenza al customer di come si comporta l'applicativo in situazioni non perfettamente specificate non è YAGNI. Poi certo sta al customer decidere il da farsi.
ps.quel test lo braso via allora
mettere in evidenza al customer di come si comporta l'applicativo in situazioni non perfettamente specificate non è YAGNI. Poi certo sta al customer decidere il da farsi.
ps.quel test lo braso via allora
Guarda, in realta' e' un suo problema. Una volta che noi abbiamo fatto passare tutti i functional test, sta a lui "scriverne" altri per specificare meglio le sue intenzioni. Noi dobbiamo solo macinare task e fissare bug e dargli quello che lui chiede. Se lui non specifica chiaramente il suo volere, e' un problema suo (ed eventualmente del coach che gli deve estrarre specifiche e task).
Se lui non specifica chiaramente il suo volere, e' un problema suo (ed eventualmente del coach che gli deve estrarre specifiche e task).
...spezzandogli al contempo le ditine :Prrr:
Scherzi a parte, ogni volta che viene fuori un comportamento non previsto/considerato gli input di tutto il team sono fondamentali (altrimenti non se ne verrebbe nemmeno a conoscenza o si sottovaluterebbe la cosa, e finiremmo con il perdere il doppio del tempo).
Abbiamo un customer pigro :(
Guarda, in realta' e' un suo problema. Una volta che noi abbiamo fatto passare tutti i functional test, sta a lui "scriverne" altri per specificare meglio le sue intenzioni. Noi dobbiamo solo macinare task e fissare bug e dargli quello che lui chiede. Se lui non specifica chiaramente il suo volere, e' un problema suo (ed eventualmente del coach che gli deve estrarre specifiche e task).
nutri troppo speranze nel customer :asd: (non in quanto Jocchan, ma in generale)
forse sono un po traviato dal lavoro, in cui le specifiche sono molto lasche, e in cui i neanche i corner case più banali sono descritti, per cui mi tocca sempre rompere le balle con le varie situazioni che mi ritrovo nel codice e nessuno mi ha detto come gestire.
e se naturalmente qualcosa va storto(anche se non era stato specificato) la colpa è mia(non propiamente visto che ho responsabilita 0 sul progetto, pero...)
Abbiamo un customer pigro :(
Forse sì :D
Però il nostro obiettivo principale è arrivare ad un prodotto che implementi un certo set di feature in un tempo accettabile per le risorse di cui disponiamo, quindi ogni input che possa anche lontanamente portare ad una ri-valutazione della scaletta e degli obiettivi al fine di arrivare allo stesso risultato (se non addirittura superiore) senza perdere tempo (o perdendone meno di quanto ne avremmo perso altrimenti), lo reputo fondamentale.
nutri troppo speranze nel customer :asd: (non in quanto Jocchan, ma in generale)
forse sono un po traviato dal lavoro, in cui le specifiche sono molto lasche, e in cui i neanche i corner case più banali sono descritti, per cui mi tocca sempre rompere le balle con le varie situazioni che mi ritrovo nel codice e nessuno mi ha detto come gestire.
e se naturalmente qualcosa va storto(anche se non era stato specificato) la colpa è mia(non propiamente visto che ho responsabilita 0 sul progetto, pero...)
Beh, considera che - per quanto si analizzi a fondo qualcosa - è sempre facile sottovalutare certi dettagli, soprattutto se non balzano facilmente all'occhio. Il loro numero, però, è inversamente proporzionale al numero di persone che ci lavorano: più persone con punti di vista differenti hanno molte più probabilità di individuare questi punti critici, ed eliminarli, rispetto a poche.
il problema (che secondo me non è un problema, come da altro post) e che se la prima da gameover, la seconda comunque fa la update.
Se posso aggiungere...ed è anche corretto perché in teoria l'aggiornamento delle griglie dovrebbe avvenire in contemporanea, non a caso è fatto con lo stesso timestamp ;)
Sono due fasi che avvengono in successione, ma a fini della meccanica di gioco e del rendering è come se avvenissero contemporaneamente.
Se posso aggiungere...ed è anche corretto perché in teoria l'aggiornamento delle griglie dovrebbe avvenire in contemporanea, non a caso è fatto con lo stesso timestamp ;)
Sono due fasi che avvengono in successione, ma a fini della meccanica di gioco e del rendering è come se avvenissero contemporaneamente.
esatto!
esatto!
Probabilmente si voleva testare proprio questo: cioè che non si bloccasse subito l'altro campo.
Se non ricordo male era una correzione che evitava che a gioco avviato senza toccare un qualsiasi tasto vincesse sempre il giocatore di sinistra. Invece in questo modo il gameover appare per entrambi.
Probabilmente si voleva testare proprio questo: cioè che non si bloccasse subito l'altro campo.
Se non ricordo male era una correzione che evitava che a gioco avviato senza toccare un qualsiasi tasto vincesse sempre il giocatore di sinistra. Invece in questo modo il gameover appare per entrambi.
Ha perfettamente senso, grazie per la precisazione :)
Ha perfettamente senso, grazie per la precisazione :)
Tra l'altro è una eventualità improbabile, ma si può verificare anche durante il gioco che entrambi i giocatori perdano nello stesso "frame".
Tra l'altro è una eventualità improbabile, ma si può verificare anche durante il gioco che entrambi i giocatori perdano nello stesso "frame".
Sì, è un evento decisamente improbabile... tanto improbabile che, se ne ammettiamo la possibilità e consentiamo ad un round di terminare in pareggio, rischiamo di cadere in un caso paradossale in cui il comportamento è teoricamente corretto, ma dal punto di vista dei giocatori può non esserlo affatto: se entrambi perdessero all'update numero 50 sarebbe tutto ok, ma se uno dei due perdesse all'update 49 (mentre l'altro avrebbe comunque perso al 50) ai loro occhi - non essendoci differenze visibili su schermo - il comportamento sarebbe inconsistente, dato che in una situazione ci sarebbe stato un pareggio e nell'altra no.
E' per questo che, quando ci occuperemo di questo dettaglio (parecchio più avanti), bisognerà tener conto anche di come questo viene percepito da chi gioca.
http://img412.imageshack.us/img412/7749/buildisbrokenbyantares8fh4.gif
it.diamonds.tests.grid.iteration.TestGridRunsIterationsAndGarbageCollects$GarbageCollectionIteration
Tests: 1, Failures: 1, Errors: 0, Duration: 0.01
Build rossa da un'ora! Fede!
Prevedo ditina spezzate :asd:
Fatto il revert. Fede, se rompi di nuovo la build e te ne vai senza guardare Cruisecontrol vengo a Birmingham e ti spezzo il legamento crociato del ginocchio.
Fatto il revert. Fede, se rompi di nuovo la build e te ne vai senza guardare Cruisecontrol vengo a Birmingham e ti spezzo il legamento crociato del ginocchio.
Ecco un motivo in più per non trasferirmi a Londra. Con fek a pochi minuti di macchina il rischio di prenderle è troppo alto :asd:
[img]it.diamonds.tests.grid.iteration.TestGridRunsIterationsAndGarbageCollects$GarbageCollectionIteration
Tests: 1, Failures: 1, Errors: 0, Duration: 0.01
Dobbiamo vedere se in checkstyle c'è qualche controllo per impedire classi private innestate dentro ad altre. Anche qualche giorno fa la build era stata rotta da una cosa simile.
Fatto il revert. Fede, se rompi di nuovo la build e te ne vai senza guardare Cruisecontrol vengo a Birmingham e ti spezzo il legamento crociato del ginocchio.
Hai ragione non h oguaradto CC... Ma come e` possibile che fosse rotta se ANT non dava problema alcuno?
Come non detto porca paletta manco ANT dava green...
Mi sa che ho lanciato solo JUnit. Mai fare checkin notturni quando il cervello fa cilecca :|
Chiedo perdono in ginocchio sui ceci ardenti.
OK fixed. Scusate tanto :P
^TiGeRShArK^
10-02-2008, 18:51
http://img412.imageshack.us/img412/7749/buildisbrokenbyantares8fh4.gif
it.diamonds.tests.grid.iteration.TestGridRunsIterationsAndGarbageCollects$GarbageCollectionIteration
Tests: 1, Failures: 1, Errors: 0, Duration: 0.01
Build rossa da un'ora! Fede!
:rotfl:
è la prima volta che vedo l'apposita gif utilizzata a dovere :asd:
Abbiamo ancora 10 REFACTOR THIS. Non sono tanti quindi diamoci dentro cosi' possiamo ripartire con i task.
Ieri sera dovevo fare qualcosa, ma non mi e' venuto in mente niente di buono! :(
Domanda:
private boolean isGemValid(Droppable droppable, DroppableColor color)
{
// TODO: REFACTOR THIS (inline booleans)
boolean isFalling = droppable.isFalling();
boolean isGem = droppable.getGridObject().getType().isGem();
boolean isBigGem = droppable.getGridObject().getType().isBigGem();
boolean isSameColor = droppable.getGridObject().getColor().equals(color);
return isGem && isSameColor && !isBigGem && !isFalling;
}
Non ho capito il refactor this. :wtf:
Veramente Bonfo dovevi levare GridObject :P
Che ne dite di spezzare GameLoop in due classi? In questo momento si occupa di gestire sia il loop del gioco che quello del menu e francamente non mi piace per niente. Penso che una classe astratta di base sia la soluzione più semplice per ora.
Avete soluzioni alternative?
Che ne dite di spezzare GameLoop in due classi? In questo momento si occupa di gestire sia il loop del gioco che quello del menu e francamente non mi piace per niente. Penso che una classe astratta di base sia la soluzione più semplice per ora.
Avete soluzioni alternative?
Assolutamente si'.
Assolutamente si'.
Perfetto :D
Per ora ci ho messo un bel todo così non me ne dimentico. Se riesco a risolvere i problemi di comprensione che ho con ant in questi giorni ci lavoro io nel weekend a questo refactoring.
Veramente Bonfo dovevi levare GridObject :P
:doh: :doh:
In realta' ho mille cose che dovrei fare :D :D
Ieri sera dovevo fare qualcosa, ma non mi e' venuto in mente niente di buono! :(
Domanda:
private boolean isGemValid(Droppable droppable, DroppableColor color)
{
// TODO: REFACTOR THIS (inline booleans)
boolean isFalling = droppable.isFalling();
boolean isGem = droppable.getGridObject().getType().isGem();
boolean isBigGem = droppable.getGridObject().getType().isBigGem();
boolean isSameColor = droppable.getGridObject().getColor().equals(color);
return isGem && isSameColor && !isBigGem && !isFalling;
}
Non ho capito il refactor this. :wtf:
Diversi problemi:
- Una valanga di boolean per poi metterli in and
- quattro invocazioni di metodi di droppable e nient'altro, che cosa ti fa venire in mente? ;)
- GridObject di mezzo
Questo metodo va spostato in Droppable e ogni tipo di droppable deve tornare false, a parte le gemme che devono tronare true se il colore e' lo stesso e la gemma non sta cadendo. Inoltre ci vuole un nome migliore di isGemValid().
Ok, ma GirdObject lo eliminiamo e gli facciamo ritornare un Droppabale Description??
Di conseguenza dai Droppable sparisce anche il getType e getColor, che tanto sono cose gestite dal DroppableDescription. Giusto?
Ti va bene "canAddToBigGem()" ??
P.S.: Quando ho letto Refactor This pensavo qualcosa di piu' "locale", e siccome era ormai sparito tutto nonsapevo cosa altro semplificare :D
l'eliminazione di GridObject comporta questo codice ( ad esempio) :
droppable.getDroppableDescription().getType().isStone()
che ne dite??
^TiGeRShArK^
13-02-2008, 20:46
l'eliminazione di GridObject comporta questo codice ( ad esempio) :
droppable.getDroppableDescription().getType().isStone()
che ne dite??
ehmm... provvisoriamente spero... :mbe:
...non dovevano sparire tutti i vari getType e is*()?:fagiano:
ehmm... provvisoriamente spero... :mbe:
...non dovevano sparire tutti i vari getType e is*()?:fagiano:
Si'!
Ehm... per tirare fuori il metodo ci vogliono i test!
Ora non ho abbastanza tempo :(
Altra cosa, puo' provare qualcun'altro a togliere GirdObject... :flower:
Anonimo e 71104 che fine han fatto? :P
Anonimo e 71104 che fine han fatto? :P
Mi sa che erano sotto esami. Ma non sono sicuro.
P.S.: e' 2 giorni che non tocco Diamonds :muro: :muro:
Ho dato un po' di colpi stasera. Ho fatto sparire pure GridObject :)
Ragazzi, non lasciate solo me e Fede a fare commit vero?
Bastano letteralmente 30 minuti al giorno per fare un commit utile.
Anche se piccolo e` pur sempre un contributo =)
Ragazzi, non lasciate solo me e Fede a fare commit vero?
GameLoop è una porcata. Mi toccherà riscrivere tutti i test e mi ci vuole tempo :O
Bastano letteralmente 30 minuti al giorno per fare un commit utile.
Anche se piccolo e` pur sempre un contributo =)
Esatto. E scuse come "non ho tempo", "non ho piu' dimestichezza col codice", "mi e' morto il gatto", "ho i brufoli", sono, per l'appunto, scuse :)
Io lavoro 10/12 ore al giorno (nelle giornate fortunate), ed una mezz'oretta per un commit la mattina o la sera riesco a trovarlo, fra le duecentotrenta cose che faccio.
Prendete un Refactor This e sistematelo, magari non ci riuscite in mezz'ora oggi e potete riprendere domani. E' sempre un valido aiuto. Non fare nessun commit e accampare scuse non e' un aiuto.
GameLoop è una porcata. Mi toccherà riscrivere tutti i test e mi ci vuole tempo :O
Grazie :)
be' senti sarà che siete più bravi a programmare ma io a fare un commit utile in mezz'ora col piffero che ci riesco... e interrompere e riprendere il giorno dopo non credo sia una buona idea perché poi si rischia di staccarsi troppo dalla versione nel repository e ci si ritrova con conflitti fenomenali (magari anche causati dal fatto che qualcuno s'era messo a lavorare sullo stesso refactor this).
per vedere commit miei pazientate ancora qualche giorno... :help:
dai che la sessione è quasi finita, nel bene o nel male :mc: :cry:
@71104: e' ovvio che se devi riscrivere mezzo repository ci metti piu' di una mezzora!!
Io ieri ho trasformato DroppableColor da classe ad enumerativo. Ci ho messo circa 7 minuti ( di cui 2 per lanciare i test e 3 tra scrivere commento e committare). Basta picconare un po' alla volta ;)
@All: che ne dite di rinominare AbstractDroppableType con DroppableType??
Tanto tutte le sottoclassi hanno visibilita' package, ovvero invisibili da fuori, quindi fuori e' visibile solo l'abstract.
Io infatti non capisco sta mania di chiamare Abstract le classi astratte :P
Io infatti non capisco sta mania di chiamare Abstract le classi astratte :P un tempo era nel checkstyle: ant si incazzava se le classi astratte non avevano un nome che cominciava con Abstract e le interfacce non avevano un nome che finiva con Interface.
un tempo era nel checkstyle: ant si incazzava se le classi astratte non avevano un nome che cominciava con Abstract e le interfacce non avevano un nome che finiva con Interface.
Ehm... non un tempo :(
Ho provato a rinominare ma il checkstyle si imbufalisce :muro:
be' senti sarà che siete più bravi a programmare ma io a fare un commit utile in mezz'ora col piffero che ci riesco... e interrompere e riprendere il giorno dopo non credo sia una buona idea perché poi si rischia di staccarsi troppo dalla versione nel repository e ci si ritrova con conflitti fenomenali (magari anche causati dal fatto che qualcuno s'era messo a lavorare sullo stesso refactor this).
Francamente preferisco vedere un commit diviso in due o tre giorni che non vedere mai un commit. Inoltre, imparare a fare i merge e' una cosa piuttosto utile.
@All: che ne dite di rinominare AbstractDroppableType con DroppableType??
Tanto tutte le sottoclassi hanno visibilita' package, ovvero invisibili da fuori, quindi fuori e' visibile solo l'abstract.
Va benissimo per me, basta che checkstyle non si arrabbi.
Va benissimo per me, basta che checkstyle non si arrabbi.
Ecco, appunto, si arrabbia :(
^TiGeRShArK^
15-02-2008, 23:45
Bastano letteralmente 30 minuti al giorno per fare un commit utile.
Anche se piccolo e` pur sempre un contributo =)
...infatti io ogni volta che sono al pc fisso di casa un committino lo faccio...
il problema è che sono praticamente sempre con il pc del lavoro (quasi sempre), con il portatile dal letto (le rare volte che me lo lasciano i miei fra), con il nokia e61i dal letto(il 90% delle volte che sono a casa).... :stordita:
..e il dramma è che per ora sono al lavoro anche il sabato e la domenica.. :muro:
^TiGeRShArK^
15-02-2008, 23:49
Esatto. E scuse come "non ho tempo", "non ho piu' dimestichezza col codice", "mi e' morto il gatto", "ho i brufoli", sono, per l'appunto, scuse :)
Io lavoro 10/12 ore al giorno (nelle giornate fortunate), ed una mezz'oretta per un commit la mattina o la sera riesco a trovarlo, fra le duecentotrenta cose che faccio.
Prendete un Refactor This e sistematelo, magari non ci riuscite in mezz'ora oggi e potete riprendere domani. E' sempre un valido aiuto. Non fare nessun commit e accampare scuse non e' un aiuto.
Grazie :)
riuscissi a stare seduto davanti ad un pc dopo i in 10...N ore al giorno a programmare lo farei anche...
ma l'unico ambiente che riesco a sopportare è il letto.... (o il cesso al massimo :asd: )
vedo se riesco domani a committare qualcosa in pausa pranzo... :sob:
^TiGeRShArK^
15-02-2008, 23:51
Francamente preferisco vedere un commit diviso in due o tre giorni che non vedere mai un commit. Inoltre, imparare a fare i merge e' una cosa piuttosto utile.
NO.
I merge non esistono :O
...magari!!! :muro:
wingman87
16-02-2008, 00:03
Ma se uno volesse unirsi al team di sviluppo cosa dovrebbe fare, leggere, scaricare? C'è un thread al riguardo? Se c'è non l'ho visto, scusate.
Si', ci sono quelli sticky:
http://www.hwupgrade.it/forum/showthread.php?t=1016614
ricorda che pero' gli indirizzsi ora sono questi:
svn://fcarucci.homeip.net/diamonds
svn://fcarucci.homeip.net/diamonds/trunk
(e' scritto in fondo al thread)
Purtroppo pero' il server e' giu', ma domani Fran rimette aq posto tutto ;)
Poi, per incominciare a capire la metodologia leggi qui (http://www.hwupgrade.it/forum/showthread.php?t=1017489) ed eventualmente qui (http://www.hwupgrade.it/forum/showthread.php?t=1019019), poi qualcuno ti fa fare un task in Pair Programming e sarai bello e immatricolato ;)
Bonfo, il server e' su ed e' stato su tutta la notte.
Problema di DNS?
Edit: la macchina virtuale della buildmachine ha deciso di non vedere la rete, forse perche' e' una bella giornata di sole. Sistemo.
Edit 2: Sistemato. Virtual Server e' terribile, ha deciso di assegnare lo stesso mac address a due macchine virtuali :-|
La build e' rotta quando si cerca di fare un dist.
Non compila. Lanciate da linea di comando:
ant dist
Non trova GarbageColelctGemsByColor in compilazione quando e' eseguito cobertura e il gioco e' compilato in release. Fede, lo fissi per cortesia?
Per cortesia, la mattina guardate anche la nightly-build e lanciate 'ant dist' prima del commit. Stiamo rompendo la build troppo spesso in questi giorni.
(magari anche causati dal fatto che qualcuno s'era messo a lavorare sullo stesso refactor this)
In teoria chi comincia a lavorare su un Refactor This dovrebbe segnalarlo qui :)
(e magari anche ricordarsi di aggiornare con "fatto" o "lascio perdere" o "domani" :fiufiu: :flower: )
Bonfo, il server e' su ed e' stato su tutta la notte.
Problema di DNS?
Edit: la macchina virtuale della buildmachine ha deciso di non vedere la rete, forse perche' e' una bella giornata di sole. Sistemo.
Edit 2: Sistemato. Virtual Server e' terribile, ha deciso di assegnare lo stesso mac address a due macchine virtuali :-|
Build machine ancora giù?
^TiGeRShArK^
16-02-2008, 13:38
impossibile stabilire la connessione.. :muro:
Build machine ancora giù?
E' su che e' una bellezza.
E' su che e' una bellezza.
Non so che dirti. Prima apriva la connessione ma poi andava in timeout.
^TiGeRShArK^
16-02-2008, 17:20
Non so che dirti. Prima apriva la connessione ma poi andava in timeout.
a me non faceva fare nemmeno l'update prima....
ora non ne ho idea... :fagiano:
Non so che dirti. Prima apriva la connessione ma poi andava in timeout.
Spooky :mbe:
Anche qui SVN non fungeva stamane.
Come faccio a fissare sta cosa i nrelease? Non capisco cosa possa cambiare da debug a release :\
^TiGeRShArK^
16-02-2008, 19:30
Anche qui SVN non fungeva stamane.
Come faccio a fissare sta cosa i nrelease? Non capisco cosa possa cambiare da debug a release :\
ma a me funge ant dist..
l'hai fissato il problema?
Cmq le differenze tra debug e release dovrebbero essere queste:
classpath:
release non ha:
${lib}/junit.jar
${lib}/jdepend-2.9.jar
${lib}/cobertura/cobertura.jar
compile:
compile-debug compila due volte: prima le classi e poi i test.
non mi pare ci sia altro nel compile...
Al lavoro su move di DroppablePair
Prima...
public void move(Direction direction)
{
// TODO: REFACTOR THIS
if (slave == null)
{
grid.moveDroppable(pivot, direction);
return;
}
if (grid.droppableCanMove(pivot, direction)
&& grid.droppableCanMove(slave, direction))
{
grid.moveDroppable(pivot, direction);
grid.moveDroppable(slave, direction);
}
else if (droppablesAreHorizontal())
{
if (grid.droppableCanMove(slave, direction))
{
grid.moveDroppable(slave, direction);
grid.moveDroppable(pivot, direction);
}
else if (grid.droppableCanMove(pivot, direction))
{
grid.moveDroppable(pivot, direction);
grid.moveDroppable(slave, direction);
}
}
}
Dopo... non ho fatto molto, ma non vedo cosa si possa fare di più...
public void move(Direction direction)
{
if (slave == null)
{
grid.moveDroppable(pivot, direction);
return;
}
if (droppablesAreHorizontal())
{
moveWhenHorizontal(direction);
}
else
{
moveWhenVertical(direction);
}
}
private void moveWhenVertical(Direction direction)
{
if (grid.droppableCanMove(pivot, direction)
&& grid.droppableCanMove(slave, direction))
{
grid.moveDroppable(pivot, direction);
grid.moveDroppable(slave, direction);
}
}
private void moveWhenHorizontal(Direction direction)
{
if (grid.droppableCanMove(slave, direction))
{
grid.moveDroppable(slave, direction);
grid.moveDroppable(pivot, direction);
}
else if (grid.droppableCanMove(pivot, direction))
{
grid.moveDroppable(pivot, direction);
grid.moveDroppable(slave, direction);
}
}
Già che c'ero ho fatto anche l'altro REFACTOR THIS di DroppablePair:
Prima...
private void rotateSlave(SlaveRotation rotation)
{
if (pivot == null || slave == null)
{
return;
}
Position position = slavePosition;
Direction newDirection;
Movement movement = rotation.getMovement(position);
newDirection = movement.direction();
position = movement.newPosition();
if (grid.droppableCanMove(pivot, newDirection))
{
Region pivotGridObject = pivot.getRegion();
// TODO: REFACTOR THIS
grid.moveDroppableToCell(slave, pivotGridObject.getTopRow()
+ newDirection.getRowDelta(), pivotGridObject.getLeftColumn()
+ newDirection.getColumnDelta());
slavePosition = position;
}
}
Dopo...
private void rotateSlave(SlaveRotation rotation)
{
if (pivot == null || slave == null)
{
return;
}
Movement movement = rotation.getMovement(slavePosition);
Direction newDirection = movement.direction();
Position position = movement.newPosition();
if (grid.droppableCanMove(pivot, newDirection))
{
Region pivotCell = pivot.getRegion();
int newRow = pivotCell.getTopRow() + newDirection.getRowDelta();
int newColumn = pivotCell.getLeftColumn() + newDirection.getColumnDelta();
grid.moveDroppableToCell(slave, newRow, newColumn);
slavePosition = position;
}
}
Semplificherò ancora sia questo che quello precedente una volta chiarito un dubbio con Ufo :D
Al lavoro su move di DroppablePair
Dopo... non ho fatto molto, ma non vedo cosa si possa fare di più...
Si puo' sempre fare di piu' :)
Ci sono tante chiamate accoppiate, su pivot e slave, una dopo l'altra: ognuna di quelle coppie va estratta in un suo metodo che esegue quell'azione sulla pair (ad esempio moveDroppable e' un ottimo candidato).
Poi le catene di if, pensa a come puoi eliminarle, creando una classe interna, o magari un enum, o magari facendo qualche assunzione, ma eliminali.
int newRow = pivotCell.getTopRow() + newDirection.getRowDelta();
int newColumn = pivotCell.getLeftColumn() + newDirection.getColumnDelta();
Questo e' molto semplice da estrarre in un metodo e spostare in Cell, che restituisce una cella a partire da una direzione. Poi i due controlli sugli oggetti null ormai invocano un Null Pattern.
Infine vedo bene un if guard al posto dell'if di CanMove...
blackknight
18-02-2008, 10:30
Si puo' sempre fare di piu' :)
Poi le catene di if, pensa a come puoi eliminarle, creando una classe interna, o magari un enum, o magari facendo qualche assunzione, ma eliminali.
La mia ignoranza mi opprime:D ... come posso eliminare un if con una classe interna?
Nel caso dell'enum non bisogna cmq fare un confronto?
La mia ignoranza mi opprime:D ... come posso eliminare un if con una classe interna?
Nel caso dell'enum non bisogna cmq fare un confronto?
Hmmm forse in questo caso con una classe interna non c'e' molta speranza. Stasera butto un'occhio attorno al codice.
Questo e' molto semplice da estrarre in un metodo e spostare in Cell, che restituisce una cella a partire da una direzione.
Quel refactoring di Cell l'ho gia` implementato io, basta usarlo :)
Quel refactoring di Cell l'ho gia` implementato io, basta usarlo :)
Ed era esattamente la semplificazione che volevo fare, il problema è che falliscono dei test, in quanto quando la cella in questione è ad esempio (0, 4) e mi muovo a sinistra quel metodo non fa controlli e cerca di creare una nuova cella (-1, 4), ma il costruttore di Cell lancia un'eccezione...
Ed era esattamente la semplificazione che volevo fare, il problema è che falliscono dei test, in quanto quando la cella in questione è ad esempio (0, 4) e mi muovo a sinistra quel metodo non fa controlli e cerca di creare una nuova cella (-1, 4), ma il costruttore di Cell lancia un'eccezione...
Ci penso io stasera.
Ed era esattamente la semplificazione che volevo fare, il problema è che falliscono dei test, in quanto quando la cella in questione è ad esempio (0, 4) e mi muovo a sinistra quel metodo non fa controlli e cerca di creare una nuova cella (-1, 4), ma il costruttore di Cell lancia un'eccezione...
Incredibile... c'ho pensato pure io stanotte!!
Queste cosine sono perfette per semplificare enormenete anche BigGem ;)
(E poi devo eliminare un paio di metodi ridondanti con il metodo fatto da ufo :D )
Ho visto ora il metodo, a me mi serve qualcosa di un po' piu' complesso. :O
Per semplificare BigGem sarebbe molto utile avere un metodo per prendere le aree adiacenti ad una region
Per semplificare BigGem sarebbe molto utile avere un metodo per prendere le aree adiacenti ad una region
Lo sto implmentando :D :D (ovviamente TDD) ;)
Lo sto implmentando :D :D (ovviamente TDD) ;)
Fatto ;)
Ottimi i refactor a BigGem :)
Ottimi i refactor a BigGem :)
Un po' alla volta ci stiamo arrivando... canMoveDown erano 2, uno in bigGem e uno in AbstractSingleDroppable. Ora ce ne e' uno solo in AbstractDroppable!
Significa che la gestione si sta sempre di piu' semplificando ;)
EDIT: fran, non so se extend sia il nome migliore
BigGemDroppableType e' vuoto. Il costruttore di BigGem ora sarebbe cosi':
public BigGem(DroppableColor color, Engine engine)
{
super(new DroppableDescription(AbstractDroppableType.GEM, color));
getRegion().resizeToContain(new Region(0, 0, 2, 2));
AnimatedSprite animatedSprite = new DroppableRenderer(engine, getRegion(), getDroppableDescription());
setAnimatedSprite(animatedSprite);
}
Commit o aspetto?
EDIT: ehm, ho scritto una cavolata... ci sarebbe rimasto solo
DROPPABLE_TYPE_NAMES.put(AbstractDroppableType.BIG_GEM, "tiles");
in DroppableDescriptor.
Ho visto il Null Pattern in Cell. Ha senso mantenere il costruttore pubblico??
L'ho messo privato e rimosso l'eccezione :)
Lo sto implmentando :D :D (ovviamente TDD) ;)
Lo abbiamo fatto in contemporanea :D
Ho fatto il revert del mio.
Commit o aspetto?
EDIT: ehm, ho scritto una cavolata... ci sarebbe rimasto solo
DROPPABLE_TYPE_NAMES.put(AbstractDroppableType.BIG_GEM, "tiles");
in DroppableDescriptor.
Commit!
public void moveDown(Grid grid)
{
if (canMoveButNotWithFullGravity(grid))
{
while (canMoveDown(grid))
{
if (getSprite().getPosition().getY() == grid.getRowUpperBound(getRegion().getTopRow()))
{
getRegion().setRow(getRegion().getTopRow() + 1); /// NOT TESTED
}
getSprite().getPosition().setY(grid.getRowUpperBound(getRegion().getTopRow()));
}
return;
}
getSprite().translate(0, (float)grid.getActualGravity() / 2);
if (getSprite().getPosition().getY() > grid.getRowUpperBound(getRegion().getTopRow()))
{
getRegion().setRow(getRegion().getTopRow() + 1);
}
}
C'e' una linea non testata. Inoltre ha complessita' alta. moveToCell() non e' testata. Chi se ne occupa?
blackknight
19-02-2008, 09:45
Stasera posso occuparmene io :cool:
jappilas
19-02-2008, 18:40
... stavo occupandomene oggi
ora, spostare il codice attuale da CrushByChestAction.updateCrushesOn() (a proposito, noto che da ieri è diventato molto più pulito, bravo bonfo:) ) a Chest.crushGemsOn(Grid) è questione di poco
DA:
CrushbychestAction:
protected void applyOn(Droppable gem)
{
// TODO: REFACTOR THIS
// isXxx methods must disappear
if (gem.getType().isChest() && !gem.isFalling())
{
isCrushed |= updateCrushesOnChest(gem);
}
}
private boolean updateCrushesOnChest(Droppable gem)
{
DroppableList gemList = new DroppableList();
getAdjacentCrushableGems(gem, gemList);
if (gemList.size() == 0)
{
return false;
}
getGrid().crushDroppablesWithScore(gemList);
return true;
}
Grid:
public void updateCrushes()
{
for (Droppable droppable : droppableList)
{
if (droppable.isFalling())
{
continue;
}
droppable.crushGemsOn(this);
}
garbageCollectDroppables();
// TODO: REFACTOR THIS
// This behavior should be defined in Chest.crushGemsOn
crushByChestAction.reset();
doIteration(crushByChestAction);
if (crushByChestAction.isCrushed())
{
scoreCalculator.incrementChainCounter();
}
}
public void crushDroppablesWithScore(DroppableList crushableGems)
{
for (Droppable gemToCrush : crushableGems)
{
scoreCalculator.addDroppable(gemToCrush);
stoneCalculator.addDroppable(gemToCrush);
removeDroppable(gemToCrush);
}
}
A
Grid:
private boolean isCrushing = false;
public void updateCrushes()
{
isCrushing = false;
for (Droppable droppable : droppableList)
{
if (droppable.isFalling())
{
continue;
}
droppable.crushGemsOn(this);
}
garbageCollectDroppables();
if (isCrushing)
{
scoreCalculator.incrementChainCounter();
}
}
public void crushDroppablesWithScore(DroppableList crushableGems)
{
for (Droppable gemToCrush : crushableGems)
{
scoreCalculator.addDroppable(gemToCrush);
stoneCalculator.addDroppable(gemToCrush);
addDroppableToGarbage(gemToCrush);
}
}
public void enableCrushCounter()
{
isCrushing = true;
}
Chest:
public void crushGemsOn(Grid grid)
{
DroppableList gemList = new DroppableList();
getAdjacentCrushableGems(this, gemList, grid);
if (gemList.size() == 0)
{
return;
}
grid.enableCrushCounter();
grid.crushDroppablesWithScore(gemList);
}
jappilas
19-02-2008, 18:58
Il Problema è che se si affidano le crush provocate dai Chest allo stesso loop for (Droppable droppable : droppableList)
{
if (droppable.isFalling())
{
continue;
}
droppable.crushGemsOn(this);
}, in Grid.updateCrushes(), in cui già vengono effettuate le crush dovute alla flashing Gem, ( che poteva essere uno dei motivi per introdurre un metodo polimorfico), viene meno la semantica della Flashing Gem che vuole la sua crush generata prima di quella di eventuali Chest , per il semplice motivo che la DroppableList è iterata nell' ordine in cui le Droppable sono fino state inserite nella Griglia fino a quel momento ( la flashing gem arrivata con l' ultima gempair sarà quindi chiamata per ultima, o penultima )
per questo, avevo in mente due soluzioni - una basata sul polimorfismo di Droppables, un oggetto di Grid privilegiato, "passate" successive in UpdateCrushes()
in Droppable
public void reactToGridInsertion()
in AbstractDroppable implementazione vuota;
in Grid
private Droppable priorityCrusherGem;
public void updateCrushes()
{
priorityCrush();
isCrushing = false;
normalCrush();
if (isCrushing)
{
scoreCalculator.incrementChainCounter();
}
}
private void normalCrush()
{
for (Droppable droppable : droppableList)
{
if (droppable.isFalling())
{
continue;
}
droppable.crushGemsOn(this);
}
garbageCollectDroppables();
}
public void priorityCrush()
{
if (priorityCrusherGem == null)
{
return;
}
if (priorityCrusherGem.isFalling())
{
return;
}
priorityCrusherGem.crushGemsOn(this);
garbageCollectDroppables();
priorityCrusherGem = null;
}
public void setPriorityCrusherGem(Droppable priorityCrusherGem)
{
this.priorityCrusherGem = priorityCrusherGem;
}
public Droppable getPriorityCrusherGem()
{
return priorityCrusherGem;
}
public void insertDroppable(Droppable droppable, int row, int column)
{
<...>
droppable.reactToInsertionInGrid(this);
}
in FlashingGem:
public void reactToInsertionInGrid(Grid grid)
{
grid.setPriorityCrusherGem((Droppable)this);
}
in TestGrid
public void testFlashingGemSetsUpAsGridPriorityCrusherGem()
{
FlashingGem gem = createFlashingGem();
insertAndUpdate(gem, 10, 1);
assertEquals("The Flashing Gem must have set up a Priority Crusher Droppable in Grid",
gem, grid.getPriorityCrusherGem());
}
public void testChestDoesntAlterGridPriorityCrusherGemField()
{
insertAndUpdate(createChest(DroppableColor.DIAMOND), 10, 1);
assertNull("The Grid's Priority Crusher Field must be empty", grid.getPriorityCrusherGem());
}
public void testNormalGemDoesntAlterGridPriorityCrusherGemField()
{
insertAndUpdate(createGem(DroppableColor.DIAMOND), 10, 1);
assertNull("The Grid's Priority Crusher Field must be empty", grid.getPriorityCrusherGem());
}
public void testStoneDoesntAlterGridPriorityCrusherGemField()
{
insertAndUpdate(createStone(DroppableColor.DIAMOND), 10, 1);
assertNull("The Grid's Priority Crusher Field must be empty", grid.getPriorityCrusherGem());
}
public void testGridPriorityCrusherNotUnsetIfCrushingWithFlashingGemStillFalling()
{
insertAndUpdate(createFlashingGem(), 10, 1);
grid.updateCrushes();
assertNotNull("The Grid's priority crusher must not be null if Flashing Gem still falling",
grid.getPriorityCrusherGem());
}
public void testGridPriorityCrusherNullIfCrushingWithFlashingGemNotFalling()
{
insertAndUpdate(createFlashingGem(), 10, 1);
makeAllGemsFall();
grid.updateCrushes();
assertNotNull("The Grid's priority crusher must be null after crushing a fallen Flashing Gem",
grid.getPriorityCrusherGem());
}
ora, un' altar soluzione potrebbe essere estrarre per prime le ultime due Droppable dalla lista e chiamare crushGemsOn su quelle fuori dal ciclo, ma mi pare poco robusta... :O
Wakka quanta roba hai fatto!
Mi sono perso a meta' :D :D
Scusa, ma non si puo' fare un metodo crush() e un crushPriority(int priority), cosi' possiamo definire la priorita' per ogni gemma o flashing o dinamite che sia e poi nel codice diventa:
int[] priorityList = {...};
for(int priority:priorityList )
{
grid.crush(priority);
}
No...?!?
jappilas
19-02-2008, 19:53
Wakka quanta roba hai fatto!troppo in effetti... so già cosa dirà fek :asd:
Mi sono perso a meta' :D :Dnon stare a leggere tutto, serviva essenzialmente a dimostare che non ho ignorato i test ;):D
essenzialmente:
grid tiene posto per una gemma prioritizzata;
priorityCrusher.crushGemOn è chiamata ( con annessa garbage collection) prima del loop e quel campo è svuotato subito dopo;
la gestione di quel campo è fatta affidando alle singole Droppable (polimorfismo) la reazione all' inserimento in griglia: questo per non avere
if (gem.getType.isFlashingGem()) in Grid ;)
è anche vero che ero partito dall' assunzione che non ci potesse essere più di una flashing gem per volta e avevo concepito il tutto in funzione di quello :O
Scusa, ma non si puo' fare un metodo crush() e un crushPriority(int priority), cosi' possiamo definire la priorita' per ogni gemma o flashing o dinamite che sia e poi nel codice diventa:
int[] priorityList = {...};
for(int priority:priorityList )
{
grid.crush(priority);
}
No...?!?buona idea - in effetti si potrebbe semplificare ulteriormente tenendo conto del fatto che le "priorità" sono solo due (flash > chest)...
Scusa, ma non si puo' fare un metodo crush() e un crushPriority(int priority), per favore Bonfo, d'ora in poi diamo sempre nomi chiari ed esplicativi :D
"crushPriority" tradotto significa "distruggi la priorità" o qualcosa del genere :asd:
un bel "With" non ce lo vedo male: "crushWithPriority", se ho capito bene cosa dovrebbe fare il metodo che ipotizzi.
per favore Bonfo, d'ora in poi diamo sempre nomi chiari ed esplicativi :D
"crushPriority" tradotto significa "distruggi la priorità" o qualcosa del genere :asd:
un bel "With" non ce lo vedo male: "crushWithPriority", se ho capito bene cosa dovrebbe fare il metodo che ipotizzi.
Gia'... ho pensato ini italiano :(
Il nome del metodo e priorityCrush, o forse e' meglio ancora prioritizedCrush :D :D
Gia'... ho pensato ini italiano :(
Il nome del metodo e priorityCrush, o forse e' meglio ancora prioritizedCrush :D :D :mano:
jappilas
19-02-2008, 21:11
versione finale, codice complessivo:
CrushPriority
public enum CrushPriority
{
ABSOLUTE_PRIORITY (10), NORMAL_PRIORITY(5), NO_PRIORITY(0);
private int value;
private CrushPriority(int value)
{
this.value = value;
}
public int getValue()
{
return value;
}
} con valore intero a fini di eventuale confronto
in Grid:
private CrushPriority[] crushPriorities = {CrushPriority.ABSOLUTE_PRIORITY, CrushPriority.NORMAL_PRIORITY};
public void updateCrushes()
{
isCrushing = false;
for (CrushPriority priority : crushPriorities)
{
for (Droppable droppable : droppableList)
{
if (droppable.isFalling())
{
continue;
}
droppable.crushGemsOn(this, priority);
}
garbageCollectDroppables();
}
if (isCrushing)
{
scoreCalculator.incrementChainCounter();
}
}
public void enableCrushCounter()
{
isCrushing = true;
}
Chest:
public void crushGemsOn(Grid grid, CrushPriority priority)
{
if (priority.getValue() > CrushPriority.NORMAL_PRIORITY.getValue())
{
return;
}
[ ....]
}
In Flashing Gem non è necessario modificare alcunchè a parte il prototipo del metodo
grazie per il suggerimento :)
^TiGeRShArK^
19-02-2008, 21:39
versione finale, codice complessivo:
...
In Flashing Gem non è necessario modificare alcunchè a parte il prototipo del metodo, avendo priorità "assoluta" non è necessario fare verifiche, si esegue direttamente la crush appena possibile - l' importante è che i chest non vengano crushati nello stesso loop
grazie per il suggerimento :)
...ma NO_PRIORITY quando viene usata? :mbe:
non bastava usare un booleano crushIsPrioritary per il confronto anzichè un enum nel caso in cui serva confrontare solo 2 valori? :fagiano:
Se NO_PRIORITY non e' usato deve essere tolto.
isCrushing deve sparire a tutti i costi.
Grid deve essere un contenitore stateless. Non deve contenere uno stato come isCrushing.
jappilas
19-02-2008, 22:19
updateCrushes() controlla a ogni passata quante Droppable vengono rimosse dalla GC, se >0 significa che vi è stata una crush in quella passata, se vi è stata almeno una crush alla fine del loop si aumenta il Chain Counter public void updateCrushes()
{
int crushes = 0;
for (CrushPriority priority : crushPriorities)
{
for (Droppable droppable : droppableList)
{
if (droppable.isFalling())
{
continue;
}
droppable.crushGemsOn(this, priority);
}
if ((droppablesToGarbageCollect.size()) > 0)
{
crushes++;
}
garbageCollectDroppables();
}
if (crushes>0)
{
scoreCalculator.incrementChainCounter();
}
}
nessun metodo speciale enableCrush() o simile chiamato da Chest;
CrushPriority.NO_PRIORITY rimosso - lascerei il sistema a interi per la semantica della "priorità della crush in corso maggiore della mia (per il chest) "... :stordita:
Perche' un getValue()??
Non basterebbe aggiungere campo a droppableDescriptor indicando la priorita' e se currentPriority == myPriority eseguo??
jappilas
19-02-2008, 22:32
Perche' un getValue()??
Non basterebbe aggiungere campo a droppableDescriptor indicando la priorita' e se currentPriority == myPriority eseguo??hai ragione :O
hai ragione :O
Stavo pensando ora che frose Droppable Descriptor non e' il posto migliore...
... non so cosa proporre :rolleyes:
Mi fido ti jappilas ;)
ragazzi, non stavo più seguendo qui ma ditemi, per caso qualcuno sta lavorando su CrushByChestAction? :stordita:
perché mi stavo mettendo a fare un paio di refactor this che erano segnati in quella action (e che naturalmente contrbuirebbero a farla sparire), ma se già qualcuno la sta ammazzando non voglio disturbare :D
jappilas
19-02-2008, 23:20
ragazzi, non stavo più seguendo qui ma ditemi, per caso qualcuno sta lavorando su CrushByChestAction? :stordita:
perché mi stavo mettendo a fare un paio di refactor this che erano segnati in quella action (e che naturalmente contrbuirebbero a farla sparire), ma se già qualcuno la sta ammazzando non voglio disturbare :Dl' ho appena ammazzata io ;) http://forums.nsn3.net/style_emoticons/default/80.gif
Stavo pensando ora che frose Droppable Descriptor non e' il posto migliore...
... non so cosa proporre :rolleyes:
Mi fido ti jappilas ;)... alla fine ho lasciato il solo enum perchè non serviva nemmeno il getValue() - e in effetti mi è parso non servisse nemmeno inserire la crush priority nel descrittore, visto che viene usato solo durante l' esecuzione di un metodo, nel metodo stesso... :stordita:
FlashingGem.crushGemsOn(...) è eseguita comunque, Chest.crushGemsOn(...) se la priorità è != NORMAL_PRIORITY, ritorna... ;)
non riesco a collegarmi a svn :muro:
CrushPriority.NO_PRIORITY rimosso - lascerei il sistema a interi per la semantica della "priorità della crush in corso maggiore della mia (per il chest) "... :stordita:
YAGNI. Toglilo.
vBulletin® v3.6.4, Copyright ©2000-2025, Jelsoft Enterprises Ltd.