BigGem, per gli amici BugGem!

[Bonfo]

Allora mi stavo mettendo a testare mergeUp e mergeRight (http://www.hwupgrade.it/forum/showpo...postcount=124), quando sono esploso

E' normale che il costruttore di BigGem sia totalmente sgangerato e probabilmente poco testato?!?

Cosa se ne fa bigGem di essere costrutita con una size 1x1 (anzi bisognerebbe costruirla solo con size 2x2) e poi cosa se ne fa di sapere da quali gemme e' stata creata, e poi che se ne fa dell position della sprite( ultimo parametro Point) a costruzione, che tanto dipenda da row e column passatigli come primi parametri ??
Inoltre BigGem e' un Droppable, e quindi la sua row e column non devo essere gestite a creazione, ma ad inserimento come per le altre droppable.

E poi e' normale che siano pochissimi i testCase che usano questo costruttore?? Vuol dire che tutte le volte che abbiamo problemi con BigGem andiamo a a:
-creare la griglia,
- inserire le gemme
-fare updateBigGem

Insomma.... abbiamo fatto un po' i porcellini

Altro che calcio rotante volante, qui ci vuole la sacra tecnica del colpo della capocciata di travertino di Hokuto

[Ufo13]

Se si puo` cambiare BugGem eliminando le gemme contenute in essa ben venga! Le prime implementazioni le richiedevano ma forse ora...

[Bonfo]

Allora, come avete capito, secondo me il costruttore di BigGem è "sbagliato".
Quello giusto, per me, è:

    public BigGem(DroppableColor color, AbstractEngine engine);

che inizializza una BigGem 2x2 con la sprite corretta e basta.
Il poizionamento della sprite e della BigGem vengono gestiti all'inserimento in grid, come succede per le altre droppable.
Inoltre non è dall'esterno che si aggiungono le gemma alla bigGem e che si cancellano quele assorbite da grid, ma se ne occupa la BigGem, che sa benissimo come fare queste cose.
Anche perchè a prima vista la differenza tra extend e mergeUp/Right non è così chiara.


Ora, per sapere come procedere, devo capire bene dove si vuole andare con il DroppabaleDescriptor (che non è testato ).

Se l'obiettivo è di avere un Droppabale generico che si comporta in modo diverso in base al descriptor, una specie di "behaviour injection" ( me lo sono inventato adesso, chissa se esiste ), allora dobbiamo renderci conto che tutte quele cose brutte (isGem, isStone, isSomthing, isSomethingElse...) probabilmente verrano risolte con il poliformismo in questa classe.
A questo punto mi viene da dire che deve quasi divenatare uno Startegy... in modo tale che quando si dice alla griglia TurnStoneInToGem o qualsiaisi altro "stato", ogni droppable si comporterà in modo diverso in base al descriptor.
Ciò facendo però stiamo semplicemente portando fuori dal concetto di droppable la gerechia di Gem, Stone, Flash e quant'altro, che finirà inesorabilemnte in DroppableDescriptor.

E' questo quello che vogliamo?

In questo caso mi ritornerebbe utile farmi generare la AnimatedSprite direttamente dal descriptor.

Esempio:

    public BigGem(DroppableColor color, AbstractEngine engine)
    {
        
        super(new DroppableDescription(AbstractDroppableType.BIG_GEM, color), engine);
    }

    protected AbstractDroppable(DroppableDescription droppableDescription, AbstractEngine engine)
    {
        this.type = droppableDescription.getType();
        this.color = droppableDescription.getColor();
        
        setAnimatedSprite(droppableDescription.createSprite(engine));
    }

    public AnimatedSprite createSprite(AbstractEngine engine) {
        return type.createAnimatedSprite(engine, color);
    }

    public AnimatedSprite createAnimatedSprite(AbstractEngine engine, DroppableColor color) {
        TiledSprite sprite = new TiledSprite(color.createTiledImage(engine), 2, 2);
        return new AnimatedSprite(sprite, AnimationDescription.createNullAnimation());
    }

Quello sopra è il caso della BigGem.


P.S.: anche AnimationDescriptor non è testato....

P.P.S.: ehm.... è colpa del coach se mancano i test nei Descriptor. O non ho capito niente io (), oppure vediamo se si spezza le ditine anche da solo


Oh, scherzo.... non mi far bannare

[Ufo13]

Originariamente inviato da Bonfo

Allora, come avete capito, secondo me il costruttore di BigGem è "sbagliato".
Quello giusto, per me, è:

    public BigGem(DroppableColor color, AbstractEngine engine);

che inizializza una BigGem 2x2 con la sprite corretta e basta.
Il poizionamento della sprite e della BigGem vengono gestiti all'inserimento in grid, come succede per le altre droppable.
Inoltre non è dall'esterno che si aggiungono le gemma alla bigGem e che si cancellano quele assorbite da grid, ma se ne occupa la BigGem, che sa benissimo come fare queste cose.
Anche perchè a prima vista la differenza tra extend e mergeUp/Right non è così chiara.


Ora, per sapere come procedere, devo capire bene dove si vuole andare con il DroppabaleDescriptor (che non è testato ).

Se l'obiettivo è di avere un Droppabale generico che si comporta in modo diverso in base al descriptor, una specie di "behaviour injection" ( me lo sono inventato adesso, chissa se esiste ), allora dobbiamo renderci conto che tutte quele cose brutte (isGem, isStone, isSomthing, isSomethingElse...) probabilmente verrano risolte con il poliformismo in questa classe.
A questo punto mi viene da dire che deve quasi divenatare uno Startegy... in modo tale che quando si dice alla griglia TurnStoneInToGem o qualsiaisi altro "stato", ogni droppable si comporterà in modo diverso in base al descriptor.
Ciò facendo però stiamo semplicemente portando fuori dal concetto di droppable la gerechia di Gem, Stone, Flash e quant'altro, che finirà inesorabilemnte in DroppableDescriptor.

E' questo quello che vogliamo?

In questo caso mi ritornerebbe utile farmi generare la AnimatedSprite direttamente dal descriptor.

Esempio:

    public BigGem(DroppableColor color, AbstractEngine engine)
    {
        
        super(new DroppableDescription(AbstractDroppableType.BIG_GEM, color), engine);
    }

    protected AbstractDroppable(DroppableDescription droppableDescription, AbstractEngine engine)
    {
        this.type = droppableDescription.getType();
        this.color = droppableDescription.getColor();
        
        setAnimatedSprite(droppableDescription.createSprite(engine));
    }

    public AnimatedSprite createSprite(AbstractEngine engine) {
        return type.createAnimatedSprite(engine, color);
    }

    public AnimatedSprite createAnimatedSprite(AbstractEngine engine, DroppableColor color) {
        TiledSprite sprite = new TiledSprite(color.createTiledImage(engine), 2, 2);
        return new AnimatedSprite(sprite, AnimationDescription.createNullAnimation());
    }

Quello sopra è il caso della BigGem.


P.S.: anche AnimationDescriptor non è testato....

P.P.S.: ehm.... è colpa del coach se mancano i test nei Descriptor. O non ho capito niente io (), oppure vediamo se si spezza le ditine anche da solo


Oh, scherzo.... non mi far bannare

Ovviamente se testi una classe a colpi di refactoring ti esce non testata. O meglio ti esce testata indirettamente.

In teoria la "potenza di testing" rimane la stessa (a meno di non aggiungere funzionalita`).

