[CICLO 4] Test Driven Development task 4.2.2 (cdimauro vs 71104)

[71104]

come al solito i commit saranno precari per un po'.

dunque, test list:
- test per coda vuota
- test per coda non vuota
- test per rimozione di un elemento dalla coda (anche più d'uno)
- test per ordine corretto degli elementi della coda (anche più d'uno)
- test per la correttezza del tipo di evento (pressione o rilascio)
- test per i timestamp
- varie ed eventuali...

allora, il test per la coda vuota è molto semplice:

public void testQueueEmpty()
{
	assertTrue(input.isQueueEmpty());
}

che è talmente semplice che faccio anche quello per la coda non vuota

public void testQueueNotEmpty()
{
	input.generateKey(KeyCode.vk_Left);
	assertFalse(input.isQueueEmpty());
}

cdimauro, questi due come li risolveresti?

[cdimauro]

Per il primo test avrei risolto semplicemente aggiungendo il seguente codice alla classe input:

    public boolean isQueueEmpty()
    {
      return true;
    }

Aggiungendo il secondo test, mi vedo costretto a introdurre la variabile d'istanza empty a Input e a modificare un costruttore un po' di methodi:

    private boolean empty;

    private Input(AbstractKeyboard keyboard)
    {
        this.keyboard = keyboard;
        keys = new BitSet();
        empty = true;
    }

    public boolean isQueueEmpty()
    {
      return empty;
    }

    public void generateKey(KeyCode key)
    {
        keys.set(key.value(), true);
        empty = false;
    }

Il minimo indispensabile per far passare i test...

A te la palla Alberto.

[71104]

ok, a questo punto già mi rendo conto di aver dimenticato un'aspetto importante della gestione della coda: la rimozione di elementi. l'inserzione avviene quando vengono ricevuti eventi dalla tastiera, ma la rimozione? quindi siccome l'operazione di rimozione ci serve, aggiungo questo test:

public void testQueueItemRemoval()
{
	input.generateKey(KeyCode.vk_Right);
	assertTrue(input.extractKey() == KeyCode.vk_Right);
}

l'operazione di peek casomai la aggiungeremo dopo (non è neanche detto che serva).

PS: ho editato il post iniziale

[cdimauro]

Per far passare questo test introduco un'altra variabile, che mi serve per ricordare l'ultimo tasto "premuto", e aggiungo anche il metodo extractKey() :

    private KeyCode lastKey;

    public void generateKey(KeyCode key)
    {
        keys.set(key.value(), true);
        empty = false;
        lastKey = key;
    }


    public KeyCode extractKey()
    {
        empty = true;
        return lastKey;
    }

A questo punto scrivo io il test per il punto seguente, che è:
" test per ordine corretto degli elementi della coda (anche più d'uno)".

    public void testQueueSequenceOrder()
    {
      input.generateKey(KeyCode.vk_Right);
      input.generateKey(KeyCode.vk_Up);
      assertTrue(input.extractKey() == KeyCode.vk_Right);
      assertTrue(input.extractKey() == KeyCode.vk_Up);
    }

A te Alberto

[71104]

ok, scusate torno tra una mezz'ora, vado a cena ^^

[cionci]

Posso muovere un dubbio ? Siamo sicuri che serva la rimozione degli eventi dalla coda ? Perchè invece non realizzate una coda circolare di N elementi...
Anche perchè...chi si mette a rimuovere gli eventi dalla coda ? Nel senso che gli utilizzatori di input non possono sapere se lo stesso input servirà anche ad altri quindi la politica di rimozione diverrebbe molto complessa...

[cionci]

Ah...muovete i test che state scrivendo in it.diamonds.tests.ignore altrimenti la build fallisce ed il repository non deve mai stare con la build rotta...

[71104]

allora, l'ultimo test di cdimauro introduce la triangolazione sul campo dell'ultimo tasto premuto, nel senso che potrei risolverlo aggiungendo un'altro membro analogo a lastKey, ma per triangolazione ne aggiungo un numero virtualmente illimitato creando la coda
per risolvere il test ho aggiunto un campo next a KeyCode (inizializzato a null) e ho modificato il codice di generateKey ed extractKey come segue:

    public void generateKey(KeyCode key)
    {
        keys.set(key.value(), true);
        empty = false;
        if (null != queue)
        {
            queue.next = key;
        }
        else {
            queue = key;
        }
    }


    public KeyCode extractKey()
    {
        KeyCode result = queue;
        if (null != queue)
        {
            queue = queue.next;
            result.next = null;
        }
        empty = (null != queue);
        return result;
    }

(tra parentesi il campo lastKey è ora diventato queue, nome più significativo in questo caso).
cionci, la coda circolare è inutile da implementare, per noi sarebbe solo codice in più, potenzialmente sbagliato, e la rimozione serve per forza: la dovrà usare reactToInput per lavorare sugli input (senza contare che serve necessariamente per testare il corretto ordine degli elementi nella coda).

PS: ovviamente poi dopo aver sistemato reactToInput dovremo anche eliminare la gestione degli input tramite bitset, e questo sarà un refactoring che faremo alla fine.
dal momento che si tratta quasi esclusivamente di rimuovere codice, non sarà necessario usare altri test durante il refactoring, l'importante è controllare ad ogni piccolo cambiamento che i test attuali continuino a passare tutti.

[cdimauro]

Originariamente inviato da cionci

Posso muovere un dubbio ? Siamo sicuri che serva la rimozione degli eventi dalla coda ? Perchè invece non realizzate una coda circolare di N elementi...
Anche perchè...chi si mette a rimuovere gli eventi dalla coda ? Nel senso che gli utilizzatori di input non possono sapere se lo stesso input servirà anche ad altri quindi la politica di rimozione diverrebbe molto complessa...

L'input dovrebbe essere associato a un ben determinato oggetto, che lo "consumerà". E quindi non dovrebbe essere accessibile ad altri oggetti.

Per quanto riguarda la coda circolare, se c'è già una classe Java disponibile non c'è problema. Io non conosco la libreria di Java, quindi non so. Altrimenti è meglio prendere un Vector e implementare la coda in questo modo.

Il tutto IMHO, chiaramente.

[71104]

ragazzi, tra imprevisti rotture di OO ed errori nostri nel codice questo task sta durando una vita!!!
allora, la mia implementazione dei nodi della queue non va bene, e per mettere in evidenza l'errore concettuale basta questo test:

        input.generateKey(KeyCode.vk_Right);
        input.generateKey(KeyCode.vk_Up);
        input.generateKey(KeyCode.vk_Left);
        input.generateKey(KeyCode.vk_Up);
        assertTrue(input.extractKey() == KeyCode.vk_Right);
        assertTrue(input.extractKey() == KeyCode.vk_Up);
        assertTrue(input.extractKey() == KeyCode.vk_Left);
        assertTrue(input.extractKey() == KeyCode.vk_Up);

il problema è che il Java non gestisce le enum con istanze e puntatori come gli oggetti, cioè se io inserisco due volte un vk_Right io di fatto non ho inserito due oggetti diversi, ma ho inserito due volte la stessa enum, quindi il campo next di vk_Right all'ultima inserzione viene modificato in maniera errata.
ora stiamo cercando la soluzione: sarà necessario implementare la coda diversamente.

[71104]

ok, ho risolto il problema grazie a cdimauro; scusate il ritardo ma ho avuto un ulteriore contrattempo, come se non bastassero... -_-'
ecco qua le ultime modifiche:

    public void generateKey(KeyCode key)
    {
        keys.set(key.value(), true);
        empty = false;
        queue.add(key);
    }


    public KeyCode extractKey()
    {
        KeyCode result = queue.remove();
        empty = queue.isEmpty();
        return result;
    }

ed inoltre il campo queue è diventato di tipo LinkedList<KeyCode>.

a questo punto la prima cosa che vedo è che c'è una duplicazione perché l'informazione di lista vuota è data sia dal campo empty sia da queue.isEmpty()...

[Jocchan]

Originariamente inviato da 71104

a questo punto la prima cosa che vedo è che c'è una duplicazione perché l'informazione di lista vuota è data sia dal campo empty sia da queue.isEmpty()...

Una volta accertato che si tratta di una duplicazione, e che l'eliminazione di uno dei due elementi non comporta nessun danno, sai bene cosa bisogna fare

[cdimauro]

Ho pensato io al refactoring.

Ho eliminato empty da tutte le parti e l'ho rimpiazzato soltanto in isQueueEmpty:

    public boolean isQueueEmpty()
    {
      return queue.size() == 0;
    }

[71104]

ok, adesso il prossimo test (che testa il tipo di evento, cioè pressione o rilascio del tasto) richiede un po' di refactoring dei test precedenti:

    public void testQueueItemRemoval()
    {
        input.generateKey(KeyCode.vk_Right);
        assertTrue(input.extractKey().key() == KeyCode.vk_Right);
    }


    public void testQueueSequenceOrder()
    {
        input.generateKey(KeyCode.vk_Right);
        input.generateKey(KeyCode.vk_Up);
        assertTrue(input.extractKey().key() == KeyCode.vk_Right);
        assertTrue(input.extractKey().key() == KeyCode.vk_Up);
    }
    
    
    public void testEventType()
    {
        input.generateKey(KeyCode.vk_Left, true);
        input.generateKey(KeyCode.vk_Left, false);
        assertTrue(input.extractKey().state());
        assertFalse(input.extractKey().state());
    }

testEventType è il nuovo test; ho assunto che la nuova classe KeyEvent che costituisce un nodo della lista abbia un metodo public state() che restituisce il tipo di evento (boolean quindi) e un metodo key() che restituisce il KeyCode.
la sequenza generata in questo test è semplice, pressione e rilascio di un tasto, ma poi si potrebbe fare un ulteriore test più robusto per testare combinazioni più complesse (casomai dopo però).

EDIT: ho cambiato il nome del metodo type() in state().

[Jocchan]

Originariamente inviato da 71104

la sequenza generata in questo test è semplice, pressione e rilascio di un tasto, ma poi si potrebbe fare un ulteriore test più robusto per testare combinazioni più complesse (casomai dopo però).

Mi raccomando facciamolo, stiamo implementando questa coda proprio per le situazioni più concitate quindi meglio assicurarci che tutto vada per il meglio

[71104]

Originariamente inviato da Jocchan

Mi raccomando facciamolo, stiamo implementando questa coda proprio per le situazioni più concitate quindi meglio assicurarci che tutto vada per il meglio

hai ragione

[cdimauro]

Per far passare il test ho implementato una classe KeyEvent in input, che serve a contenere l'informazione relativa al tasto premuto e al suo stato, modificando le parti di input che prima usavano KeyCode:

    public class KeyEvent
    {
        private KeyCode keyCode;
        
        private boolean keyState;
        

        public KeyEvent(KeyCode keyCode, boolean keyState)
        {
            this.keyCode = keyCode;
            this.keyState = keyState;
        }
        

        public KeyCode key()
        {
            return keyCode;
        }
        

        public boolean state()
        {
           return keyState;   
        }
    
    }


    private LinkedList<KeyEvent> queue;


    private Input(AbstractKeyboard keyboard)
    {
        this.keyboard = keyboard;
        keys = new BitSet();
        queue = new LinkedList<KeyEvent>();
    }


    public void generateKey(KeyCode key, boolean state)
    {
        keys.set(key.value(), state);
        queue.add(new KeyEvent(key, state));
    }


    public void generateKey(KeyCode key)
    {
        generateKey(key, true);
    }


    public KeyEvent extractKey()
    {
        return queue.remove();
    }

Inoltre adesso generateKey(KeyCode key) utilizza generateKey(KeyCode key, boolean state), dove ho "centralizzato" tutto il codice.

Qui e in qualche altra parte si potrà effettuare del refactoring, ma per adesso m'interessa finire: il letto mi aspetta.

[cdimauro]

Rimane da associate un timestamp a ogni evento generato. Il che vuol dire accertarsi che un tasto premuto prima NON possa avere un timestamp
maggiore di un tasto premuto dopo:

    public void testEventTimestamp()
    {
        input.generateKey(KeyCode.vk_Left, true);
        input.generateKey(KeyCode.vk_Left, false);
        assertTrue(input.extractKey().getTimestamp() < input.extractKey().getTimestamp());
    }

A te Alberto.

P.S. Spero che Java non sia come il C, per cui un compilatore può processare liberamente gli argomenti di una funzione/metodo, altrimenti bisognerà modificare l'assert mettendo il primo timestamp in una variabile temporanea.

[71104]

allora, ho risolto il test mettendo un campo timestamp in KeyEvent con relativo getter (il "setter" in questo caso sarebbe il costruttore, al quale ho aggiunto un parametro); ma c'è un piccolo dettaglio da notare: pur essendo il codice di implementazione corretto, il test inizialmente non passava perché i due timestamp risultavano identici, e non strettamente l'uno minore dell'altro; ho inserito tra le due inserzioni una piccola sleep da 100 ms, soluzione secondo me migliore del cambiare il simbolo < con <= (come proponeva cdimauro) in quanto il simbolo <= potrebbe rendere inutile il test (non testiamo realmente il caso, più realistico, in cui il primo timestamp è minore del secondo; cambiando le cose in teoria potrebbe pure saltare fuori l'errore che in realtà avevo sbagliato a scrivere il codice e quindi mettendo la sleep in realtà i due timestamp risultavano invertiti, o cose del genere, ma il test non lo evidenziava).
ecco come è diventato il test:

    public void testEventTimestamp() throws InterruptedException
    {
        input.generateKey(KeyCode.vk_Left, true);
        Thread.sleep(100);
        input.generateKey(KeyCode.vk_Left, false);
        assertTrue(input.extractKey().getTimestamp() < input.extractKey().getTimestamp());
    }

ed ecco il codice della classe KeyEvent:

    public class KeyEvent
    {
        private KeyCode keyCode;
        
        private boolean keyState;
        
        private long timestamp;
        

        public KeyEvent(KeyCode keyCode, boolean keyState, long timestamp)
        {
            this.keyCode = keyCode;
            this.keyState = keyState;
            this.timestamp = timestamp;
        }
        

        public KeyCode key()
        {
            return keyCode;
        }
        

        public boolean state()
        {
           return keyState;   
        }
        
        public long getTimestamp()
        {
            return timestamp;
        }
    
    }

a nanna
anzi, io non ancora, voglio aggiungere quell'ultimo test della sequenza di input complessa...

[cdimauro]

OK. Per adesso va bene: vado a nanna che il letto mi aspetta da almeno 3 ore :-D

Ci sono un po' di refactoring da fare, un po' di rename di metodi / attributi, e dovremmo discutere di qualche dettaglio come faceva notare Alberto.

A domani. Buonanotte a tutti