Programma "Refactor This!" [Da leggere]

[jappilas]

Originariamente inviato da VICIUS

E visto che ci siamo in questo momento ho messo la funzione shutDown() che spegne tutto in GameLoop ma mi accorgo solo ora che forse starebbe meglio in Game che è la classe che si preoccupa di creare tutto.

io la metterei in Environment (togliendo la chiamata a stopMusic() che ricade nella procedura di uscita dal game loop) , comunque chiamata alla fine da Game.start() (meglio forse sarebbe run()) per simmetria rispetto alla creazione dell' environment

[fek]

Originariamente inviato da ^TiGeRShArK^

fatto..
il moltiplicatore * 2 serviva perchè il movimento verso il basso veniva calcolato facendo gravity / 2...
Però praticamente tutti i test passavano lo stesso tranne un paio in cui veniva calcolata la posizione della cella al pixel.
Ora passa tutto ed è un bel pò + leggibile e soprattutto si capisce meglio cosa fanno quei test

Ottimo!

Originariamente inviato da ^TiGeRShArK^

mmm..
è normale che la build machine dica:

cmq ora mi sa che vado al lavoro....

Boh, devo fare un clean di CC.

[thebol]

Originariamente inviato da VICIUS

Avevo notato anche io questa cosa durante il refactoring ed ero arrivato alla stessa soluzione. Sono praticamente sicuro di aver fatto il commit dopo aver estratto il corpo del loop in una funzione esterna di nome loopStep. No basta usare quella invece di loopOneStep?

loopStep == loopOneStep, ero solo pigro ad andare a vedere il nome esatto

Cavolo ecco come faceva!
Quindi in questo momento la variabile quit rappresenta sia l'uscita dal gioco che l'uscita dal loop. Non ci conviene gestire i due casi separatamente per rendere più chiara la gestione dei loop? E visto che ci siamo in questo momento ho messo la funzione shutDown() che spegne tutto in GameLoop ma mi accorgo solo ora che forse starebbe meglio in Game che è la classe che si preoccupa di creare tutto.

Ora come ora game è solo un "lanciatore" di loop. Per cui solo i loop possono essere avviati e spenti (chiamando la exitLoop di solito da un event). In più controlla a ogni loop che l'engine non sia in shutdown, cosa che capita quando si chiude la finestra.

Forse si potebbe fare che sia engine che quando viene chiamata la sua closeWindow() notifichi un evento di quit al loop che poi lo processerà. Bisogna vedere se sta in piedi.

ps il mio test del loop ha qualche problema, il thread rimane attivo fino a quando la suit di test non viene completata. Non crea problemi, però è brutto

sto testando un altra soluzione, vi faro sapere

[thebol]

ecco la nuova versione

    public void testLoop()
    {
        long timestamp = environment.getTimer().getTime();        

        ((MockTimer)environment.getTimer()).sleepOnAdvance(true);

        Runnable task = new Runnable() 
        {
            public void run()
            {
                loop.loop();
            }
        };

        Thread thread = new Thread(task);
        thread.start();
        
        try
        {
            Thread.sleep(100);
        }
        catch (InterruptedException e)
        {
            ;
        }
        
        loop.exitLoop();
        
        try
        {
            thread.join();
        }
        catch (InterruptedException e)
        {
            ;
        }
        assertTrue(timestamp + 2 < environment.getTimer().getTime());
        long numberOfAdvanceTime = environment.getTimer().getTime() - timestamp;
        assertEquals("The number of advance in timer must be equal to number of updateState in loop", numberOfAdvanceTime, mockLoop.getNumStateUpdated());
    }

in particolare testa che il loop abbia girato almeno + di 2 volte e che il numero di volte che il timer è avanzato sia uguale al numero di volte che è stata chiamata la updateState sul loop

Ho provato anche a farlo fallire (facendo un loop infinito) e con il fork attivato in junit il test fallisce per timeout(impostabile da xml il default mi sembra di aver letto 18 sec).

Con il fork disattivato invece il test rimane inloopato ma forse chiamando una join con timeout si può risolvere

