View Full Version : Refactoring e l'importanza del codice di qualita'
Sto rifattorizzando handleKeyEvent() di Grid e procede tutto in maniera abbastanza spedita.
Refactoring dopo refactoring, tutto rigorosamente usando gli strumenti di Eclipse arrivo a questo:
public int handleKeyEvent(KeyEvent event)
{
if(KeyCode.vk_Down == event.key())
{
return handleDownKey(event);
}
else
if(KeyCode.vk_Left == event.key())
{
return leftKey.handleEvent(event);
}
else
if(KeyCode.vk_Right == event.key())
{
return rightKey.handleRight(this, event);
}
return 0;
}
Mi piace molto, c'e' questo metodo handleEvent() della classe keyData che racchiude il codice comune ai due branch per i tasti left e right. L'ultimo passo e' eliminare handleRight e usare handleEvent perche' sono sostanzialmente identici.
Lo faccio:
if(KeyCode.vk_Left == event.key())
{
return leftKey.handleEvent(event);
}
else
if(KeyCode.vk_Right == event.key())
{
return rightKey.handleEvent(event);
}
Test fallito. La gemma non si muove a destra. C'e' un problema. Neppure vi devo dire che se non fosse fallito quel test avrei dato per scontato che tutto stava andando bene perche' quel refactoring nella mia testa e' naturale.
Dov'e' il problema? Un buon dieci minuti rifacendo il refactoring passettino per passettino (io non voglio usare il debugger) e scopro che per una distrazione non mi ero accorto che i due branch ritornano due valori diversi (1 e -1). Ma che cosa rappresentano quei due numeri? Non lo so, dal codice del metodo non e' chiaro. Ma e' davvero una distrazione mia? Si', sicuramente lo e', ma e' anche vero che il codice non mi ha aiutato, non chiarisce subito il significato di quei due numeri. Allora sono andato a vedere la chiamata a handleKeyEvent() e scopro che quel numero e' usato per incrementare o decrementare la colonna corrente.
Ora, dov'e' il problema? Il problema e' che il metodo handleKeyEvent sta svolgendo due compiti:
1) Sta gestendo gli eventi di pressione dei tasti.
2) Sta calcolando di quanto la gemma si debba spostare a seguito della pressione di un tasto.
Questo metodo ha due responsabilita' e quando un'entita ha due responsabilita' c'e' un (piccolo o grande) problema. Nel nostro caso e' piuttosto piccolo, ma io chiarirei meglio le intenzioni che il codice vuole esprimere e separerei questi due compiti.
Ecco dove sono arrivato col refactoring:
public int handleKeyEvent(KeyEvent event)
{
if(KeyCode.vk_Down == event.key())
{
return handleDownKey(event);
}
else
if(KeyCode.vk_Left == event.key())
{
return leftKey.handleEvent(event);
}
else
if(KeyCode.vk_Right == event.key())
{
return rightKey.handleEvent(event);
}
return 0;
}
Ed ho risolto il problema dell'offset in maniera un po' sporca cosi':
public int handleEvent(KeyEvent event)
{
processEvent(event);
if(isPressed())
{
return columnOffset;
}
return 0;
}
Chi si vuole occupare di separare i due comportamenti e rifattorizzare ulteriormente questo codice? Siamo molto vicini ad una soluzione molto elegante, in cui aggiungere altri tasti non ci richieda una modifica a handleKeyEvent().
mumble mumble, il post era molto interessante; tra l'altro aggiungerei anche una cosa: questo è uno dei motivi percui non bisogna scrivere valori costanti nel codice, perché altrimenti non si capisce il loro significato; be' poi in realtà in questo caso sostituire i due valori con delle costanti non sarebbe stato affatto elegante, la soluzione migliore è come dici tu di fare in modo che ciascun metodo abbia il suo compito specifico.
be', non vorrei sembrare troppo "famelico" ^^' ma se non si propone nessun altro io sono disponibile domani sera :p
EDIT: molto sul tardi però, è meglio se lo fa qualcun altro.
DanieleC88
01-12-2005, 12:55
be', non vorrei sembrare troppo "famelico" ^^'
Tu SEI famelico. :Prrr:
Interessante questo thread, è bene che lo segua.
e va bene, qui sono l'unico che ha fame, cavolo eccheccavolo, mi mangio pure questo :D
allordunque, mumble, vediamo un po'...
anche io sto guardando :)
io invece non più perché mi sono state richieste degli aggiornamenti nel parser :|
Ok... Io sto imparando il codice dato che sono appena arrivato... mumble mumble :)
Hmmm mi viene in mente una soluzione per astrazione...
la classe KeyData potrebbe diventare abstract... Resterebbe invariata apparte il costruttore che diventerebbe KeyData() e verrebbe rimosso il campo columnOffset...
aggiungerei pure un metodo:
abstract void executeKey();
per ogni tasto che vogliamo gestire estendiamo la classe KeyData es:
private class LeftKey extends KeyData
{
public void executeKey()
{
Gem gem = getGemUnderControl();
removeGemFromGrid(gem);
gem.setCellColumn(gem.getCellColumn() + 1);
addGemToGrid(gem);
getGemUnderControl().move(-1 * xStep, 0);
}
}
A questo punto potremmo utilizzare una HashTable per associare KeyCode->KeyData
handleKeyEvent diventerebbe una cosa tipo:
public void handleKeyEvent(KeyEvent event)
{
KeyData keyData = hashTable.get(event.key());
keyData.execute();
}
Sicuramente è da migliorare assai e ci sarebbero altre mille ottimizzazioni... Voglio solo vedere se il concetto può piacervi :)
Un possibile miglioramento sarebbe evitare di mettere il codice dello spostamento della gemma dentro execute() e spostarlo dentro un metodo privato che sposti la gemma orizzontalmente di un dato "int offset"
Non so se fare il commit stasera o meno (con ant nessun problema ma c'è un po' di roba da sistemare)...
Nell'indecisione aspetto domani... Domani mattino/pome ci lavoro in facoltà e domani sera dovrei essere pronto per il commit :)
Credo che una parte improtante del refactoring da fare (oltre a quello che hai fatto te) fosse questa:
public int handleKeyEvent(KeyEvent event)
{
if(KeyCode.vk_Down == event.key())
{
return handleDownKey(event);
}
else
if(KeyCode.vk_Left == event.key())
{
return leftKey.handleEvent(event);
}
else
if(KeyCode.vk_Right == event.key())
{
return rightKey.handleEvent(event);
}
return 0;
}
Si deduce da questo:
"Siamo molto vicini ad una soluzione molto elegante, in cui aggiungere altri tasti non ci richieda una modifica a handleKeyEvent()."
Io una soluzione ce l'ho in mente ed una parte della soluzione è proprio quella di derivare la gestione dei vari tasti da una classe astratta...
Ok... Io sto imparando il codice dato che sono appena arrivato... mumble mumble :)
Come lo trovi in termini di leggibilita'?
Sicuramente è da migliorare assai e ci sarebbero altre mille ottimizzazioni... Voglio solo vedere se il concetto può piacervi :)
Un Command Pattern da manualetto :)
Sono indeciso se muoverci subito verso un command pattern perche' abbiamo solo tre comandi. Sicuramente in futuro ci servira', ma vorrei pagare la flessibilita' del command pattern quando ne abbiamo realmente bisogno.
Il tuo refactoring e' comunque la strada da seguire in futuro.
Come lo trovi in termini di leggibilita'?
Un Command Pattern da manualetto :)
Sono indeciso se muoverci subito verso un command pattern perche' abbiamo solo tre comandi. Sicuramente in futuro ci servira', ma vorrei pagare la flessibilita' del command pattern quando ne abbiamo realmente bisogno.
Il tuo refactoring e' comunque la strada da seguire in futuro.
Io passerei subito al Command tanto ci tocchera di sicuro farlo in futuro. Altri due o tre "comandi" e sforiamo il controllo sulla complessita e noi non lo vogliamo.
ciao ;)
Scusate, ma quali sono gli altri comandi che dobbiamo gestire in grid ?
Come lo trovi in termini di leggibilita'?
Hmmm abbastanza chiaro anche se a mio avviso si potrebbe rendere piu` chiara la separazione dei ruoli tra i vari metodi e classi (parlo di Grid e KeyEvent)
Un Command Pattern da manualetto :)
Sono indeciso se muoverci subito verso un command pattern perche' abbiamo solo tre comandi. Sicuramente in futuro ci servira', ma vorrei pagare la flessibilita' del command pattern quando ne abbiamo realmente bisogno.
Il tuo refactoring e' comunque la strada da seguire in futuro.
Stasera appena arrivo a casa faccio il commit del mio refactoring e do un'ochiata al Command Pattern sul libro che mi avevi consigliato (Design Patterns mi pare... dovrebbe esserci li` :p).
Scusate, ma quali sono gli altri comandi che dobbiamo gestire in grid ?
Jocchan?
Io passerei subito al Command tanto ci tocchera di sicuro farlo in futuro. Altri due o tre "comandi" e sforiamo il controllo sulla complessita e noi non lo vogliamo.
ciao ;)
Io invece aspetterei perche' non voglio fare ragionamenti tipo "tanto ci tocchera' di sicuro farlo in futuro". You Ain't Gonna Need it.
Secondo me dobbiamo fare questo refactoring quando il controllo sulla complessita' sforera'.
DanieleC88
02-12-2005, 12:25
Scusate, ma quali sono gli altri comandi che dobbiamo gestire in grid ?
Forse per ora non ce ne sono. Se in futuro venissero inserite anche combinazioni di tasti? Oppure tasti per mettere in pausa, togliere il sonoro, mostrare un menù... non saprei. :)
Scusate, ma quali sono gli altri comandi che dobbiamo gestire in grid ?
Jocchan all'inizio del progetto parlava della possibilità di fare uso della dinamite e lanciare massi nel campo avversario a comando. Queste sarebbero altre due cose da gestire in grid. Penso. Poi si era persino parlato di giocare in due sullo stesso campo contro il pc.
ciao ;)
- Due tasti (su e Space) per ruotare le coppie di gemme nei due sensi
- 5 tasti funzione (F1... F5) per attivare gli insulti sonori
- 3 tasti funzione (F6... F8) per muoversi nella soundtrack
- Un tasto (Ctrl) per attivare la dinamite nella modalità Advanced
- Un tasto (Shift) per usare le Desperation Moves nella modalità Advanced
- Un tasto (Esc) per mettere in pausa (nello story mode) e/o muoversi tra i menu
- Un tasto (Invio) per confermare nei menu e skippare i dialoghi
Eh sì, direi che ne useremo parecchi ;)
Non vedo l'ora di poter fare commit :p
- Due tasti (su e Space) per ruotare le coppie di gemme nei due sensi
Sì, ma solo questi vanno in Grid...comunque se sono previsti questi altri due tasti credo che tu possa anche fare il commit... Aspettiamo una conferma da fek...
Sì, ma solo questi vanno in Grid...comunque se sono previsti questi altri due tasti credo che tu possa anche fare il commit... Aspettiamo una conferma da fek...
Ok. Good to go.
ho fatto il commit del mio refactoring... A mio avviso c'è da sistemare ancora questo:
public void handleKeyRepetition(AbstractTimer timer)
{
if(leftKey.isReapeated(timer.getTime()) && !rightKey.isPressed())
{
leftKey.executeKey();
}
if(rightKey.isReapeated(timer.getTime()) && !leftKey.isPressed())
{
rightKey.executeKey();
}
}
Io aggiungerei dentro AbstractKeyData il campo:
AbstractKeyData excludingKey /* Forse il nome non è il massimo ma non mi viene in mente altro al momento...*/
excludingKey è il tasto che se premuto annulla l'effetto per quello corrente (es. excludingKey = rightKey per leftKey e viceversa).
si potrebbe poi implementare una lista di tasti che sono correntemente premuti (quando key.isPressed() viene aggiunto alla lista e quando !key.isPressed() viene rimosso)
a questo punto handleKeyRepetition non farebbe altro che chiamare executeKey() in tutti i tasti correntemente premuti (solo nel caso in cui il rispettivo excludingKey non sia premuto)
Onestamente tutta questa gestione dell'Input la separerei da Grid che sta cominciando ad essere abbastanza pesante :p
Onestamente tutta questa gestione dell'Input la separerei da Grid che sta cominciando ad essere abbastanza pesante :p
Si', Grid sta diventando troppo pesante e l'input va separato. Al punto in cui siamo adesso non dovrebbe essere troppo complesso, ma ci sono ancora alcuni dettagli che voglio vedere definiti prima di passare a questo refactoring. Le altre idee sono buone, ma suonano molto over engineering. Implementiamo solo strettamente quello che e' necessario.
YAGNI, non dimentichiamolo mai ;)
Hmmm ok... Visto che sto ancora imparando in questo metodo di sviluppo daresti un'occhiata al codice per vedere se ci sarebbero dei test da aggiungere per le modifiche che ho fatto? Mi sarebbe utile per capire meglio come ragionare nel preparare i test :)
Hmmm ok... Visto che sto ancora imparando in questo metodo di sviluppo daresti un'occhiata al codice per vedere se ci sarebbero dei test da aggiungere per le modifiche che ho fatto? Mi sarebbe utile per capire meglio come ragionare nel preparare i test :)
I test mi sembra che vadano bene. Faccio il commit adesso di un po' di refactoring, dagli un'occhiata.
Sì con queste modifiche è ancora più chiaro :)
ragazzi, non o se ve ne siete già accorti e mi scuso se sono ripetitivo (ho smesso ieri di seguire il thread :p), ma qualche vostro refactoring ha causato un evidente degradamento di performance... ^^'
provate a tenere premuta la freccia giù durante tutta la caduta di una gemma: il movimento risulta irregolare, tanto che certe volte sembra che la gravità torni al valore "normale" anziché a quello "accelerato"...
Non ho notato questo problema. Joc, puoi confermarlo?
E' possibile che si stiano creando troppi oggetti temporanei e il garbage collector faccia problemi. 71104, dai un'occhiata agli oggetti creati in Grid?
A me non da questo problema...
e che lo da solo a me?? :|
boh, provate a riprodurlo nelle mie stesse condizioni, cioè windows media player e firefox aperti e magari con numerose gemme già "dropped" nella griglia... vi assicuro che da me quando ho riempito la griglia circa a metà (forse poco più) le gemme rallentano visibilmente e hanno un movimento estremamente incostante, specie verso la fine. questo mi succede tenendo la freccia giù praticamente sempre premuta: la tengo quasi costantemente premuta perché secondo me aldilà di tutto le due velocità di caduta che abbiamo impostato nell'xml sono troppo basse: i valori ideali sono quelli dell'originale, che sono più alti dei nostri.
PS: domani provo a vedere meglio questa questione, magari facendo l'update a qualche revisione precedente e vedendo a partire da quale revisione mi si presenta il problema; nel frattempo ho committato una versione un po' migliorata del parser (adesso Jocchan può fare qualche esperimento col comando show in tutte le sue versioni, e domani proverò anche ad implementare altri comandi in versione di prova).
a proposito: domani non posso assolutamente fare il task con Daniele :cry:
ho troppo poco tempo e già devo fare queste cose; il task richiede imho almeno 1 ora a disposizione, che ho a malapena. perciò lo dovremo fare domenica se a Daniele va bene...
Molto probabilmente il task manager di Windows non è affidabile ma il processo del gioco mi occupa circa il 2% della CPU e 29MB di Memoria... Non mi sembra così probitivo... Domani appena trovo un po' di tempo ci do un'occhiata...
Sarà che ho un un PC mediamente pompato, ma ho provato a giocare con due finestre di Firefox aperte ed il trailer di MGS4 a 720p (un video bello tosto insomma) che mi scorreva di sotto, e non ho avuto nessun problema. Proverò a testare l'ultima build anche sul Pentium2... ma là non ci provo nemmeno a far partire un video a 720p :p
Il problema ti si presenta ancora?
Ho provato a fare un refactoring di Grid per eliminare la ripetizione (i vari gestori dei tasti esistono sia nel map che come membri privati)... Però chiaramente si complicano un po' le classi di gestione dei tasti... Ora i gestori si presentano in questo modo:
private class LeftKeyHandler extends AbstractKeyEventHandler
{
public LeftKeyHandler(KeyCode keyCode)
{
super(keyCode);
}
public void handleRepetition(long timeStamp, long repeatDelay)
{
if(!rightKey.isPressed())
super.handleRepetition(timeStamp, repeatDelay);
}
public void handle()
{
if(isPressed() && canMove(-1))
{
Gem gem = getGemUnderControl();
removeGemFromGrid(gem);
gem.setCellColumn(gem.getCellColumn() - 1);
addGemToGrid(gem);
getGemUnderControl().move(-xStep, 0);
}
}
}
Il codice che gestisce i tasti è diventato questo:
public void handleKeyEvent(KeyEvent event)
{
leftKey.handleEvent(event);
rightKey.handleEvent(event);
downKey.handleEvent(event);
}
public void handleKeyRepetition(AbstractTimer timer)
{
rightKey.handleRepetition(timer.getTime(), repeatDelay);
leftKey.handleRepetition(timer.getTime(), repeatDelay);
}
ora non si presenta più, dev'essere stato per forza merito di cionci; se si presentasse ancora ve lo farò sapere.
Hmmm... non e' male. Mi piace.
C'e' qualcosa che ancora non mi convince. Secondo me il concetto di "Key" dovrebbe essere astratto in un'altra classe e separato dall'handler. Ma non mi e' ancora chiaro come.
ora non si presenta più, dev'essere stato per forza merito di cionci; se si presentasse ancora ve lo farò sapere.
non ho fatto niente :D
Ma con queste modifiche hai di nuovo riproposto il problema:
Chi si vuole occupare di separare i due comportamenti e rifattorizzare ulteriormente questo codice? Siamo molto vicini ad una soluzione molto elegante, in cui aggiungere altri tasti non ci richieda una modifica a handleKeyEvent().
Aggiungere un tasto ora richiede di nuovo di modificare handleKeyEvent...
A parte che avevo rotto la build :stordita: Credevo di aver già lanciato ant...invece mi ero sbagliato :fagiano:
Ora bisogna modifcare handleKeyEvent se si aggiunge un tasto, ma se i tasti da aggiungere sono solo due ;) In questo modo però ho tolto la duplicazione...
non ho fatto niente :D :mbe:
allora era il mio computer ieri che scazzava... :mbe:
magari era colpa del media player... :mbe:
lol :D
Infatti hai ragione non chiede alcuna modifica in AbstractKeyEventHandler ma la richiede di nuovo qui:
public void handleKeyEvent(KeyEvent event)
{
leftKey.handleEvent(event);
rightKey.handleEvent(event);
downKey.handleEvent(event);
}
Ho messo la HashMap perchè era stato chiesto di non dover più modificare quel metodo di Grid anche nel caso di aggiunta di tasti nuovi :)
magari era colpa del media player... :mbe:
Il Media Player ha sempre fatto più danni delle guerre e del femminismo :stordita:
Ho messo la HashMap perchè era stato chiesto di non dover più modificare quel metodo di Grid anche nel caso di aggiunta di tasti nuovi :)
Ti spiego perchè ho tolto l'HashMap... Quella funzione era facilmente gestibile con l'hashmap...ma l'altra non lo era...
Anzi...vediamo un po' provo a reintrodurre l'hashmap...
Sì è vero...
Si potrebbe scorrere tutta la HashMap per esempio prendendone il set di tutte le Key con hashMap.keySet(), prendere i vari KeyEventHandler e chiamare handleRepetition(...) su ognuno
Ma non tutti i tasti necessitano di ripetizione (vedi Down)...
Ho messo la funzione in questo modo:
public void handleKeyRepetition(AbstractTimer timer)
{
eventMap.get(KeyCode.vk_Left).handleRepetition(timer.getTime(), repeatDelay);
eventMap.get(KeyCode.vk_Right).handleRepetition(timer.getTime(), repeatDelay);
}
Si potrebbe trovare il modo di distinguere i tasti che necessitano di ripetizione ed altri che invece non ne necessitano... Di primo acchito mi viene in mente che Left e Right (questi due per ora) potrebbero implementare un'interfaccia apposita o vedi tu :)
Ho fatto il commit, controlla se ti piace...
Di primo acchito mi viene in mente che Left e Right (questi due per ora) potrebbero implementare un'interfaccia apposita o vedi tu :)
Sinceramente non ne vedo la necessita... I tasti non vanno aggiunti dinamicamente e il numero è abbastanza piccolo... Quindi si può benissimo fare la lista della spesa in quella funzione ;)
Visto :)
Mi sarebbe piaciuto riuscire ad escludere l'aggiunta anche nella ripetizione ma hai ragione probabilmente saranno pochi i tasti di quel tipo :)
Si potrebbe trovare il modo di distinguere i tasti che necessitano di ripetizione ed altri che invece non ne necessitano... Di primo acchito mi viene in mente che Left e Right (questi due per ora) potrebbero implementare un'interfaccia apposita o vedi tu :)
You Aren't Gonna Need It
You Aren't Gonna Need It
You Aren't Gonna Need It
You Aren't Gonna Need It
Ragazzi, state facendo overengineering adesso. Non ci serve piu' flessibilita', ci serve solo semplificare Grid non complicarla.
Ragazzi, state facendo overengineering adesso. Non ci serve piu' flessibilita', ci serve solo semplificare Grid non complicarla.
Anche l'ultima modifica che ho fatto ? Controlla se puoi... Sicuramente permette di semplificare gli handler e non rende più complicata la gestione...
DanieleC88
03-12-2005, 11:54
perciò lo dovremo fare domenica se a Daniele va bene...
Va benissimo. Tanto dobbiamo anche aspettare il 6.1.3. ;)
Anche l'ultima modifica che ho fatto ? Controlla se puoi... Sicuramente permette di semplificare gli handler e non rende più complicata la gestione...
Sto guardando ora. La tua modifica e' ok, da li' sono partito per un'altra semplificazione. Il mio obiettivo e' tirare fuori gli handler dalla classe Grid perche' non voglio che abbiano visibilita' su tutta Grid, ma solo sulla sua interfaccia pubblica. Sto seguendo il principio di passare ad ogni oggetto il minimo di informazioni necessario affinche' svolga il suo compito.
Sto seguendo il principio di passare ad ogni oggetto il minimo di informazioni necessario affinche' svolga il suo compito.
Me lo stavo immaginavo :)
Va benissimo. Tanto dobbiamo anche aspettare il 6.1.3. ;)
Domanda: non è più pratico intanto iniziare subito i task in pair programming? Così chi dei due ha tempo intanto già posta qualcosa, e parte del lavoro domenica sarà già pronto.
Non mi riferisco a questo caso in particolare, dato che serve prima il 6.1.3, però potrebbe essere utile intanto iniziare il task e buttare giù qualche test.
DanieleC88
03-12-2005, 13:52
Domanda: non è più pratico intanto iniziare subito i task in pair programming? Così chi dei due ha tempo intanto già posta qualcosa, e parte del lavoro domenica sarà già pronto.
Non mi riferisco a questo caso in particolare, dato che serve prima il 6.1.3, però potrebbe essere utile intanto iniziare il task e buttare giù qualche test.
Si che può essere utile. Però senza il task 6.1.3 dovremmo andare un po' "alla cieca" nello scrivere i test.
Dicevo infatti in generale, e non in questo caso :D
Questo perchè ho notato che spesso ci si dà appuntamento a giorno X per iniziare (data che comunque può sempre saltare), mentre credo sarebbe più pratico evitare di farlo ;)
DanieleC88
03-12-2005, 15:24
Dicevo infatti in generale, e non in questo caso :D
Ah scusa. :D
Comunque si, hai ragione. ;)
vBulletin® v3.6.4, Copyright ©2000-2025, Jelsoft Enterprises Ltd.