|
|
|
|
Strumenti |
03-02-2008, 20:21 | #1 |
Senior Member
Iscritto dal: Oct 2002
Città: California
Messaggi: 11781
|
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!" 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.
__________________
"We in the game industry are lucky enough to be able to create our visions" Ultima modifica di fek : 06-02-2008 alle 14:53. |
03-02-2008, 22:03 | #2 |
Senior Member
Iscritto dal: Nov 2005
Città: Bologna
Messaggi: 1303
|
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 |
03-02-2008, 22:20 | #3 |
Bannato
Iscritto dal: Feb 2005
Città: Roma
Messaggi: 7027
|
bella come idea
mi metto a caccia. |
03-02-2008, 22:35 | #4 |
Senior Member
Iscritto dal: Jul 2002
Città: Reggio Calabria -> London
Messaggi: 12091
|
oh porca
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 ) 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
__________________
|
03-02-2008, 23:05 | #5 |
Bannato
Iscritto dal: Feb 2005
Città: Roma
Messaggi: 7027
|
it.diamonds.grid.action.CrushByFlashAction.java:53
codice attuale: Codice:
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; } Codice:
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; } |
04-02-2008, 00:18 | #6 |
Bannato
Iscritto dal: Feb 2005
Città: Roma
Messaggi: 7027
|
mi ci sono scervellato, meglio di così non riesco a fare: non riesco ad eliminare quegli if (PER ORA). committo così com'è.
|
04-02-2008, 01:07 | #7 |
Bannato
Iscritto dal: Feb 2005
Città: Roma
Messaggi: 7027
|
minchia, guardate che roba
it.diamonds.tests.droppable.gems.TestBigGemInGrid:457 Codice:
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); } Codice:
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); } Ultima modifica di 71104 : 04-02-2008 alle 01:11. |
04-02-2008, 03:17 | #8 |
Senior Member
Iscritto dal: Nov 2005
Città: Bologna
Messaggi: 1303
|
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: Codice:
private static int rows; private static int columns; ... public Grid(Environment environment, Point origin) { .... rows = environment.getConfig().getInteger("rows"); columns = environment.getConfig().getInteger("columns"); .... } Se non viene mai costruita una grid, rows e columns sono entrambi a 0. Ma poi mi speighi il vantaggio di averli statici?!? Io no sono lo spezzatore di dita ufficiale, ma poi finisce che chiedo una delega . |
04-02-2008, 04:16 | #9 |
Senior Member
Iscritto dal: Nov 2005
Città: Bologna
Messaggi: 1303
|
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. |
04-02-2008, 05:47 | #10 |
Senior Member
Iscritto dal: Nov 2005
Città: Bologna
Messaggi: 1303
|
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. Ultima modifica di Bonfo : 04-02-2008 alle 07:57. |
04-02-2008, 08:20 | #11 |
Senior Member
Iscritto dal: Nov 2005
Messaggi: 1536
|
Minchia Bonfo avevamo appena iniziato a cambiare tutto in Cell e hai ri-revertato i cambiamenti fatti
L'idea e` usare Cell ovunque. |
04-02-2008, 08:37 | #12 | ||
Senior Member
Iscritto dal: Oct 2002
Città: California
Messaggi: 11781
|
Quote:
Quote:
Quale TODO hai rifattorizzato per questo commit? La regola vale per tutti.
__________________
"We in the game industry are lucky enough to be able to create our visions" |
||
04-02-2008, 08:52 | #13 |
Senior Member
Iscritto dal: Oct 2002
Città: California
Messaggi: 11781
|
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.
__________________
"We in the game industry are lucky enough to be able to create our visions" |
04-02-2008, 09:10 | #14 |
Senior Member
Iscritto dal: Oct 2002
Città: California
Messaggi: 11781
|
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.
__________________
"We in the game industry are lucky enough to be able to create our visions" |
04-02-2008, 09:14 | #15 |
Senior Member
Iscritto dal: Nov 2005
Messaggi: 1536
|
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...
|
04-02-2008, 09:22 | #16 |
Senior Member
Iscritto dal: Oct 2002
Città: California
Messaggi: 11781
|
Si', e' chiaro che se stai facendo una serie di commit come me in questo momento, basta un solo TODO.
__________________
"We in the game industry are lucky enough to be able to create our visions" |
04-02-2008, 10:43 | #17 |
Bannato
Iscritto dal: Feb 2005
Città: Roma
Messaggi: 7027
|
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
|
04-02-2008, 19:08 | #18 |
Senior Member
Iscritto dal: Aug 2001
Città: San Francisco, CA, USA
Messaggi: 13826
|
Prima:
Codice:
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: Codice:
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); } } (Non sono molto esperto di refactoring )
__________________
GPU Compiler Engineer |
04-02-2008, 19:14 | #19 |
Bannato
Iscritto dal: Feb 2005
Città: Roma
Messaggi: 7027
|
|
04-02-2008, 19:15 | #20 |
Senior Member
Iscritto dal: Aug 2001
Città: San Francisco, CA, USA
Messaggi: 13826
|
Oookkei
__________________
GPU Compiler Engineer |
Strumenti | |
|
|
Tutti gli orari sono GMT +1. Ora sono le: 23:39.