[thebol]

    public void testLoop()
    {
        long timestamp = environment.getTimer().getTime();        

        ((MockTimer)environment.getTimer()).sleepOnAdvance(true);

        Runnable task = new Runnable() 
        {
            public void run()
            {
                loop.loop();
            }
        };

        Thread thread = new Thread(task);
        thread.start();
        
        try
        {
            Thread.sleep(100);
        }
        catch (InterruptedException e)
        {
            ;
        }
        
        loop.exitLoop();    
        
        try
        {
            thread.join(20000);
        }
        catch (InterruptedException e)
        {
            ;
        }
        
        if (thread.isAlive())
        {
            fail("The thread must no be alive");
            thread.stop();
        }
        
        assertTrue(timestamp + 2 < environment.getTimer().getTime());
        long numberOfAdvanceTime = environment.getTimer().getTime() - timestamp;
        assertEquals("The number of advance in timer must be equal to number of updateState in loop", numberOfAdvanceTime, mockLoop.getNumStateUpdated());
    }

questo codice funziona anche con la fork disabilitata in junit. L'unico problema e la lunghezza di questo test(ma è un eccezione di test), e la chiamata thread.stop() che è deprecata e da warning a tempo di compilazione(e deprecata perchè per come sono le specifiche della jvm non si può garantire che un thread venga veramente stoppato, per cui si consiglia di usare una variabile volatile da controllare nel ciclo del loop per stoppare il thread).

Si può evitare la chiamata, ma si lascia un thread girare a vuoto fino alla fine del test(può essere accettabile).

L'unico problema potrebbe venire dal java memory model. Infatti 2 thread che accedono alla stessa variabile, non hanno la garanzia di vedere lo stesso stato della variabile, a meno che l'accesso non sia sincronizzato o la variabile sia volatile. Ora queste sono problematiche che raramente accadono in scenari mono processore(di solito capitano in scenari in cui ci sono cluster di cpu) per cui il problema e molto remoto.

L'alternativa sarebbe modificare quit all'interno di abstarctLoop rendendola volatile, però si cambierebbe l'implementazione per venire incontro ai test :\

[Bonfo]

Originariamente inviato da thebol

L'alternativa sarebbe modificare quit all'interno di abstarctLoop rendendola volatile, però si cambierebbe l'implementazione per venire incontro ai test :\

Il che in TDD è più che normale!
In ogni caso la cosa si sta mostrando "succhia" tempo e in qualche modo non KISS. Purtroppo ora non mi vengono in mente molte idee...

[Bonfo]

Originariamente inviato da ^TiGeRShArK^

fatto..
il moltiplicatore * 2 serviva perchè il movimento verso il basso veniva calcolato facendo gravity / 2...
Però praticamente tutti i test passavano lo stesso tranne un paio in cui veniva calcolata la posizione della cella al pixel.
Ora passa tutto ed è un bel pò + leggibile e soprattutto si capisce meglio cosa fanno quei test

Dell x2 ne avevamo già parlato quando stavamo mettendo a posto canMoveWithFullGravity(), che a proposito è ancora sbagliata perchè supporta solo movimenti <= 1Cel.
Attenzione, se la gravità cirtica era 20, era necessario scrivere 20 * 2. Se ora hai messo 20, il che vuol dire gravità effettiva 10, il test passa ancora ( in quanto sei ancora dentro illimite), ma il primo che va a modificare il codice può andare a infilare un bug senza accorgersene, in quanto non c'è il test sul caso limite.

in ogni caso non modificare i test di nuovo, ma elimino semplicemente il / 2 nei conti.... tanti alla fine è una costante, basta andare a modifcare i valori nel config dividendoli per 2.

Alla faccia del costumer

[Bonfo]

L'operazione di rimozione del /2 mi sta facendo notare alcune cose:
1) Molti test controllano la stessa cosa, solo a livelli diversi. Ad esempio il controllo del corretto movimento di gem avviene a livello di Droppable, di Grid e di GRidController. Significa che l'integrazione è corretta, m anon ci complica un po' la vita??
2) Molti test sono gravity-dependant, ovvero il numro di update era stato calibrato per ottenere il test corretto...
Non avendo ancora cambiato il config, molti test falliscono senza ragionevoli motivi... tranne il fatto che ora il numero di update vanno dimezzati

