Torneo Refactoring

E' partito un torneo di refactoring.

Non partecipando, comunque ho deciso di affrontare il problema, per il momento commento come avrei affrontato la prova01:

Il codice e' versionato nel repos. aziendale.

prima di fare il refactoring richiesto, che chiede di sfruttare la somiglianza tra evaluate e forEachDo, creo dei test appositi per essi, e poi rifattorizzo, e verifico che i test passano anche dopo il refactoring.

Per testare la forEachDo:
mi appoggio ad una implementazione di Block che implementa la evaluate concatenando i toString degli oggetti processati in una unica stringa globale.
Se applico la evaluate con questa implementazione di Block, ad una collezione contenente "first" e "second" mi aspetto che dopo il test la stringa globale abbia concatenato "first" e "second".

public class BlockImplForTest implements Block {
public static String references="";
public static void reset() {
    references="";
}

public  void evaluate(Object object)
{
    references+=object.toString();
}
}

Il test e' il seguente:

@Test
public void testSelect() {
OrderedCollection col = new OrderedCollection();
col.add("first");
col.add("second");

Block stringRefAppender=getFreshBlockImplForTest();
col.forEachDo(stringRefAppender);

assertEquals(BlockImplForTest.references,"first"+"second");
assertFalse(BlockImplForTest.references.equals("wrong"));

}

Per rendere piu' evidente che Il Block che uso deve essere "fresco", lo reperisco tramite questo metodo che fa anche un reset dello stato:



BlockImplForTest getFreshBlockImplForTest() {
BlockImplForTest.reset();
return new BlockImplForTest();
}

Forse, come design, e' discutibile aver usato una variabile globale, ma allo scopo del test va piu' che bene.

Il test del select crea una classe anonima che implementa il PredicateBlock in modo che che la is restituisca true solamente se la stringa vale "second".
In questo modo, mi aspetto che la lista filtrata attraverso questo PredicateBlock contenga solamente la stringa second. Sfrutto parte della logica del precedente test per scandagliare la collezione restituita e verificare che contenga solamente "second".

@Test
public void testSelector() {
OrderedCollection col = new OrderedCollection();
col.add("first");
col.add("second");

Block stringRefAppender=getFreshBlockImplForTest();

assertEquals("",BlockImplForTest.references);
PredicateBlock getOnlySecond= new PredicateBlock()
{
    public boolean is(Object object) {
        return  "second".equals(object);
    }

};

OrderedCollection ordCol = col.select(getOnlySecond);
ordCol.forEachDo(stringRefAppender);
assertEquals("second",BlockImplForTest.references);

}

Il test ci assicura che l'attuale implementazione dei metodi di OrderCollection fanno quello che ci aspettiamo, ed ora la si rifattorizza cercando di sfruttare la somiglianza tra forEachDo e select.
Entrambe eseguono la scansione della collezione, solo che una esegue una certa operazione, e lo fa su tutti gli elementi, senza restituire nulla, l'altra restituisce una sottocollezione di tutti i membri della collezione che soddisfano un certo predicato.

Ragionando un attimo, dal punto di vista logico possono essere due casi particolari di una unica operazione che scandisce gli elementi , valuta il predicato, esegue l'operazione ed (eventualmente) restituisce gli elementi processati.

Il caso particolare della forEachDo e' che il predicato restituisce sempre true, che non ci interessa utilizzare il valore restituito, mentre il caso particolare della select e' che la evaluate non esegue nulla.

La rifattorizziamo dunque creando due implementazioni di Block e PredicateBlock in accordo con questi due casi particolari:

1) Creiamo il Block e il PredicateBlock corrispondente ai casi particolari appena discusi:

private static final PredicateBlock passesAll =
new PredicateBlock() {
    public boolean is(Object object)   {
        return true;
    }
};

private static final Block neuterBlock =
new Block() {
    public void evaluate(Object object)  { }
};

2) Aggiungiamo un metodo "evaluate and select":

private OrderedCollection evaluateAndSelect(PredicateBlock pb, Block b)
{
OrderedCollection result = new OrderedCollection();
Iterator iterator = _items.iterator();
while(iterator.hasNext()) {
    Object object = iterator.next();
    if (pb.is(object)) {
        b.evaluate(object);
        result.add(object);
    }
}
return result;
}

3) Trasformiamo la ForEach e la Select in modo che siano appunto casi particolari dell'utilizzo di evaluateAndSelect sfruttando le implementazioni "neutre" di Block ed PredicateBlock:

public OrderedCollection select(PredicateBlock aBlock) {
return evaluateAndSelect(aBlock,neuterBlock);
}

public void forEachDo(Block aBlock) {
evaluateAndSelect(passesAll,aBlock);
}

la qualità non è negoziabile

Nel suo libro relativo alla sua esperienza di Scrum e di Xp, Eric Kniberg spiega che il loro approccio nello sviluppo tra i vari principi adotta quello che la "qualità non è negoziabile".
Più precisamente si distingue tra qualità esterna e qualità interna.
La qualità esterna può essere rappresentata da una interfaccia utente scarsa, povera, anti-intuitiva, mentre la qualità interna può essere rappresentata da un codice robusto, ben testato, privo di bachi, manutenibile, etc...

La qualità esterna può essere considerata semplicemente parte dello scope, mentre la qualità interna dovrebbe essere un must non negoziabile.

La qualità percepibile è quella esterna, dunque si potrebbe essere tentati ad essere accomodanti a riguardo della qualità interna, ed accettare da parte del committente discorsi del tipo "rispetto il fatto che tu stimi che questa attività possa essere valutata in 6 story-points, ma sono sicuro che con un quick fix potresti trasformarla in una attività valutabile in soli 2 story-points, [tanto l'utente non se ne accorge]".

Questo discorso andrebbe respinto nei confronti del committente, anche se nella pratica questo è molto difficile, perché ciò che si vende è ciò che s vede.

Dunque è difficile dare il giusto valore alla qualità interna.

Se ne discute in un tread della lista "scrumdevelopment".