Orrore, abbiamo una GetType!!!

[Bonfo]

Originariamente inviato da fek

Se vedo una GetType nella code base, prendo l'aereo da Varna, mi paracaduto in Italia, vi spezzo a tutti quanti le ginocchia e poi riparto per l'Inghilterra.

Certo che dopo la FP vi siete dati alla pazza gioia... le pagherete tutte :|

Ehm... c'erano anche prima della FP


In ogni caso pernso che l'eliminazione di isChest e altri non è con un canCrush ( che alla fine siamo lì), ma con il polimorfismo.
Ovvero tutti quanti hanno il metodo crush() che fa qualcosa e in base a che istanza sono mi comporto di conseguenza...

Il getType si elimina veramente solo con il polimorfismo!

[cdimauro]

E' esattamente ciò che auspico.

[jappilas]

Originariamente inviato da Bonfo

<...>
Il getType si elimina veramente solo con il polimorfismo!

su questo non ci piove

ma, come dicevo nel post precedente, non ci sono solo le Droppables con le loro interfaccie, ma anche un sistema di GridAction, che si appoggiano a quelle interfaccie - ma anche, che incapsulano la policy nonchè la conoscenza ( ad es liste di Droppable da testare, o cancellare dalla Grid) con cui gestire le Droppables le quali da oggetti passivi si limitano a essere testate e manipolate

se venisse introdotto un design polimorfico questa relazione tra Droppables e Grid Action verrebbe invertita, o sovvertita (le GridAction in effetti avrebbero molto meno senso di esistere)
se si introducesse un metodo crush() in ogni Droppable, bisognerebbe trovare un design appropriato per gestire a livello della singola Droppable (tenendo conto della "simmetria" tra Droppable) la policy e la "conoscenza" che prima era "globale" nella GridAction, e probabilmente ricontestualizzare il comportamento della Droppable stessa ( riadattare gli stati della Grid inficianti il behaviour) ...
non dico che sia intrinsecamente difficile (anzi magari viene fuori che può essere implementato con pochissimo codice semplice ) ciononostante ho: il sospetto che si tratti non di un lavoro di ripulitura del codice ma proprio di un problema nuovo di design di cui dovrete tenere conto a livello sia di codice che dei test, l' impressione che la riconversione sia meno semplice e indolore dell' (eventuale) interfaccia a "queries" di cui sopra ( che comunque era intesa come un primo passo per intanto introdurre nell' attuale code base una gerarchia di classi ad interfaccia unificata, e non come "soluzione finale"...) e il timore che un nuovo "caso Dynamite" sia sempre in agguato

[Ufo13]

Originariamente inviato da 71104

fek, tanto che ci sei devo chiederti due cose fondamentali:
1) che dobbiamo fare quando vediamo in una classe metodi pubblici utilizzati solamente dai test, cerchiamo di eliminarli e di testare in altri modi?
2) possiamo usare instanceof solo nei test, e non nel codice di produzione?

Dico la mia sulla (1): per me van tolti...

[fek]

Originariamente inviato da 71104

fek, tanto che ci sei devo chiederti due cose fondamentali:
1) che dobbiamo fare quando vediamo in una classe metodi pubblici utilizzati solamente dai test, cerchiamo di eliminarli e di testare in altri modi?
2) possiamo usare instanceof solo nei test, e non nel codice di produzione?

1) A me quei metodi pubblici non creano grossissimi problemi, ma, come sempre, se e' possibile eliminarli e semplificare l'interfaccia di una classe non ho nulla in contrario. Ad esempio isNull() nel caso di un NullAudio si puo' facilmente eliminare rifattorizzando i test relativi usando dei cast.

2) instanceof non mi piace, mi sembra sempre una scorciatoia poco elegante per non pensare al design e buttare giu' qualcosa alla spera in fek (delirio di onnipotenza ). Se semplifica il codice e non c'e' altra soluzione posso anche soprassedere, ma mi devi giustificare la scelta molto bene e nove su dice c'e' sempre una soluzione piu' semplice.

[fek]

Originariamente inviato da Bonfo

Ehm... c'erano anche prima della FP

Non li ho visti, maledetti.

In ogni caso pernso che l'eliminazione di isChest e altri non è con un canCrush ( che alla fine siamo lì), ma con il polimorfismo.
Ovvero tutti quanti hanno il metodo crush() che fa qualcosa e in base a che istanza sono mi comporto di conseguenza...

Il getType si elimina veramente solo con il polimorfismo!

L'idea del metodo crush() mi piace, ma come dice Marco questo e' un refactoring un po' complesso e va pensato bene. Vedo che il design a query e' stato abusato e non mi piace l'implementazione corrente.

[fek]

Originariamente inviato da jappilas