[^TiGeRShArK^]

Originariamente inviato da thebol

    public void testLoop()
    {
        long timestamp = environment.getTimer().getTime();        

        ((MockTimer)environment.getTimer()).sleepOnAdvance(true);

        Runnable task = new Runnable() 
        {
            public void run()
            {
                loop.loop();
            }
        };

        Thread thread = new Thread(task);
        thread.start();
        
        try
        {
            Thread.sleep(100);
        }
        catch (InterruptedException e)
        {
            ;
        }
        
        loop.exitLoop();    
        
        try
        {
            thread.join(20000);
        }
        catch (InterruptedException e)
        {
            ;
        }
        
        if (thread.isAlive())
        {
            fail("The thread must no be alive");
            thread.stop();
        }
        
        assertTrue(timestamp + 2 < environment.getTimer().getTime());
        long numberOfAdvanceTime = environment.getTimer().getTime() - timestamp;
        assertEquals("The number of advance in timer must be equal to number of updateState in loop", numberOfAdvanceTime, mockLoop.getNumStateUpdated());
    }

questo codice funziona anche con la fork disabilitata in junit. L'unico problema e la lunghezza di questo test(ma è un eccezione di test), e la chiamata thread.stop() che è deprecata e da warning a tempo di compilazione(e deprecata perchè per come sono le specifiche della jvm non si può garantire che un thread venga veramente stoppato, per cui si consiglia di usare una variabile volatile da controllare nel ciclo del loop per stoppare il thread).

Si può evitare la chiamata, ma si lascia un thread girare a vuoto fino alla fine del test(può essere accettabile).

L'unico problema potrebbe venire dal java memory model. Infatti 2 thread che accedono alla stessa variabile, non hanno la garanzia di vedere lo stesso stato della variabile, a meno che l'accesso non sia sincronizzato o la variabile sia volatile. Ora queste sono problematiche che raramente accadono in scenari mono processore(di solito capitano in scenari in cui ci sono cluster di cpu) per cui il problema e molto remoto.

L'alternativa sarebbe modificare quit all'interno di abstarctLoop rendendola volatile, però si cambierebbe l'implementazione per venire incontro ai test :\

ehmm...
sto un pò fuso ora come ora..
ma anzichè usare un join non era meglio mettere nel thread un while(condition) con condition di tipo volatile boolean settabile a piacere a true o a false da un metodo synchronized?

[^TiGeRShArK^]

Originariamente inviato da Bonfo

Dell x2 ne avevamo già parlato quando stavamo mettendo a posto canMoveWithFullGravity(), che a proposito è ancora sbagliata perchè supporta solo movimenti <= 1Cel.
Attenzione, se la gravità cirtica era 20, era necessario scrivere 20 * 2. Se ora hai messo 20, il che vuol dire gravità effettiva 10, il test passa ancora ( in quanto sei ancora dentro illimite), ma il primo che va a modificare il codice può andare a infilare un bug senza accorgersene, in quanto non c'è il test sul caso limite.

in ogni caso non modificare i test di nuovo, ma elimino semplicemente il / 2 nei conti.... tanti alla fine è una costante, basta andare a modifcare i valori nel config dividendoli per 2.

Alla faccia del costumer

doh...
io ora ho sostituito tutti i Cell.SIZE * 2 con normalGravity ..
..e dove era prevista la stronger o la strongest gravity settavo il metodo corrispondente.
Nei test con il controllo della posizioen al pixel avevo correttamente effettuato il controllo ponendo yStep pari a actualGravity / 2, ma nei test dove non era previsto il controllo al pixel non l'ho fatto..

[Bonfo]

Il CELL_SIZE * 2 era proprio per settare il valore limite (che è assolutamente diverso dal valore di normal gravity) almeno quelli prova a revertarli!

