Refactoring di Actions

[fek]

http://fcarucci.homeip.net:8080/crui...id.action.html

Abbiamo due problemi:

- CrushByFlashAction ha una complessita' di 3.2 contro una complessita' media di 1.5. Tutto cio' che sta sopra 2.0 merita la nostra attenzione. Mi serve un volontario che si occupi di semplificare questa action per poi eliminarla.

- RemoveDroppableAction: c'e' un metodo non testato. Se quel metodo non e' testato ed e' vuoto, perche' sta li'? Va eliminato o portato in una classe base.

Lo scopo ultimo sara' la rimozione delle Action, perche' hanno un'interazione davvero oscura col resto del codice. Ma prima di tutto vanno semplificate, per facilitarne la rimozione.

In futuro faremo la stessa cosa con un bel Command Pattern, magari semplificato.

Mi servono due volontari.

[Bonfo]

Col command dopo ci viene il log gratis.

[Baol]

Volontario per la CrushByFlashAction presente.

O almeno, provo a lavorarci su e vi dico

EDIT: ma sbaglio o questa classe non è per nulla testata? E se è così, perchè il Coverage Report indica una copertura del 95%? Che devo fare? I test ci sono ma da un'altra parte? Li scrivo? Lascio perdere tanto poi verrà eliminata? Ditemi un po' voi

[thebol]

Originariamente inviato da Bonfo

Col command dopo ci viene il log gratis.

dejavu

ma le action non erano state fatte proprio in ottica di implementare un command pattern?

cos'hanno attualmente di diverso?(apparte il fatto che non sono gli unici modi in cui la griglia viene modificata..)

[fek]

Originariamente inviato da thebol

dejavu

ma le action non erano state fatte proprio in ottica di implementare un command pattern?

cos'hanno attualmente di diverso?(apparte il fatto che non sono gli unici modi in cui la griglia viene modificata..)

Si', quella era l'idea, ma l'esecuzione non fu delle piu' felici

Baol, applica l'antica tecnica di refactoring delle sette stelle di hokuto dell'assimilazione anale. Eliminala quell'action.

[Baol]

Porto tutto in Grid? Non dovrebbe essere complicato
EDIT: ma se ci sta lavorando già 71104 a sto punto io mi fermo...

[Ufo13]

No, per favore, il codice deve essere portato VIA da Grid allo stato attuale delle cose.

[fek]

Originariamente inviato da Baol

Porto tutto in Grid? Non dovrebbe essere complicato
EDIT: ma se ci sta lavorando già 71104 a sto punto io mi fermo...

Continua da dove ha lasciato 71104 oggi e porta via il codice da Grid.

[Baol]

Originariamente inviato da fek

Continua da dove ha lasciato 71104 oggi e porta via il codice da Grid.

E' che stavo facendo esattamente quello che ha fatto 71104, comunque ok, reverto tutto e vedo quello che posso fare (non sono così veloce come altri, purtroppo ).

[jappilas]

Originariamente inviato da fek

<..> l'antica tecnica di refactoring delle sette stelle di hokuto dell' assimilazione anale.

[fek]

Io e Fede ci stiamo facendo prendere la mano da Ken Shiro

[fek]

Originariamente inviato da Baol

E' che stavo facendo esattamente quello che ha fatto 71104, comunque ok, reverto tutto e vedo quello che posso fare (non sono così veloce come altri, purtroppo ).

Va benissimo cosi', non devi essere veloce, devi fare le cose bene
Cosi' tutti sono piu' veloci.
Inoltre il codice e' di tutti, come avrai notato non c'e' qualcuno che ha la responsabilita' di un qualche pezzo di codice o sistema, ma tutti interveniamo ovunque. Quindi e' ottimo per te ripartire dal lavoro fatto da Alberto e proseguire in quella direzione.

Mi raccomando: fai le cose semplici. Non devi fare il commit per forza. Se quello che hai scritto non e' semplice, chiedi consigli qui su come semplificarlo, oppure fai il revert e ricomincia. La seconda volta di solito funziona tutto meglio e si fa molto piu' in fretta.

[Ufo13]

Comunque penso che Ken Shiro abbia segnato profondamente la mia esistenza :|

Ho pure la OST originale

[Baol]