quindi, si può rimuovere le varie getType(), isChest() e affini - a patto però di ripensare anche il resto del codice che circonda la Grid;
oppure, si può come dici tu usare instanceof, ma l' "obbrobrio" finirebbe anche nel codice di produzione e non solo nei test;

La soluzione tipica qui e' creare un oggetto polimorfico. Ma non voglio creare una enorme gerarchia che poi va mantenuta. Quindi non piu' di due livelli, massimo tre in casi eccezionali.

[71104]

Originariamente inviato da fek

La soluzione tipica qui e' creare un oggetto polimorfico. Ma non voglio creare una enorme gerarchia che poi va mantenuta. Quindi non piu' di due livelli, massimo tre in casi eccezionali.

se da AbstractDroppable derivano tutti i tipi di gemme di livelli ne vengono solo due; se però ci teniamo anche la AbstractSingleDroppable (che deriva da AbstractDroppable, e dalla quale derivano tutte le gemme tranne BigGem) allora diventano tre. comunque questa gerarchia ce l'abbiamo già, quindi il polimorfismo non può fare che bene
il problema qui è che questa getType che abbiamo restituisce un oggetto AbstractDroppableType che rispecchia esattamente il tipo (la classe) di gemma, inteso come foglia dell'albero della gerarchia sopra descritta. if e catene di if a non finire insomma, cioè se usassimo instanceof nel codice di produzione sarebbe esattamente la stessa cosa (di' la verità, ti sono venuti i brividi, non credevi che la codebase fosse tanto sporca )

non c'è da ristrutturare la gerarchia, ci troviamo semplicemente nella situazione in cui:
1) i metodi polimorfici di AbstractDroppableType e derivati possono essere spostati dall'oggetto tipo all'oggetto vero e proprio, cioè la gemma (e questo refactoring è non dico semplice ma non-troppo-complesso)
2) tra questi metodi, tutti quelli che cominciamo con "is" effettuano un test a runtime del tipo di gemma (e qui viene il discorso "instanceof sarebbe la stessa cosa" ), e quindi devono essere eliminati ed il tutto refattorizzato di conseguenza, e non devono essere sostituiti da metodi canXxx perché sarebbe qualcosa di molto simile (e questo è il refactoring difficilerrimo).

perché il refactoring del punto 2 è difficile? non perché vada ristrutturata la gerarchia delle gemme radicata in AbstractDroppable (che resta così com'è tale e quale), ma perché va ristrutturato ben altro; indovina cosa?
il codice per il logging delle actions

il problema è il seguente: quei metodi isXxx vengono usati ad esempio per testare se una gemma che ha appena finito di cadere è un chest o una flashing gem, insomma se può "crushare", perché in quel caso il crush va effettuato tramite una action, che è l'unico modo per modificare Grid (o almeno in teoria dovrebbe esserlo ai fini della costruzione del log, ma in realtà il log che attualmente esce fuori da una partita è fatto male perché mancano alcuni eventi a causa del fatto che non tutto il codice usa le actions, ancora ci sono dei metodi "diretti" in Grid): una volta istanziata la action questa chiama un metodo isXxx e controlla come si deve comportare a seconda del tipo di gemma.

il tutto mi porta (OT) al discorso "codice di logging": quando è stato deciso di introdurre il paradigma delle actions avevamo del codice ancora piuttosto sporco per poterlo fare, e sostituire tutti i metodi diretti non era facile (e quindi lo si è fatto solo in parte). il codice per il logging insomma è un lavoro mai terminato che per ora (IMHO) ci crea solo problemi. possiamo eliminarlo? (cosa che avevamo già fatto nella versione di SourceForge )

l'idea comunque non era male, solo che la reintrodurrei (se proprio necessaria) casomai molto più avanti, a codice completamente ripulito.

edit - sia chiaro, un conto è il logging delle actions e un conto sono le actions di per se'; a creare problemi sono le actions (sono le maggiori utenti dei metodi isXxx di cui sopra), ma ovviamente senza le actions scompare anche il logging.

[71104]

Originariamente inviato da fek

2) instanceof non mi piace, mi sembra sempre una scorciatoia poco elegante per non pensare al design e buttare giu' qualcosa alla spera in fek (delirio di onnipotenza ). Se semplifica il codice e non c'e' altra soluzione posso anche soprassedere, ma mi devi giustificare la scelta molto bene e nove su dice c'e' sempre una soluzione piu' semplice.