L'unico problema e` che i test sono indiretti e quindi difficili da identificare.

Quando ho estratto AnimatedSprite da AbstractSingleDroppable la classe non aveva test. Alcuni li ho dovuti scrivere a manina ma altri li ho semplicemente trovati rovistando in altri file. Comunque quelle due classi hanno bisogno dei loro test case :P

[fek]

Originariamente inviato da Bonfo

Allora, come avete capito, secondo me il costruttore di BigGem è "sbagliato".
Quello giusto, per me, è:

    public BigGem(DroppableColor color, AbstractEngine engine);

che inizializza una BigGem 2x2 con la sprite corretta e basta.
Il poizionamento della sprite e della BigGem vengono gestiti all'inserimento in grid, come succede per le altre droppable.

Perfetto. Fai pure. Ieri mi sono limitato a semplificare il costruttore ma senza cambiarne comportamento.

Ora, per sapere come procedere, devo capire bene dove si vuole andare con il DroppabaleDescriptor (che non è testato ).

DroppableDescription e AnimationDescription sono due mie creazioni
Sono due value type, hanno semantica puramente di valore (anche se in Java non puo' essere forzata). In parole povere contengono solo dati, sono copiabili, non hanno comportamento. E senza comportamento non c'e' nulla da testare

Quindi anche in futuro rimarranno tali. Ho provato a spostare un createImage in DroppableDescripton ma rompe la mia idea di semantica di valore, infatti avrebbe bisogno di un test diretto che non ho scritto, perche' era un semplice tentativo di refactoring, che non mi sta piacendo.

Le classi Description NON sono degl strategy e non lo devono diventare.

P.P.S.: ehm.... è colpa del coach se mancano i test nei Descriptor. O non ho capito niente io (), oppure vediamo se si spezza le ditine anche da solo


Oh, scherzo.... non mi far bannare

Value type = no behavior = no test

[fek]

Originariamente inviato da Ufo13

Ovviamente se testi una classe a colpi di refactoring ti esce non testata. O meglio ti esce testata indirettamente.

In teoria la "potenza di testing" rimane la stessa (a meno di non aggiungere funzionalita`).

L'unico problema e` che i test sono indiretti e quindi difficili da identificare.

Qui si scontrano due necessita':
1- Testare una classe che esce dal refactorinng per avere test diretti
2- Non scrivere decine di test ridondanti che complicano solo il codice, vanno mantenuti e non ci aggiungono copertura

Purtroppo non c'e' una risposta univoca, dipendera' da situazione a situazione. Noi propenderemo comunque per il testare, a parte casi nei quali i test gia' esistenti che coprono quella classe sono abbastanza semplici e a basso livello.

Quando ho estratto AnimatedSprite da AbstractSingleDroppable la classe non aveva test. Alcuni li ho dovuti scrivere a manina ma altri li ho semplicemente trovati rovistando in altri file. Comunque quelle due classi hanno bisogno dei loro test case :P

Sono value type, non c'e' niente da testare!
Che senso avrebbe testare se il numero 5 e' proprio il numero 5?

[Ufo13]

Non ho il codice sottomano ma mi pare che abbiano dei getter. Per me i getter si possono testare... Magari e` overkilling ma di solito testiamo tutto..

[Bonfo]

Originariamente inviato da fek

Perfetto. Fai pure. Ieri mi sono limitato a semplificare il costruttore ma senza cambiarne comportamento.

Ok

Originariamente inviato da fek

... perche' era un semplice tentativo di refactoring, che non mi sta piacendo.

Vuol dire che i descriptor spariscono, o solo il createImage???

Originariamente inviato da fek

Value type = no behavior = no test

Imparo, imparo, imapro

[fek]

Originariamente inviato da Ufo13

Non ho il codice sottomano ma mi pare che abbiano dei getter. Per me i getter si possono testare... Magari e` overkilling ma di solito testiamo tutto..

E' un value type! Io neppure ci metterei i getter! Ma poi checkstyle si arrabbia.

[Bonfo]

Fatto!

Adesso BigGem assomiglia molto di più ad un droppabale!!

Ora si costruisce una gemma con un determinato colore e una area 2x2.
Quindi per costruire gemme non c'è più bisogno di fare qui mandrubbi insert2x2Block, insertBigGem, updateBigGem e chi più ne ha più ne metta... Ora basta fare:

        BigGem bigGem = new BigGem(DroppableColor.DIAMOND, engine);
        grid.insertDroppable(bigGem, row, column);

Per averele più grandi basta fare

        singleGem.getRegion().setColumn(column);
        singleGem.getRegion().setRow(row);

        bigGem.addGem(singleGem);

        assertEquals(6, bigGem.getArea());

Dove row e column rappresentano la celle fino alla quale volete far espandere la BigGem.
Volendo si può aggiungere un costruttore con width e height.

Ora tutti a mettere a posto i test!!! (io vado a letto )

[Bonfo]

Ho cercato anche di eliminare le included gems.
Verebbe anche bene, se no per il fatto che BigGem vorrebbe eliminare le gemme appena le accorpa, ma così ci si becca la ConcurrentModificationException.

Per adesso ho lascito così, ma quella lista lì deve sparire!!

Viene usata solo in updateBigGems() di Grid.
O mettiamo in bigGem un metodo clearIncludedGems(), oppure aggiungiamo uno stato che non fa nient'altro che iterare la grid e pulirla dalle gemme segnate come da eliminare o con un flag ( che fa schifo) o con un lista.

Io voto per il metodo clearIncludedGems()

[Ufo13]

Originariamente inviato da Bonfo

Fatto!

Adesso BigGem assomiglia molto di più ad un droppabale!!

Ora si costruisce una gemma con un determinato colore e una area 2x2.
Quindi per costruire gemme non c'è più bisogno di fare qui mandrubbi insert2x2Block, insertBigGem, updateBigGem e chi più ne ha più ne metta... Ora basta fare:

        BigGem bigGem = new BigGem(DroppableColor.DIAMOND, engine);
        grid.insertDroppable(bigGem, row, column);

Per averele più grandi basta fare

        singleGem.getRegion().setColumn(column);
        singleGem.getRegion().setRow(row);

        bigGem.addGem(singleGem);

        assertEquals(6, bigGem.getArea());

Dove row e column rappresentano la celle fino alla quale volete far espandere la BigGem.
Volendo si può aggiungere un costruttore con width e height.

Ora tutti a mettere a posto i test!!! (io vado a letto )

Hmm non mi piace la bigGem che si espande se aggiungi una Gem... Molto meglio se aggiungi una Region con le caratteristiche giuste (non basta una region 1x1 per esempio).

Ottimo lavoro comunque

[fek]

Bel lavoro Bonfo! E' elegantissimo.

Originariamente inviato da Ufo13

Hmm non mi piace la bigGem che si espande se aggiungi una Gem... Molto meglio se aggiungi una Region con le caratteristiche giuste (non basta una region 1x1 per esempio).

Ottimo lavoro comunque

Anche secondo me. Metterei un metodo che espande aggiungendo una che delega il lavoro ad un metodo che espande aggiungendo una region. Ma solo se il secondo serve a semplificare qualche pezzo di codice o test. Altrimenti va bene la sola gemma.

[fek]

Originariamente inviato da Bonfo

Ho cercato anche di eliminare le included gems.
Verebbe anche bene, se no per il fatto che BigGem vorrebbe eliminare le gemme appena le accorpa, ma così ci si becca la ConcurrentModificationException.

Per adesso ho lascito così, ma quella lista lì deve sparire!!

Viene usata solo in updateBigGems() di Grid.
O mettiamo in bigGem un metodo clearIncludedGems(), oppure aggiungiamo uno stato che non fa nient'altro che iterare la grid e pulirla dalle gemme segnate come da eliminare o con un flag ( che fa schifo) o con un lista.

Io voto per il metodo clearIncludedGems()

Francamente non mi piace nessuna delle due idee. Cercherei un meccanismo piu' semplice ed elegante che non e' costretto ad esporre questo meccanismo interno.

[Ufo13]

In Grid esiste gia` removeGem. Basta chiamarlo da BigGem per le gemme incluse...

[Baol]

Secondo me quella lista da qualche parte ci deve essere, non può sparire. Era "cattiva" quando se ne faceva un uso assurdo, ma è ovvio che se vuoi allargare la BigGem devi eliminare le gemme adiacenti, e non puoi farlo in concomitanza con il loro rilevamento o ti becchi la concurrent...
Certo, non va passata da oggetto a oggetto, andrebbe semplicemente creata, usata e "salutata" all'interno di uno stesso metodo.

Ad esempio, non si potrebbe delegare a UpdateBigGems il piccolo onere (per lei) di trovare ed eliminare le gemme, e invece far fare a BigGem solo la sua estensione tramite una Region?

[Ufo13]

Mi sembra complicato.

La soluzione semplice, secondo me, e` questa:
- Grid non contiene alcuna lista (in questo caso le Gem diventate BigGem) se non i Droppable contenuti
- BigGem mentre si estende si segna le gemme da eliminare in una lista
- Alla fine di ogni estensione BigGem itera la lista e chiama, per ogni gemma, grid.removeDroppable(gem);

Non funzionerebbe cosi`? Non ho il codice sotto mano...

[fek]

Originariamente inviato da Ufo13

Mi sembra complicato.

La soluzione semplice, secondo me, e` questa:
- Grid non contiene alcuna lista (in questo caso le Gem diventate BigGem) se non i Droppable contenuti
- BigGem mentre si estende si segna le gemme da eliminare in una lista
- Alla fine di ogni estensione BigGem itera la lista e chiama, per ogni gemma, grid.removeDroppable(gem);

Non funzionerebbe cosi`? Non ho il codice sotto mano...

Mi piace molto. Ed abbiamo anche un volontario. Tu
Mi sembra un task da fare in pair e in TDD. Stasera dovrei tornare a casa presto e ci possiamo pensare assieme.

[Ufo13]

molto pene.

[71104]

Originariamente inviato da Ufo13

molto pene.

ma è una mania quella di fregarmi le battute
e dire che questa non l'avevo neanche mai scritta sul forum