Refactoring e l'importanza del codice di qualita'

[fek]

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

[71104]

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

EDIT: molto sul tardi però, è meglio se lo fa qualcun altro.

[DanieleC88]

Originariamente inviato da 71104

be', non vorrei sembrare troppo "famelico" ^^'

Tu SEI famelico.

Interessante questo thread, è bene che lo segua.

[71104]

e va bene, qui sono l'unico che ha fame, cavolo eccheccavolo, mi mangio pure questo
allordunque, mumble, vediamo un po'...

[Ufo13]

anche io sto guardando

[71104]

io invece non più perché mi sono state richieste degli aggiornamenti nel parser :|

[Ufo13]

Ok... Io sto imparando il codice dato che sono appena arrivato... mumble mumble

[Ufo13]

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"

[Ufo13]

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

[cionci]

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

[fek]

Originariamente inviato da Ufo13

Ok... Io sto imparando il codice dato che sono appena arrivato... mumble mumble

Come lo trovi in termini di leggibilita'?

Originariamente inviato da Ufo13

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.

[VICIUS]

Originariamente inviato da fek

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

[cionci]

Scusate, ma quali sono gli altri comandi che dobbiamo gestire in grid ?

[Ufo13]

Originariamente inviato da fek

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

[fek]

Originariamente inviato da cionci

Scusate, ma quali sono gli altri comandi che dobbiamo gestire in grid ?

Jocchan?

Originariamente inviato da VICIUS

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]

Originariamente inviato da cionci

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.

[VICIUS]

Originariamente inviato da cionci

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

[Jocchan]

- 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

[Ufo13]

Non vedo l'ora di poter fare commit

[cionci]

Originariamente inviato da Jocchan

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