View Full Version : Call To Arms: oggi e domani si fa refactoring (ancora piu' del solito)
Mi giunge voce che TestGameLoop ha bisogno di una sostanziale rivisitazione.
Ci sono 4 test che falliscono (a breve i particolari).
Edito il topic man mano che trovo nuove aeree in cui intervenire.
Voglio vedere molta gente intervenire in questo topic durante il finesettimana, e prendersi amorevole cura del codice. E questa e' una richiesta molto gentile ;)
Basta anche che rinominiate un metodo, aggiungiate un test, semplifichiate una riga di codice, non c'e' bisogno di fare un lavoro grosso. Anche se non avete mai fatto un task e state studiando il codice (dnarod), prendete un metodo e semplificatelo, e' il modo migliore per imparare.
Io e Vicius partiamo subito. Voglio vedervi seguire a ruota.
Dunque:
- fate update del codice
- fate il refactoring (test driven)
- lanciate ant
- fate di nuovo update
- lanciate ant di nuovo
- commit
^TiGeRShArK^
04-03-2006, 14:52
io volevo iniziare stamattina col refactoring..
ma mi sono ritrovato la build rotta :p
cmq io stavo pensando a semplificare un pò testGrid che si inizia a non capirci più niente....
volevo estrarre i test specifici per warningbox e crushbox tanto per iniziare... epoi vedendo facendo :D
appena la build è verde parto :Prrr:
io volevo iniziare stamattina col refactoring..
ma mi sono ritrovato la build rotta :p
cmq io stavo pensando a semplificare un pò testGrid che si inizia a non capirci più niente....
volevo estrarre i test specifici per warningbox e crushbox tanto per iniziare... epoi vedendo facendo :D
appena la build è verde parto :Prrr:
La build e' verde. Prosegui a sistemare TestGrid. Io sono partito da TestGameLoop, ma prima devo dare una bella sistemata a PlayField: il costruttore ha 7, dico 7, parametri. Sette parametri. Sette parametri. Chi ha osato tanto? :D
^TiGeRShArK^
04-03-2006, 15:18
La build e' verde. Prosegui a sistemare TestGrid. Io sono partito da TestGameLoop, ma prima devo dare una bella sistemata a PlayField: il costruttore ha 7, dico 7, parametri. Sette parametri. Sette parametri. Chi ha osato tanto? :D
:rotfl:
io già scleravo quando vedevo costruttori con 4 parametri :D
cmq ora parto col refactoring:oink:
:D
Finito il lavoro su GameLoop e PlayField, ora sembra decisamente piu' semplice, ma c'e' ancora molto spazio di manovra.
Ci sono alcune cose che non mi piacciono:
1) I Keymapping erano e sono tutt'ora assolutamente non testati e vanno testati. Ho aggiunto una classe KeyMappings che e' un insulto alla programmazione in Java (l'ho praticamente scritta in C++ per pigrizia): chi vuole cimentarsi in una versione piu' elegante e ben testata e' libero di farlo. Anzi, e' caldamente invitato a farlo.
2) AbstractGame, a mio avviso, cosi' com'e' ha poco senso. Sicuramente non e' un Game astratto, a occhio sembra piu' una specie di bootstrapper. Poi ha una relazione molto strana con GameFactory: secondo me GameFactory deve sparire, perche' la complessita' che aggiunge non sembra assolutamente giustificata. Inoltre sono entrambe non testate (male male male).
3) PlayFields e GameLoop hanno una relazione fin troppo intima. Ho cercato di slegarle un po'. con un minimo di successo, ma ancora non mi piace. GameLoop dovrebbe sia creare sia gestire i PlayFields a occhio. Questa parte necessita di ulteriore semplificazione.
4) GameLoop e' testata in maniera francamente imbarazzante. Diciamocelo, sembra che chi ha scritto GameLoop abbia messo quei test tanto per far vedere che qualche test c'e', ma non con lo spirito di scrivere la classe test-driven. Non mi e' piaciuto affatto. Infatti il design ne e' uscito piuttosto povero. Ci vuole una migliore copertura ed un miglior design.
In generale ho notato poca attenzione alla qualita' del codice e al suo design, e la cosa ci ha portato in questa situazione in cui noto che pochi hanno voglia di mettere mano nel codice, perche' oggettivamente non e' facile da districare. Ci vuole piu' attenzione, piu' refactoring e tornare ai principi che hanno funzionato tanto bene fino a ieri: fare le cose semplici.
Ho letto ora il post.
Anch'io darò una mano come posso.
Devo essere sincero...mi sono un po' perso gli utlimi passaggi e novità sul codice. Quindi ci metterò un po' di tempo a tornare sull'onda.
Altra cosa...grazie fek di essere tornato a spezzarci le ditina... :D :D
Finito il lavoro su GameLoop e PlayField, ora sembra decisamente piu' semplice, ma c'e' ancora molto spazio di manovra.
Ci sono alcune cose che non mi piacciono:
1) I Keymapping erano e sono tutt'ora assolutamente non testati e vanno testati. Ho aggiunto una classe KeyMappings che e' un insulto alla programmazione in Java (l'ho praticamente scritta in C++ per pigrizia): chi vuole cimentarsi in una versione piu' elegante e ben testata e' libero di farlo. Anzi, e' caldamente invitato a farlo.
2) AbstractGame, a mio avviso, cosi' com'e' ha poco senso. Sicuramente non e' un Game astratto, a occhio sembra piu' una specie di bootstrapper. Poi ha una relazione molto strana con GameFactory: secondo me GameFactory deve sparire, perche' la complessita' che aggiunge non sembra assolutamente giustificata. Inoltre sono entrambe non testate (male male male).
3) PlayFields e GameLoop hanno una relazione fin troppo intima. Ho cercato di slegarle un po'. con un minimo di successo, ma ancora non mi piace. GameLoop dovrebbe sia creare sia gestire i PlayFields a occhio. Questa parte necessita di ulteriore semplificazione.
4) GameLoop e' testata in maniera francamente imbarazzante. Diciamocelo, sembra che chi ha scritto GameLoop abbia messo quei test tanto per far vedere che qualche test c'e', ma non con lo spirito di scrivere la classe test-driven. Non mi e' piaciuto affatto. Infatti il design ne e' uscito piuttosto povero. Ci vuole una migliore copertura ed un miglior design.
In generale ho notato poca attenzione alla qualita' del codice e al suo design, e la cosa ci ha portato in questa situazione in cui noto che pochi hanno voglia di mettere mano nel codice, perche' oggettivamente non e' facile da districare. Ci vuole piu' attenzione, piu' refactoring e tornare ai principi che hanno funzionato tanto bene fino a ieri: fare le cose semplici.
Mi occupo del (1)
3) PlayFields e GameLoop hanno una relazione fin troppo intima. Ho cercato di slegarle un po'. con un minimo di successo, ma ancora non mi piace. GameLoop dovrebbe sia creare sia gestire i PlayFields a occhio. Questa parte necessita di ulteriore semplificazione.
Ho tolto il riferimento all'origine del gameOverMessage da gameLoop e l'ho spostato in playField(tramite la classe playFieldDesciptor) e ora gameloop fa pura creazione e gestione(update(), reactToinput(), showgameOverMessage(LayerManager layerManager), quit()) di playfield.
Ho tolto il riferimento all'origine del gameOverMessage da gameLoop e l'ho spostato in playField(tramite la classe playFieldDesciptor) e ora gameloop fa pura creazione e gestione(update(), reactToinput(), showgameOverMessage(LayerManager layerManager), quit()) di playfield.
Ho appena trovato un bug.. Puoi guardare nel thread dei problemi per vedere se l'hai introdotto con il refactoring? Magari sai dove può essere...
Sono certo che bisogna aggiungere dei test da quelle parti.
2) AbstractGame, a mio avviso, cosi' com'e' ha poco senso. Sicuramente non e' un Game astratto, a occhio sembra piu' una specie di bootstrapper. Poi ha una relazione molto strana con GameFactory: secondo me GameFactory deve sparire, perche' la complessita' che aggiunge non sembra assolutamente giustificata. Inoltre sono entrambe non testate (male male male).
In effetti ora il nome ha poco senso ma quando ho creato la classe non c'era tutto quel codice dentro. Ora abstract non serve assolutamente più, anzi da fastidio perché visto che non posso testarla una classe abstract. Purtroppo non so che nome dare alla nuova classe. Bootstrap va bene ?
GameFacotry serve a condividere il codice del bootstrap con i test altrimenti poi ci riduciamo come TestGameLoop in cui l'85% del codice è un copia e incolla da AbstractGame e GameLoop. Ora basta farsi un TestableGameFactory che crea dei MockEngine, MockKeybaord...
4) GameLoop e' testata in maniera francamente imbarazzante. Diciamocelo, sembra che chi ha scritto GameLoop abbia messo quei test tanto per far vedere che qualche test c'e', ma non con lo spirito di scrivere la classe test-driven. Non mi e' piaciuto affatto. Infatti il design ne e' uscito piuttosto povero. Ci vuole una migliore copertura ed un miglior design.
Colpa mia anche qui. Avevo cominciato a scrivere il tutto in maniera TDD ma quando ho visto che dovevo scrivere il codice due volte sia in Game che in TestGameLoop diciamo che mi sono rotto dopo i primi due test e ho cominciato a fare refactoring per cercare di risolvere.
ciao ;)
In effetti ora il nome ha poco senso ma quando ho creato la classe non c'era tutto quel codice dentro. Ora abstract non serve assolutamente più, anzi da fastidio perché visto che non posso testarla una classe abstract. Purtroppo non so che nome dare alla nuova classe. Bootstrap va bene ?
Si', qualcosa come GameBootstrap mi sembra ok.
GameFacotry serve a condividere il codice del bootstrap con i test altrimenti poi ci riduciamo come TestGameLoop in cui l'85% del codice è un copia e incolla da AbstractGame e GameLoop. Ora basta farsi un TestableGameFactory che crea dei MockEngine, MockKeybaord...
Ok, capito, allora serve solo un po' di lifiting e chiarire bene i rapporti con GameBootstrap. Ci penso un po'.
Colpa mia anche qui. Avevo cominciato a scrivere il tutto in maniera TDD ma quando ho visto che dovevo scrivere il codice due volte sia in Game che in TestGameLoop diciamo che mi sono rotto dopo i primi due test e ho cominciato a fare refactoring per cercare di risolvere.
Il codice e' di tutti e se ci sono pochi test o il design e' carente, la colpa e' di tutti, mai di uno solo :)
^TiGeRShArK^
05-03-2006, 18:45
Il codice e' di tutti e se ci sono pochi test o il design e' carente, la colpa e' di tutti, mai di uno solo :)
no, è sempre colpa di Chet :O
:D
P.S. ma siamo sincronizzati che posti sempre poco prima che io torni sul forum a vedere com'è la situazione??? o ci spii tutti???:sofico:
no, è sempre colpa di Chet :O
:D
P.S. ma siamo sincronizzati che posti sempre poco prima che io torni sul forum a vedere com'è la situazione??? o ci spii tutti???:sofico:
Vi osserviamo. Sempre. :fiufiu:
ciao ;)
sto cercando di sistemare la faccenda GameLoop + abstract + tutto il resto
Ho fatto un po di ordine nei test che riguardano le griglie. C'erano una vetina di versioni uguali di insertAndUpdate, createGem e altre funzioni di aiuto. Alcune implementavano sul posto. Altre estendevano una classe astratta e altre usavano una classe esterna di aiuto. Ho messo tutto dentro a GridTestCase e fatto estendere quella classe ai vari test.
Nello smanettare pero ho incontrato 5 test che ora si rifiutano di passate. Si trovano in TestBigGemInGrid. Chi li ha scritti può guardare se ho fatto un casino che io non ci capisco niente :p
ciao ;)
Ho eliminato Factory per Game... Diciamo che c'è ancora molto da fare però almeno diminuisce il codice non testato (eliminandole non ho avuto manco mezzo errore all'interno dei test).
Ora do un'occhio a quello che dice Vic :)
Vic a me non da errore sui test... Mi da errore perchè GridTestCase non contiene nessun test
^TiGeRShArK^
05-03-2006, 20:06
Ho eliminato in grid questa duplicazione:
// TODO: queste due funzioni sono "quasi" identiche.
private boolean hasGemMovedToNextRow(Gem gem)
{
final float gemNextPosition = gem.getY() + actualGravity;
final float rowUpperBound = gem.getCellRow() * yStep + bounds.top();
return gemNextPosition > rowUpperBound;
}
private boolean hasGemMovedToRowBottom(Gem gem)
{
final float gemNextPosition = gem.getY();
final float rowUpperBound = gem.getCellRow() * yStep + bounds.top();
return gemNextPosition >= rowUpperBound;
}
in questo modo:
private boolean hasGemMovedToNextRow(Gem gem)
{
return hasGemMovedToRow(gem, actualGravity - 0.5F);
}
private boolean hasGemMovedToRowBottom(Gem gem)
{
return hasGemMovedToRow(gem, 0);
}
private boolean hasGemMovedToRow(Gem gem, float gravity)
{
final float gemNextPosition = gem.getY() + gravity;
final float rowUpperBound = gem.getCellRow() * yStep + bounds.top();
return gemNextPosition >= rowUpperBound;
}
i test passano tutti, ma non riesco a trovare un nome abbastanza esplicativo al metodo che ho provvisoriamente chiamato hasGemMovedToRow....
qualche suggerimento?:fagiano:
^TiGeRShArK^
05-03-2006, 20:15
Vic a me non da errore sui test... Mi da errore perchè GridTestCase non contiene nessun test
a me con ant non da nessun errore, da eclipse mi da il tuo stesso prob con GridTestCase
^TiGeRShArK^
05-03-2006, 20:26
ok, confermo
qualche suggerimento per il metodo di cui sopra??? :D
Vic a me non da errore sui test... Mi da errore perchè GridTestCase non contiene nessun test
GridTestCase non viene eseguito perché non comincia con Test e va bene cosi. I test che non vanno sono commentati e stanno in TestBigGemInGrid.
ciao ;)
Che si dice di bello ?!?!?
Sono tornato da poco... :D
GridTestCase non viene eseguito perché non comincia con Test e va bene cosi. I test che non vanno sono commentati e stanno in TestBigGemInGrid.
ciao ;)
aggiungendo:
testDummy() { assertTrue(true); }
va... Quindi mi sa che viene aggiunto anche se non inizia con Test...
per i test commentati ora guardo
^TiGeRShArK^
05-03-2006, 21:14
Che si dice di bello ?!?!?
Sono tornato da poco... :D
bentornato tesoro!:smack:
:D
per caso capisci qualcosa delle BigGem???
ci sono 4 test che non passano dopo un refactoring di vic e io non ho idea di come sono implementate le BigGem... :fiufiu:
bentornato tesoro!:smack:
:D
per caso capisci qualcosa delle BigGem???
ci sono 4 test che non passano dopo un refactoring di vic e io non ho idea di come sono implementate le BigGem... :fiufiu:
In pratica fallisce perchè quando vengono inserite le gemme vengono updatate e scendono di una posizione rendendo impossibile aggiungere quelle sotto.. Non capisco perchè prima funzionava ed adesso no :\
bentornato tesoro!:smack:
:D
per caso capisci qualcosa delle BigGem???
ci sono 4 test che non passano dopo un refactoring di vic e io non ho idea di come sono implementate le BigGem... :fiufiu:
Ci posso provare... La build machine è verde, i test che falliscono sono commentati ?
aggiungendo:
testDummy() { assertTrue(true); }
va... Quindi mi sa che viene aggiunto anche se non inizia con Test...
per i test commentati ora guardo
Strano ho provato più volte mentre mettevo a posto con ant e la classe non veniva mai chiamata. :confused:
Come lanci i test ?
ciao ;)
Ci posso provare... La build machine è verde, i test che falliscono sono commentati ?
Se guardi nei task di eclipse ci sono un po' di TODO da eliminare :)
ciao ;)
Se guardi nei task di eclipse ci sono un po' di TODO da eliminare :)
Ehm...dove ? :stordita:
Ehm...dove ? :stordita:
Window -> Show view -> Other. In General scegli Tasks.
ciao ;)
fatto io... Fixati i test...
Vicius: lanciando il tool grafico dentro eclipse mi dava quel errore...
^TiGeRShArK^
05-03-2006, 21:55
infatti anche a me con ant non dava problemi, ma con eclipse si senzaa testdummy.
Cmq buono che ora sia tutto verde.... qual'era il problema?? :p
bisogna chiamare il gem.drop() quando si crea la bigGem in quei casi...
Stavo guardando i test che avevo sistemato poco fa e ho notato una marea di getGrid() getTimer() ma non riesco a capire a che cosa servono? per quale motivo mettere private gli attributi nella classe madre e usare delle getter per fare sto lavoro. Rendono praticamente illeggibile il codice a mio avviso.
ciao ;)
A me hanno insegnato ad usare sempre dei getter...
...immagina che da domani invece di usare millisecondi usiamo i microsecondi per l'attributo pubblico la modifica toccherebbe l'intera classe con il getter basta fare:
return 100*time
Ti piace come risposta??? :cool:
A parte che sono d'accordo con i getter, ma in ogni caso è una imposizione di checkstyle (quindi di fek ;)), non ci possono essere variabili pubbliche...a parte le costanti...
Stavo guardando i test che avevo sistemato poco fa e ho notato una marea di getGrid() getTimer() ma non riesco a capire a che cosa servono? per quale motivo mettere private gli attributi nella classe madre e usare delle getter per fare sto lavoro. Rendono praticamente illeggibile il codice a mio avviso.
ciao ;)
Lo so ma la build era rotta senza getter/setter. Comunque proprio pubblici era meglio di no... Al massimo protected ed era rotta anche così... CheckStyle li vuole privati o nisba :P
A parte che sono d'accordo con i getter, ma in ogni caso è una imposizione di checkstyle (quindi di fek ;)), non ci possono essere variabili pubbliche...a parte le costanti...
E a mio avviso e' un'ottima imposizione perche' costringe a pensare piu' attentamente al design. In una classe ci sono molti getter/setter? Vuol dire che ci sono molti campi e vuol dire che c'e' sicuramente modo di separare questa classe in piu' classi piu' piccole oppure di riorganizzarla in modo da eliminare il problema.
Lo so ma la build era rotta senza getter/setter. Comunque proprio pubblici era meglio di no... Al massimo protected ed era rotta anche così... CheckStyle li vuole privati o nisba :P
Sei sicuro? mentre sistemavo i test lanciavo ant in continuazione e checkstyle non è mai fallito. Dopo mangiato rifaccio una prova veloce mettendoli protected e togliendo i getter per una conferma.
In ogni caso non ho nulla contro i getter solo che trovo assurdo che una classe figlia debba usare i getter poter accedere in qualsiasi modo alla classe madre.
ciao ;)
A me hanno insegnato ad usare sempre dei getter...
...immagina che da domani invece di usare millisecondi usiamo i microsecondi per l'attributo pubblico la modifica toccherebbe l'intera classe con il getter basta fare:
return 100*time
Ti piace come risposta??? :cool:
Per le classi del sistema in generale sono perfettamente d'accordo con te. Ma se uno è talmente coglione da usare il codice dei test dentro al proprio codice è giusto che se lo prenda in quel posto :p
ciao ;)
Sei sicuro? mentre sistemavo i test lanciavo ant in continuazione e checkstyle non è mai fallito. Dopo mangiato rifaccio una prova veloce mettendoli protected e togliendo i getter per una conferma
Confermo. Funziona perfettamente e checkstyle non dice niente.
ciao ;)
Confermo. Funziona perfettamente e checkstyle non dice niente.
E sicuramente ha più senso...almeno per le classi derivate...
Per le classi del sistema in generale sono perfettamente d'accordo con te. Ma se uno è talmente coglione da usare il codice dei test dentro al proprio codice è giusto che se lo prenda in quel posto :p
ciao ;)
ooooppss....non avevo capito il problema :flower:
Confermo. Funziona perfettamente e checkstyle non dice niente.
ciao ;)
Molto meglio allora :)
vBulletin® v3.6.4, Copyright ©2000-2025, Jelsoft Enterprises Ltd.