[CICLO 6/bis] Xmas Carol Story 2

[Jocchan]

Originariamente inviato da cdimauro

Ragazzi, tra qualche ora parto per Roma. Ci vediamo al mio ritorno. Ne approfitto per fare gli auguri di buone feste a tutti.

Divertiti, e buone feste

[fek]

Originariamente inviato da cionci

Sì

Aggiungendo un factory method a InputReactor non togli alcuna duplicazione, quindi imho è un refactoring non necessario... Il factory method serve proprio per eliminare le duplicazioni...

L'aggiunta degli handler necessari a Grid è compito di GridController...e quindi deve essere fatta nel costruttore...

Magari il factory method di GridController in futuro potrebbe essere necessario (anche se secondo me potrebbe benissimo andarci un costruttore al suo posto), ma quello di InputReactor in futuro non farà altro che complicare il design...visto che dovremmo aggiungere un factory method per ogni classe che usa handlers diversi (e applica handlers diversi a classi diverse)...

Comunque nell'ultima build ho spostato il contenuto del factory method di InputReactor nel costruttore di GridController...

Tra l'altro anche il fatto stesso che GridController astrae sia Grid che InputReactor giustificherebbe la loro allocazione all'interno di un costruttore...

Si', proseguite su questa linea. Questo dovrebbe eliminare anche quell'orribile factory method di InputReactor in GridController (lo misi io).

Una piccola nota, cerchiamo durante queste vacanze di dare un po' di stabilita' alla codebase: la necessita di fare refactoring non e' una licenza a stare sempre in refactoring land e a cambiare il codice tanto per fare. Ogni refactoring deve avere uno scopo preciso per semplificare il codice, e' giusto fare tentativi ma e' altrettanto importante sapere quando fare il commit del refactoring e quando non farlo.

[cionci]

Visto che Xmas.2.1 è un task veramente lungo ho fatto un refactoring abbastanza grosso (figuriamoci come sarebbe stato il task) per avvantaggiare chi dovrà svolgere il task...ho portato fuori da Grid tutti i metodi *GemUnderControl ed ho modificato la signature di Grid.update...aggiungendo come parametro la gemma su cui dovranno essere applicati i cambiamenti... Ho spostato l'update delle animazioni delle gemme in update di GridController... In questo modo il metodo update di Grid potrà essere applicato anche a più gemme (una coppia ) con chiamate separate...

Ovviamente ho rimesso a posto la quintalata di test che davano errori...

[thebol]