il problema è questo: quando abbiamo a che fare coi famosi oggetti polimorfici (come ad esempio i numerosi tipi di gemme) e dobbiamo ad esempio testare che un certo metodo (il cui valore di ritorno nominalmente è un generico AbstractSingleDroppable) restituisca un'istanza di una gemma e non di un chest (per dire), nel test dobbiamo essere in grado di distinguere la gemma dal chest, entrambi derivati da AbstractSingleDroppable. per farlo finora avevamo il metodo ovvio di usare getType().isChest o getType().isGem; quando questi metodi scompariranno (perché si spera sia così ) dovremo passare a delle alternative, ovvero testare sull'AbstractSingleDroppable ritornato delle caratteristiche uniche per ciascun tipo di oggetto: il punteggio per esempio, tramite AbstractSingleDroppable.getScore(), oppure il nome tramite toString(). le caratteristiche di questi oggetti però potrebbero essere tante e ciascuna di esse potrebbe non identificare univocamente l'oggetto (il punteggio, bah... e se due classi derivate hanno lo stesso punteggio?), quindi, dal momento che stiamo parlando dei test e non del codice di produzione, mi piacerebbe se fosse avallato l'uso di instanceof

[AnonimoVeneziano]

Beh, visto che hai tirato in ballo la toString() facciamo in modo che la toString() di ogni droppable ritorni il nome dell'istanza dell'oggetto, così siamo apposto (tanto non mi sembra sia usata per altri scopi , credo sia ancora all'implementazione di Object, e non credo possa avere scopi più nobili in futuro)

Ciao

[71104]

infatti ci avevo pensato, però io a toString preferisco addirittura instanceof: mi sembra completamente univoco, "hardcoded", senza possibilità di errore.

[71104]

Originariamente inviato da fek

Ad esempio isNull() nel caso di un NullAudio si puo' facilmente eliminare rifattorizzando i test relativi usando dei cast.

inizialmente non capivo bene quest'ultima cosa, poi andando a zonzo per il codice ne ho incontrato un esempio e ho capito. praticamente si tratta di lasciare che sia un'eccezione a far fallire il test piuttosto che una assertTrue su instanceof.

ottimo, allora farò così

[Bonfo]

Sicuramente il meccanismo ad actions e gli states ( ovvero tutta la logica di gioco ) sono la parte piu' "critica" dell'applicazione.

Non per nulla le action hanno la complessita' piu' alta di tuttoil resto,come testimonia cobertura.

Per i test, siccome te le crei tu a manina le istanze o di gem o di chest, basta stare attenti mentre si fa il test a non perdersi le refrenze e tutto dovrebbe andre a posto da solo

Ultima cosa: asfaltiamo il logging e rifacciamolo. Sicuramente se avremo le action fatte bene, basta che le facciamo implementare il pattern command e dovremmo avre logging e replay della partita praticamente gratis.

[Ufo13]

Originariamente inviato da fek

1) A me quei metodi pubblici non creano grossissimi problemi, ma, come sempre, se e' possibile eliminarli e semplificare l'interfaccia di una classe non ho nulla in contrario. Ad esempio isNull() nel caso di un NullAudio si puo' facilmente eliminare rifattorizzando i test relativi usando dei cast.

2) instanceof non mi piace, mi sembra sempre una scorciatoia poco elegante per non pensare al design e buttare giu' qualcosa alla spera in fek (delirio di onnipotenza ). Se semplifica il codice e non c'e' altra soluzione posso anche soprassedere, ma mi devi giustificare la scelta molto bene e nove su dice c'e' sempre una soluzione piu' semplice.

Potrei sbagliarmi visto che e` passato un sacco di tempo ma mi pare che per testare gli stati del gridcontroller (sono sempre in GC?) instanceof sarebbe stato utile perche` non esisterebbe altro modo per identificare lo stato (forse il dynamic cast).

[Ufo13]

Originariamente inviato da Bonfo

Sicuramente il meccanismo ad actions e gli states ( ovvero tutta la logica di gioco ) sono la parte piu' "critica" dell'applicazione.

Non per nulla le action hanno la complessita' piu' alta di tuttoil resto,come testimonia cobertura.

Per i test, siccome te le crei tu a manina le istanze o di gem o di chest, basta stare attenti mentre si fa il test a non perdersi le refrenze e tutto dovrebbe andre a posto da solo

Ultima cosa: asfaltiamo il logging e rifacciamolo. Sicuramente se avremo le action fatte bene, basta che le facciamo implementare il pattern command e dovremmo avre logging e replay della partita praticamente gratis.

Il logging l'ho personalmente asfaltato qualche giorno fa. Se vedi altro codice relativo (dubito) cancella pure :P

[fek]

Originariamente inviato da 71104

