Call To Arms: oggi e domani si fa refactoring (ancora piu' del solito)

[fek]

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^]

io volevo iniziare stamattina col refactoring..
ma mi sono ritrovato la build rotta
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
appena la build è verde parto

[Ufo13]

volentieri

[fek]

Originariamente inviato da ^TiGeRShArK^

io volevo iniziare stamattina col refactoring..
ma mi sono ritrovato la build rotta
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
appena la build è verde parto

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?

[^TiGeRShArK^]

Originariamente inviato da fek

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?

io già scleravo quando vedevo costruttori con 4 parametri
cmq ora parto col refactoring

[fek]

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.

[Bonfo]

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...

[Ufo13]

Originariamente inviato da fek

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)

[thebol]

Originariamente inviato da fek

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.

[Ufo13]

Originariamente inviato da thebol

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.

[fek]

Chi si occupa del 2)?

[VICIUS]

Originariamente inviato da fek

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...

Originariamente inviato da fek

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

[fek]

Originariamente inviato da VICIUS

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^]

Originariamente inviato da fek

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


P.S. ma siamo sincronizzati che posti sempre poco prima che io torni sul forum a vedere com'è la situazione??? o ci spii tutti???

[VICIUS]

Originariamente inviato da ^TiGeRShArK^

no, è sempre colpa di Chet


P.S. ma siamo sincronizzati che posti sempre poco prima che io torni sul forum a vedere com'è la situazione??? o ci spii tutti???

Vi osserviamo. Sempre.

ciao

[Ufo13]

sto cercando di sistemare la faccenda GameLoop + abstract + tutto il resto

[VICIUS]

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

ciao

[Ufo13]

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

[Ufo13]

Vic a me non da errore sui test... Mi da errore perchè GridTestCase non contiene nessun test

[^TiGeRShArK^]

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?