Stanno per suonare le campane per la CrushByFlashAction.
E' tutto a posto, ho solo una domanda: possono esserci più FlashingGem contemporaneamente? E quindi la crush deve tenere conto che possano crushare più d'una? Perchè modificando il codice l'ho involontariamente portato a considerare una sola flashing (modifico in un attimo nel caso, basta aggiungere un ciclo) ma nessun test è fallito per questo...
Questo vuol dire che devo aggiungerlo vero?

[Baol]

Ho modificato per permettere più flashingGems, i test che coprono ci sono già.
Al momento ho lasciato in Grid, semplicemente perchè l'unica cosa che ho aggiunto è un'iterazione sulla griglia, e per portarla fuori ci serve l'Iterator no?
(avevo anche pensato a un'eventuale classe accessoria "GridGemFinder" in cui spostare tutti i metodi che cerchino delle gemme nella lista, però non mi convinceva).
Ora committo, qui di seguito il codice:

public void updateCrushes()
    {
        crushByFlash();

        crushByChestAction.reset();
        doIteration(crushByChestAction);

        crushedGemsCounter += crushByChestAction.getCrushedGemsCounter();
        if(crushByChestAction.isCrushed())
        {
            chainCounter++;
        }
    }


    private void crushByFlash()
    {
        DroppableList flashingGems = findFlashingGems();
        for(Droppable flashingGem : flashingGems)
        {
            flashingGem.getCrusherObject().makeCrush(this);
        }
    }


    private DroppableList findFlashingGems()
    {
        DroppableList listOfFlashingGems = new DroppableList();

        for(Droppable droppable : droppableList)
        {
            if(droppable.getGridObject().getType().isFlashingGem()
                && !droppable.getFallingObject().isFalling())
            {
                listOfFlashingGems.add(droppable);
            }
        }

        return listOfFlashingGems;
    }

e CrushByFlashAction è eliminata

[fek]

Originariamente inviato da Baol

Stanno per suonare le campane per la CrushByFlashAction.
E' tutto a posto, ho solo una domanda: possono esserci più FlashingGem contemporaneamente? E quindi la crush deve tenere conto che possano crushare più d'una? Perchè modificando il codice l'ho involontariamente portato a considerare una sola flashing (modifico in un attimo nel caso, basta aggiungere un ciclo) ma nessun test è fallito per questo...
Questo vuol dire che devo aggiungerlo vero?

Si', devi aggiungerlo tu. Domanda a Jocchan se devi considerare o no questo caso prima.

[fek]

            flashingGem.getCrusherObject().makeCrush(this);

Mi togli quel getCrusherObject() per favore?

if(droppable.getGridObject().getType().isFlashingGem()
                && !droppable.getFallingObject().isFalling())
            {
                listOfFlashingGems.add(droppable);
            }

E poi cerca di eliminare la dipendenza da isFlashingGem() e, possibilmente, anche getFallingObject().

Ottimo lavoro fino ad ora

[Jocchan]

E' molto difficile che ci sia più di una flashing gem, ma non è impossibile. Questo perchè, pur avendo la limitazione (voluta) di non avere mai due flashing nella stessa coppia, è possibile che si verifichi questa situazione nella griglia:

R - gemma rossa
B - gemma blu
F - flashing

|          |
|          |
|          |
|RRRR   RRR|
|RRRR   RRR|
|BB      BB|
|BB  F   BB|
------------

...ovvero si lascia cadere una flashing in un punto in cui non è a contatto con nulla (per comodità nell'esempio ho usato solo big gem).

Con il codice attuale cosa succederebbe in questa situazione? E se un'altra flashing venisse poggiata su quella già presente?

Quello che deve succedere è che la flashing rimanga lì, sola soletta in fondo alla griglia, fino a quando non verrà in contatto con qualcosa. A quel punto, sempre seguendo l'ordine di priorità che ha adesso, dovrà azionarsi.
Non ricordo però se era stato previsto il caso di due flashing a contatto tra loro.

[Ufo13]

Per logica direi che se due flashing vengono a contatto allora tutte le flashing devono essere eliminate... Dopotutto e` un colore NO_COLOR no?

[fek]

Originariamente inviato da Jocchan

Quello che deve succedere è che la flashing rimanga lì, sola soletta in fondo alla griglia, fino a quando non verrà in contatto con qualcosa. A quel punto, sempre seguendo l'ordine di priorità che ha adesso, dovrà azionarsi.
Non ricordo però se era stato previsto il caso di due flashing a contatto tra loro.

In SPF2T la flashing gem che tocca il fondo esplode.