sono sempre stato favorevole a questo refactoring(l'avevo fatto io steso, ma poi ero andato oltre e mi era stato revertato).

ma con ufo, stavamo ponderando una strada diversa, cioè mantenere in grid GemUnderControl. gridControll avrebbe settato la gemma pivot come gemUnderControl, l'avrebbe mossa, avrebbe settato la gemma slave, e l'avrebbe mossa(questa come idea generale).

La cosa aveva senso, non so quanto avrebbe potuto funzionare(ci stava guardando ufo), ma poteva essere interessante.

[cionci]

Comunque potrebbe funzionare anche avendo tirato fuori gemUnderControl..almeno da come hai descritto quello che volete fare...
Dopo tutto avere gemUnderControl in Grid presuppone che grid debba occuparsi anche dell'input cosa che invece non fa...IMHO era un controsenso lasciare gemUnderControl in grid...

[Ufo13]

Originariamente inviato da cionci

Comunque potrebbe funzionare anche avendo tirato fuori gemUnderControl..almeno da come hai descritto quello che volete fare...
Dopo tutto avere gemUnderControl in Grid presuppone che grid debba occuparsi anche dell'input cosa che invece non fa...IMHO era un controsenso lasciare gemUnderControl in grid...

sì... Come avevo detto anche secondo me bisognava muoversi su questa strada grazie per il refactoring è stato un bel regalo

[cionci]

Originariamente inviato da Ufo13

sì... Come avevo detto anche secondo me bisognava muoversi su questa strada grazie per il refactoring è stato un bel regalo

E' stato il tuo ultimo refactoring che mi ha ispirato

[Ufo13]

Task Xmas.2.1 completato

[thebol]

aggiunta la classe gemsPair e test del caso(molti spostati da gridController)

Nota:
negli handler viene fatto un controllo se esiste la gemma pivot, questo controllo non sarebbe piu giusto farlo nella moveLeft/Right di gridController o di gemsPair?


cmq ora gli altri task possono andare avanti direi.

Ciao

[Ufo13]

sì direi che ormai si può fare da move

Hai lasciato di nuovo codice commentato.. cancello

[cionci]

Ottimo Io direi di togliere moveLeft e moveRight da GridController...

[Ufo13]

Idem

[DanieleC88]

Originariamente inviato da thebol

cmq ora gli altri task possono andare avanti direi.

Stasera/domani mi metto al lavoro e comincio il task 2.2.

[thebol]

Originariamente inviato da cionci

Ottimo Io direi di togliere moveLeft e moveRight da GridController...

fatto, ho tolto anche il controllo sulla gemma pivot negli handler e l'ho messo nelle moveleft/right di gemspair.

la chiamata negli handler ora risulta però un po prolissa..

gridController.getGemsPair().moveRight(gridController.getGrid());


era per questo che l'avevo lasciata in gridController, ma in effetti era una dupicazione inutile.

[Ufo13]

thebol avevo già fatto io hai fatto update prima di commitare?

[thebol]

Originariamente inviato da Ufo13

thebol avevo già fatto io hai fatto update prima di commitare?

durante il commit ho visto le differenze

[Ufo13]

Jocchan, non mi è chiaro cosa deve succedere quando una rotazione della mia (quella del tasto C) non è possibile... Ripetendo la rotazione semplicemente avrei un ritorno alla situazione attuale


P.S. Il task 2.3 non lo ha preso nessuno?

[Jocchan]

Originariamente inviato da Ufo13

Jocchan, non mi è chiaro cosa deve succedere quando una rotazione della mia (quella del tasto C) non è possibile... Ripetendo la rotazione semplicemente avrei un ritorno alla situazione attuale

Stavo per scrivere "esatto", ma in effetti mi hai fatto venire in mente un caso limite, per cui dobbiamo agire in maniera differente.
Normalmente, se la pivot è a sinistra/destra della slave, e non c'è spazio di movimento, è normale che il comando venga ignorato.
Idem se la pivot è sotto la slave (se è sopra, non ci saranno problemi in nessun caso), vuol dire che sta per collidere e non c'è lo spazio per ruotare.
In questo caso, però, se l'utente preme il tasto quando la pivot è appena entrata nell'ultima casella libera prima della collisione, una non risposta ai comandi potrebbe essere frustrante.
Quindi, in questo caso (e solo in questo) direi che possiamo fare un'eccezione alla "regola di spostamento" (al contrario di come ti ho detto ieri su msn, cioè che per coerenza dovevamo mantenere lo stesso tipo di comportamento), e consentire che le due gemme vengano scambiate di posizione tra di loro.

[Ufo13]

quindi è valido solo nel caso in cui pivot è sotto, slave è sopra e sta per collidere?

[Jocchan]

Aspetta, ci sto riflettendo ancora su, ed in effetti questa eccezione non va bene, perchè - appunto - va evidentemente contro le altre regole di spostamento, e quindi creerebbe delle situazioni poco coerenti. Vediamo di impostare la cosa con una certa "permissività".

Quando la pivot è sotto la slave, ed è appena entrata nella casella libera più in basso (diciamo che ne ha occupati al max 10 pixel), se il giocatore preme il tasto C faremo in modo di forzare lo spostamento al solito modo (insomma, la slave si piazza sotto la pivot, e quest'ultima - in pratica - torna su fino ai bordi della casella)... in pratica, la facciamo entrare con la forza.
Se si supera questo valore limite, lo spostamento sarà impossibile e verrà ignorato.

Siamo costretti a dividere il tutto in questi due casi per consentire lo stesso al giocatore spostamenti "all'ultimo secondo", senza generare incoerenze