(di' la verità, ti sono venuti i brividi, non credevi che la codebase fosse tanto sporca )

Mi e' venuto un sospetto quando ho letto qualcuno scrivere che il problema di Diamonds fosse la metodologia test-driven ed ho pensato "Vediamo che succede quando smettono di scrivere test-driven"... Abbiamo visto

non c'è da ristrutturare la gerarchia, ci troviamo semplicemente nella situazione in cui:
1) i metodi polimorfici di AbstractDroppableType e derivati possono essere spostati dall'oggetto tipo all'oggetto vero e proprio, cioè la gemma (e questo refactoring è non dico semplice ma non-troppo-complesso)
2) tra questi metodi, tutti quelli che cominciamo con "is" effettuano un test a runtime del tipo di gemma (e qui viene il discorso "instanceof sarebbe la stessa cosa" ), e quindi devono essere eliminati ed il tutto refattorizzato di conseguenza, e non devono essere sostituiti da metodi canXxx perché sarebbe qualcosa di molto simile (e questo è il refactoring difficilerrimo).

Il piano mi piace, abbiamo anche un volontario.
Mi apri un topic e mi scrivi una serie di task dettagliati che descrivono i passi di questi due refactoring?
Ogni passo dev'essere sufficientemente breve e conciso e relativamente semplice da implementare e testare. Diciamo che ti sto chiedendo un "task break down" del lavoro.

perché il refactoring del punto 2 è difficile? non perché vada ristrutturata la gerarchia delle gemme radicata in AbstractDroppable (che resta così com'è tale e quale), ma perché va ristrutturato ben altro; indovina cosa?
il codice per il logging delle actions

Il logging e' eliminato, quindi non porti il problema. Lo introduciamo di nuovo quando la code base e' piu' semplice.

il codice per il logging insomma è un lavoro mai terminato che per ora (IMHO) ci crea solo problemi. possiamo eliminarlo? (cosa che avevamo già fatto nella versione di SourceForge )

Togli tutto.

[fek]

Originariamente inviato da AnonimoVeneziano

Beh, visto che hai tirato in ballo la toString() facciamo in modo che la toString() di ogni droppable ritorni il nome dell'istanza dell'oggetto, così siamo apposto (tanto non mi sembra sia usata per altri scopi , credo sia ancora all'implementazione di Object, e non credo possa avere scopi più nobili in futuro)

Ciao

Che fai bari?
Un problema per volta: una volta che siamo alle prese con questo codice vediamo se c'e' un modo semplice per testare queste classi senza usare instanceof.

[71104]

Originariamente inviato da Ufo13

Potrei sbagliarmi visto che e` passato un sacco di tempo ma mi pare che per testare gli stati del gridcontroller (sono sempre in GC?) instanceof sarebbe stato utile perche` non esisterebbe altro modo per identificare lo stato (forse il dynamic cast).

post #32

Originariamente inviato da Ufo13

Il logging l'ho personalmente asfaltato qualche giorno fa. Se vedi altro codice relativo (dubito) cancella pure :P

le actions sono state introdotte a suo tempo per favorire il logging: dovendo essere l'unico modo per modificare Grid costituivano una "canalizzazione intercettabile" di tutti gli eventi da registrare nel log. le actions le considero strettamente legate al sistema di logging, quindi vanno eliminate assieme ad esso e sostituite con metodi diretti in Grid (che tra l'altro ancora stanno là ) o altrimenti in GridController.

[71104]

Originariamente inviato da fek

Mi e' venuto un sospetto quando ho letto qualcuno scrivere che il problema di Diamonds fosse la metodologia test-driven ed ho pensato "Vediamo che succede quando smettono di scrivere test-driven"... Abbiamo visto

a me sembrava di averlo detto dopo il fallimento...
il codice su cui stiamo lavorando è antecedente, no? :|

Il piano mi piace, abbiamo anche un volontario.
Mi apri un topic e mi scrivi una serie di task dettagliati che descrivono i passi di questi due refactoring?
Ogni passo dev'essere sufficientemente breve e conciso e relativamente semplice da implementare e testare. Diciamo che ti sto chiedendo un "task break down" del lavoro.

ci provo, ma i punti potrebbero essere soggetti a modifiche future: man mano che si va avanti coi refactoring bisogna vedere come si evolve la situazione per decidere cosa fare.

Il logging e' eliminato, quindi non porti il problema. Lo introduciamo di nuovo quando la code base e' piu' semplice.

sempre se servirà ancora visto che il suo scopo era il debugging, e una codebase semplice funziona semplicemente meglio

Originariamente inviato da fek

Che fai bari?
Un problema per volta: una volta che siamo alle prese con questo codice vediamo se c'e' un modo semplice per testare queste classi senza usare instanceof.

ormai io ho iniziato già da ieri sera come hai detto di fare tu nel post #25 al punto 1)
(ho già tolto i due isNull)

[71104]

http://www.hwupgrade.it/forum/showthread.php?t=1666152