Ora comunque capisco perchè sto test non fa più quello che dice

    public void testBottomCollision()
    {
        grid.setNormalGravity();
        grid.insertDroppable(gem1, 12, 4);

        float oldYPosition = gem1.getAnimatedSprite().getSprite().getPosition().getY();

        grid.updateDroppable(gem1);
        grid.updateDroppable(gem1);

        Cell cell = Cell.create(13, 4);
        assertTrue(grid.isDroppableAt(cell));

        float newYPosition = grid.getDroppableAt(cell).getAnimatedSprite().getSprite().getPosition().getY();
        assertEquals(oldYPosition + grid.getActualGravity() , newYPosition, 0.0001f);
    }

infatti non collide assolutamente col fondo.

[fek]

Puoi correggerlo o integrarlo con un altro test?

[thebol]

Originariamente inviato da ^TiGeRShArK^

ehmm...
sto un pò fuso ora come ora..
ma anzichè usare un join non era meglio mettere nel thread un while(condition) con condition di tipo volatile boolean settabile a piacere a true o a false da un metodo synchronized?

perchè meglio? Lancio il thread, aspetto che il thread con il loop finisca con timeout, e poi verifico se è finito o no (a seconda di cosa mi aspetto).

cmq sono riuscito a fare tutti i test ed a eliminare il warning sulla thread.stop con un annotation.

Stasera committo

[Ufo13]

Sono tornato ieri notte. In questi giorni estraggo la classe GameTurn =)

[Bonfo]

Originariamente inviato da fek

Puoi correggerlo o integrarlo con un altro test?

No, in realta' bisognerebbe revertare tutti i Cell.SIZE_IN_PIXELS * 2 e valutare caso per caso dove abbia senso mettere NormalGravity e dove lasciare Cell.SIZE_IN_PIXELS * 2, che poi, quando finiro' di rimuovere i /2, diventera' Cell.SIZE_IN_PIXELS.

Anche perche' potremmmo poi testare i casi dove la gravita' e' > Cell.SIZE_IN_PIXELS

[Jocchan]

Originariamente inviato da Bonfo

Alla faccia del costumer

[fek]

Originariamente inviato da Bonfo

No, in realta' bisognerebbe revertare tutti i Cell.SIZE_IN_PIXELS * 2 e valutare caso per caso dove abbia senso mettere NormalGravity e dove lasciare Cell.SIZE_IN_PIXELS * 2, che poi, quando finiro' di rimuovere i /2, diventera' Cell.SIZE_IN_PIXELS.

Anche perche' potremmmo poi testare i casi dove la gravita' e' > Cell.SIZE_IN_PIXELS

Ok, proseguiamo per questa strada allora.

[cisc]

ciao a tutti, vorrei riprendere a partecipare attivamente al progetto, prima di andare ad agire su qualche refactor this, vorrei sapere se per caso c'è qualcuno che sta lavorando in modo particolare in un'area del codice, in modo da non rischiare di sovrapporci, anche considerando il fatto che so arruginito co diamonds quindi i miei refactoring saranno lentucci all'inizio, o se sono state prese alcune decisioni di design cui magari tendere nel lavoro di rendere il codice più leggibile

[Jocchan]

Originariamente inviato da cisc

ciao a tutti, vorrei riprendere a partecipare attivamente al progetto, prima di andare ad agire su qualche refactor this, vorrei sapere se per caso c'è qualcuno che sta lavorando in modo particolare in un'area del codice, in modo da non rischiare di sovrapporci, anche considerando il fatto che so arruginito co diamonds quindi i miei refactoring saranno lentucci all'inizio, o se sono state prese alcune decisioni di design cui magari tendere nel lavoro di rendere il codice più leggibile

Bentornato anche te

[Bonfo]

Originariamente inviato da fek

Ok, proseguiamo per questa strada allora.

L'altra sera mi hanno portato a casa alle 23:30
Quindi non sono riuscito ad andare avanti con le modifiche che avevo iniziato a fare e a rintrodurre i Cell.SIZE_IN_PIXEL * 2. Confido in stasera

P.S.: bentornati sia a Vifani sia a cisc