View Full Version : [CICLO 3] Test Driven Development task 3.2.6 (fpitt vs cisc)
allora, il task è:
Spostare la gestione della gravita e le collisioni dei diamanti nella classe che gestisce la griglia
che mi sembra un po impreciso, e modificherei (secondo l'interpretazione suggeritami da fek:D) in:
Spostare la gestione della gravita e le collisioni dei diamanti a livello di cella
pensiamo prima alle collisioni:
quindi, il primo test:
public void testCellBottomCollision()
{
int gridColumns = 8;
int gridRows = 14;
Bounds bounds = new Bounds(40f, 40f, 256f, 448f, Config.createForTesting());
Grid grid = new Grid(gridRows, gridColumns, bounds);
Gem gem = new Gem("diamond", bounds);
int yStep = (int)(float)bounds.getHeight() / gridRows;
grid.setGravity(gem.getGemSize());
grid.insertGem(13,4,gem);
grid.insertGem(11,4,gem);
grid.moveAllGems(0, 2 * yStep);
assertTrue (grid.getGemAt(12,4).isDropped());
}
a te fpitt, io vado a mangiare:D
EDIT: ho modificato il test:D
l'avevo detto che ero pieno di impegni in questi giorni..ed infatti, fpitt, ti devo lasciare, ci aggiorniamo a domani sera
Ok cisc, a domani sera ;)
Ragazzi, avete lasciato tutta la notte la build rotta. Questo e' molto grave.
Qualcuno puo' fare il revert degl'ultimi due commit per favore? La build e' rotta e questo non deve MAI essere lasciato stare per lungo tempo perche' significa che nessuno puo' lavorare.
Per la collisione fra diamanti, è molto semplice...
Non stravolgete il codice perchè basta modificare una sola funzione...
Ci sarebbe anche un altro test da fare... L'inserimento su una cella già occupata deve portare ad una eccezione...
Grazie per l'averlo aggiustato :)
Per la collisione fra diamanti, è molto semplice...
Non stravolgete il codice perchè basta modificare una sola funzione...
Mi raccomando ragazzi, sempre la soluzione piu' semplice.
Ci sarebbe anche un altro test da fare... L'inserimento su una cella già occupata deve portare ad una eccezione...
Mi sembrava io e 71104 avessimo scritto il test sull'inserimento in una cella gia' occupata.
fek: io non aggiustato niente :eek:
Allora me lo sono perso :)
Hmmm, allora sara' un problema della build machine. Quando arrivo a casa ci guardo.
io avevo pensato a modificare il Bounds di Gem ad un quadrato intorno alla cella corrente, che viene ridotto in caso di presenza di altri diamanti vicini, cmq, il test che non passa al momento è nel package it.diamonds.tests.ignore, la build machine considera anche questo package?
Io invece penserei di farla il più semplice possibile (vediamo intanto di far passare il test)... Guarda updateGemPosition... Come faresti a far passare il test (solo il test) ? Basta un if...
PS: consiglio di invertire l'ordine dei for per cominciare a disegnare le gemme a partire dal fondo della griglia ;)
PPS: non è detto che la gemma vada mossa prima...
vabbè, in TDD si andrà un passo alla volta, come dici tu cionci, io parlavo della soluzione finale..
vabbè, in TDD si andrà un passo alla volta, come dici tu cionci, io parlavo della soluzione finale..
La soluzione finale uscira' un passo per volta e dal refactoring. Ora preoccupatevi del test.
cmq cionci, non mi sembra che si possa risolvere con un if, dopo tutto nel test io chiedo alla gemma se ha colliso.
modifico il test, per rendere lo sviluppo più regolare:
public void testCellBottomCollision()
{
int gridColumns = 8;
int gridRows = 14;
Bounds bounds = new Bounds(40f, 40f, 256f, 448f, Config.createForTesting());
Grid grid = new Grid(gridRows, gridColumns, bounds);
Gem gem = new Gem("diamond", bounds);
int yStep = (int)(float)bounds.getHeight() / gridRows;
grid.setGravity(gem.getGemSize());
grid.insertGem(13,4,gem);
grid.insertGem(11,4,gem);
grid.moveAllGems(0, 2 * yStep);
assertTrue (grid.gemIsDropped(12.4));
}
a te fpitt
Non mi e' chiaro il significato del metodo moveAllGems(). Chi lo puo' spiegare meglio?
Non mi e' neppure chiaro il significato di swapGrid.
Muove tutte le gemme presenti nel grid dello step scelto...in pratica tutte le gemme, se non sono dropped, reagiscono all'input (viene appunto richiamato da reactToInput)...
già, una spiegazione su swapGrid sarebbe utile...
Muove tutte le gemme presenti nel grid dello step scelto...in pratica tutte le gemme, se non sono dropped, reagiscono all'input (viene appunto richiamato da reactToInput)...
Non ho capito, scusami :)
Il codice non lo chiarisce molto, in alcune parti e' un po' oscuro. In particolare non e' chiaro da dove derivi la necessita' di avere una seconda griglia alternativa e non si possa fare la stessa operazione sulla griglia principale.
Facciamo un po' di refactoring assieme?
ehm perdonatemi, sono un pò emozionato, in fondo è la prima cosa quasi sensata che
faccio per il progetto :D
Ora il test passa.
public boolean gemIsDropped(int row, int column)
{
return true;
}
fpitt, ma dai!!!!!!!!!!
cmq, fek, possiamo continuare, o è meglio chiarire prima quei punti oscuri?
io cmq il prossimo test lo posto, poi in caso bloccaci tu;)
public void testCellBottomCollision()
{
int gridColumns = 8;
int gridRows = 14;
Bounds bounds = new Bounds(40f, 40f, 256f, 448f, Config
.createForTesting());
Grid grid = new Grid(gridRows, gridColumns, bounds);
Gem gem1 = new Gem("diamond", bounds);
Gem gem2 = new Gem("diamond", bounds);
int yStep = (int)(float)bounds.getHeight() / gridRows;
grid.setGravity(gem1.getGemSize());
grid.insertGem(13, 4, gem1);
grid.insertGem(11, 4, gem2);
grid.moveAllGems(0, 2 * yStep);
assertTrue(grid.gemIsDropped(12, 4));
assertTrue (grid.getGemAt(12, 4)!=null);
}
a te, fpitt:D
fpitt, ma dai!!!!!!!!!!
cmq, fek, possiamo continuare, o è meglio chiarire prima quei punti oscuri?
io cmq il prossimo test lo posto, poi in caso bloccaci tu;)
Per ora andate pure avanti. Io rumino un po' sul codice e poi calo la mannaia impietosa del refactoring :D
Mi chiarite a parole il significato dei seguenti metodi?
- moveAllGems()
- isGemDropped()
Devo andare via ora...
Io ho presupposto che possano essere più di una le gemme in movimento... La necessità della seconda griglia viene dal fatto che se muovo una gemma, per fare un esempio, verso il basso...dopo me la ritrovo nella riga successiva è non ho niente per identificare se l'ho già mossa...
Per evitare la collisione fra gemme basta controllare in updateGemPosition se la nuova posizione è già occupata, se è occupata allora non muovo la gemma, ma la rimetto nella posizione passata al metodo...
Se il movimento è verso il basso e la gemma che occupa la posizione sotto è dropped allora devo dare un drop anche alla gemma che tento di spostare verso il basso...
Il move sulla gemma può essere fatto dopo questi controlli...
Ok... ora fermi tutti.
Ho lanciato la batteria di test e abbiamo ben tre test che non passano:
testGravity
testBottomCollision
testGridCollision
Prima che faccia invece calare mannaia del revert, tre test che non passano alla volta e' una cosa molto grave.
Quando si scrive un nuovo test che fallisce per una nuova funzionalita' deve essere il solo a fallire. Non si scrive un nuovo test che fallisce se ce ne sono altri che falliscono. Non e' questo il modo di proseguire con calma. C'e' molta confusione al momento.
Ok, chi mette a posto i test che falliscono prima di proseguire?
cionci, bisogna ricordare che però il movimento verso il bassso è continuo...
fek, ti riferisci a gemIsDropped, che verrà modificato subito in IsGemDropped, e sostanizalmente porta la gestione delle Bottom collision in Grid
a quale classe ti riferisci, io e fpitt abbiamo lavorato solo sul test che vedi su questo forum...
a quale classe ti riferisci, io e fpitt abbiamo lavorato solo sul test che vedi su questo forum...
Io vedo tre test che falliscono e devono essere corretti. Ora e' la priorita'.
fek è colpa mia, avevo cancellato la riga
swapGrid[row][column].move(moveX, yStep);
nel metodo updateGemPosition, ho aggiustato ;)
fek, nel fare il commit ricevo questo errore:
Transmitting file data ...
svn: Out of date: '/trunk/src/it/diamonds/Grid.java' in transaction '428-1'
Fai update e poi commit (questa e' buona regola sempre).
Fatto, ora è a posto. Ho eseguito tutti i test prima di committare e passano.
Bene. Proseguite piano piano ora. Ma molto piano, perche' sto facendo un po' di cambiamenti. Fate spesso l'update.
Niente paura... non sto cambiando tanto... ehm...
Ok, ci sono stati un po' di problemi stasera.
Il problema principale e' dato dalle nostre descrizioni dei task che non sono abbastanza dettagliate e vengono facilmente male interpretate, e questo causa malintesi e l'implementazione sbagliata dei task.
E' successo con il task 3.2.5 che chiedeva una cosa molto semplice, ma non era abbastanza chiaro.
Il task chiedeva di spostare il solo diamante sotto controllo di casella in casella voi tasti freccia, ma senza alcun controllo di collisioni con altri diamanti.
Ho implementato il task in TDD.
Ecco i test:
public class TestGridReactionToInput extends TestCase
{
static final int GRID_COLUMNS = 8;
static final int GRID_ROWS = 14;
private Bounds bounds;
private Grid grid;
private Gem gem;
private Input input;
public void setUp()
{
bounds = new Bounds(40f, 40f, 256f, 448f, Config.createForTesting());
grid = new Grid(GRID_ROWS, GRID_COLUMNS, bounds);
gem = new Gem("diamond", bounds);
input = Input.createForTesting();
}
public void testMoveLeft()
{
grid.insertGem(2, 4, gem);
grid.setGemUnderControl(gem);
input.generateKey(KeyCode.vk_Left);
grid.reactToInput(input);
assertTrue("Gem didnt move to the left", grid.isGemAt(2, 3));
}
public void testMoveLeftWithCollision()
{
grid.insertGem(2, 0, gem);
grid.setGemUnderControl(gem);
input.generateKey(KeyCode.vk_Left);
grid.reactToInput(input);
assertTrue("Gem must not move to the left", grid.isGemAt(2, 0));
}
public void testMoveRight()
{
grid.insertGem(2, 4, gem);
grid.setGemUnderControl(gem);
input.generateKey(KeyCode.vk_Right);
grid.reactToInput(input);
assertTrue("Gem didnt move to the right", grid.isGemAt(2, 5));
}
public void testMoveRightWithCollision()
{
grid.insertGem(2, GRID_COLUMNS - 1, gem);
grid.setGemUnderControl(gem);
input.generateKey(KeyCode.vk_Right);
grid.reactToInput(input);
assertTrue("Gem didnt move to the right", grid.isGemAt(2, GRID_COLUMNS - 1));
}
Ecco una parte del codice che soddisfa i test:
public void reactToInput(Input input)
{
Gem gem = getGemUnderControl();
Cell cell = findGemCell(gem);
if(input.isKeyLeft())
{
if (cell.column() > 0)
{
grid[cell.row()][cell.column()] = null;
insertGem(cell.row, cell.column - 1, gem);
}
}
if(input.isKeyRight())
{
if (cell.column() < grid[cell.row()].length - 1)
{
grid[cell.row()][cell.column()] = null;
insertGem(cell.row, cell.column + 1, gem);
}
}
}
public void setGemUnderControl(Gem gem)
{
if (findGemCell(gem) == null)
{
throw new IllegalArgumentException();
}
gemUnderControl = gem;
}
public Gem getGemUnderControl()
{
return gemUnderControl;
}
Questo codice non e' robustissimo e non risponde ad alcune domande:
- che succede se nessuna gemma e' sotto controllo?
- che succede se la gemma sotto controllo non e' nella griglia?
Siete liberi di scrivere i test relativi e farli passare.
Infine, e' importante che quando un task non e' perfettamente chiaro, non si facciano troppe supposizioni (tipo implementare la collisione quando non e' esplicitamente richiesta). Se siete in dubbio, domandate sul forum. Qui abbiamo avuto un'evidente problema di comunicazione, e questo vuol dire che dobbiamo comunicare di piu' e meglio in entrambe le direzioni.
Ora, se avete dubbi sul task 3.2.6 domandate pure.
I punti fondamentali di questo task sono:
- far spostare una gemma all'interno della propria cella verso il basso di pixel in pixel
- quando la gemma sfora nel'altra cella, spostarla nella nuova cella (cancellandola dalla precedente)
- quando la gemma tocca il fondo, emettere il suono
Scopriremo che implementato questo task, Bounds non ci servira' piu' e potremo eliminarlo.
cisc è tardi, ci aggiorniamo a domani mattina.
Ora, se avete dubbi sul task 3.2.6 domandate pure.
I punti fondamentali di questo task sono:
- far spostare una gemma all'interno della propria cella verso il basso di pixel in pixel
- quando la gemma sfora nel'altra cella, spostarla nella nuova cella (cancellandola dalla precedente)
- quando la gemma tocca il fondo, emettere il suono
Scopriremo che implementato questo task, Bounds non ci servira' piu' e potremo eliminarlo.
quindi non bisogna gestire la collisione con gli altri diamanti eventualmente presenti nella Grid?
Il task chiedeva di spostare il solo diamante sotto controllo di casella in casella voi tasti freccia, ma senza alcun controllo di collisioni con altri diamanti.
I punti fondamentali di questo task sono:
- far spostare una gemma all'interno della propria cella verso il basso di pixel in pixel
- quando la gemma sfora nel'altra cella, spostarla nella nuova cella (cancellandola dalla precedente)
- quando la gemma tocca il fondo, emettere il suono
Non capisco cosa c'era allora da modificare del mio task...faceva quello che chiedevi e controllava solo la collisione con il fondo...e non con altri diamanti... :what:
La collisione con i bordi era gestita dal bound di gem, se le gemme e grid avevano lo stesso bound andava tutto bene...
Ditemelo altrimenti non capisco dove ho sbagliato...
Da quello che ho capito vuoi avere solo una gemma sotto il controllo della tastiera... Benissimo quello non è un problema...
Per questo ciclo abbiamo solo una gemma, quindi è inutile preoccuparsi delle collisioni con altre gemme, o del loro controllo.
Queste arriveranno nei prossimi, meglio non avere fretta e limitarsi a fare SOLO quello che viene richiesto dai task... se questi ultimi non sono espressi in maniera chiara è colpa nostra, ma ci sono forum e MSN per metterci d'accordo sui dettagli :)
Non capisco cosa c'era allora da modificare del mio task...faceva quello che chiedevi e controllava solo la collisione con il fondo...e non con altri diamanti... :what:
La collisione con i bordi era gestita dal bound di gem, se le gemme e grid avevano lo stesso bound andava tutto bene...
Da quanto ho capito dal codice, alla pressione di un tasto, si scatenava una chiamata a moveAllGems() che spostava tutte le gemme e controllava eventuali collisioni.
Ho eliminato tutta questa parte perche' non era richiesta.
Il task richiedeva solo di muovere una gemma (quella sotto controllo, ma non era indicato), senza alcun controllo di collisione.
Il problema qui e' stato duplice: da una parte il task era specificato molto male e dall'altra l'implementazione e' stata effettuata senza chiedere informazioni piu' precise. Un problema di comunicazione che in futuro dobbiamo evitare il piu' possibile.
Tutto giusto, ma le collisioni fra gemme non venivano controllate, e la collisione con i bordi veniva controllata da Gem...veniva solo controllato se una gemma era dropped in tal caso non la muovevo...
Muovevo tutte le gemme perchè già nel task precedente che ha creato grid si presupponeva che potessero esistere più gemme...
PS: i test li avevo postati...
Per questo ciclo abbiamo solo una gemma, quindi è inutile preoccuparsi delle collisioni con altre gemme, o del loro controllo.
Queste arriveranno nei prossimi, meglio non avere fretta e limitarsi a fare SOLO quello che viene richiesto dai task... se questi ultimi non sono espressi in maniera chiara è colpa nostra, ma ci sono forum e MSN per metterci d'accordo sui dettagli :)
Spostare la gestione della gravita e le collisioni dei diamanti a livello di cella...
Comunque non capisco perchè non mi avete detto prima che c'era qualcosa che non andava...avrei potuto riprendere il codice in mano...
Per questo ciclo abbiamo solo una gemma
No, perchè la classe grid teneva già conto dell'esistenza di più gemme del grid... Il doppio ciclo for di draw c'era già nel grid che ho preso io in mano...
cionci, c'è stata molta confusione in questa storia, abbiamo confuso i task, che non erano chiarissimi, e siamo andati ad interpretazione senza magari chiedere spiegazioni, adesso io e fpitt finiamo il 3.2.6 e cerchiamo di chiudere questo ciclo, poi mi auguro che i problemi che si sono verificati in questa storia siano da insegnamento per tutti per il prossimo ciclo, l'importante è non ripetere gli stessi errori.., quindi, io ed fpitt ripartiamo, non fate update;)
Primo test:
- far spostare una gemma all'interno della propria cella verso il basso di pixel in pixel
public class TestCellsideCollision extends TestCase
{
private Gem gem;
private Grid grid;
private AbstractEngine engine;
public void setUp()
{
engine = Engine.createEngineForTesting(800, 600);
gem = Gem.createForTesting();
grid = Grid.createForTesting();
}
public void testGridGravity()
{
grid.setGravity(1.0f);
grid.insertGem(11, 4, gem);
float oldPosY = (grid.getGemAt(11, 4)).getY();
grid.draw(engine);
assertTrue(gem.getY() == oldPosY + 1.0f);
}
}
A te cisc :D
Quindi rifate tutto da capo ?!?!? Questo test passava già prima... Almeno la parte della gravità era corretta...
cionci, si, quello che facevamo prima, adesso non è più richiesto, lo so che questo test prima passava, ma fek a refactoringato tutto:D
fpitt, ho fatto passare il tuo test con queste modifiche in Grid:
public void draw(AbstractEngine engine)
{
gemCollided = false;
for (int y = 0; y < grid.length; y++)
{
for (int x = 0; x < grid[y].length; x++)
{
Gem gem = getGemAt(y, x);
if (gem != null)
{
gem.move(0,1.0f);
gem.draw(engine);
}
}
}
}
.
.
.
public void setGravity(float f)
{
// TODO Auto-generated method stub
}
ecco il nuovo test:
public void testGridGravity()
{
grid.setGravity(1.0f);
grid.insertGem(11, 4, gem);
float oldPosY = (grid.getGemAt(11, 4)).getY();
grid.draw(engine);
assertTrue(gem.getY() == oldPosY + 1.0f);
grid.setGravity(2.0f);
assertTrue(gem.getY() == oldPosY + 2.0f);
}
In pratica ha tolto tutto...bene a sapersi... Ripeto...non me lo poteva dire a me visto che mi ero occupato io di quel task ? Non capisco perchè ha dovuto inserire altra gente a farlo...
il fatto è che tu hai fatto il nostro task, il 3.2.5, o una via di mezzo tra 3.2.5 e 3.2.6....
Comunque questo non è un motivo valido per annullare anche tutto quello che avevo fatto del task 3.2.6...senza chiedermi di rimetterlo a posto...
Io ho fatto il 3.2.6, ma ho chiesto a fek se dovevo spostare anche il controllo dell'input sulla classe Grid...ecco il post di risposta:
Altra cosa... Anche la reazione all'input deve passare da Gem a PlayGrid ?
Si'.
Collisioni solo coi bordi.
senti cionci, mi rendo conto che c'è stata un po di confusione e che sarebbe stato forse più giusto far modificare il tuo task a te (e lasciamo stare il 3.2.5), questa volta è andata così, è inutile continuare con i "se" con i "ma", quindi se per te va bene, finiamo il task e cerchiamo di evitare che episodi del genere si ripetano nella prossima storia, se poi vuoi per forza terminare il task tu e continuare il nostro lavoro, fai pure... ;)
Spostare la gestione della gravita e le collisioni dei diamanti a livello di cella...
Comunque non capisco perchè non mi avete detto prima che c'era qualcosa che non andava...avrei potuto riprendere il codice in mano...
No, perchè la classe grid teneva già conto dell'esistenza di più gemme del grid... Il doppio ciclo for di draw c'era già nel grid che ho preso io in mano...
Guarda, che fossero state considerate collisioni e più gemme in movimento io l'ho saputo solo adesso, leggendo questo topic.
Siamo fuori task perchè non si parla di niente di tutto ciò, in realtà le cose erano molto più semplici, e questi fattori sarebbero stati considerati più avanti :)
Non so di chi sia la colpa, e non mi interessa, dato che la metodologia che ci siamo imposti ci obbliga tutti ad attenerci al 100% ai task, senza aggiungere nulla di nostro, ed eventualmente comunicando per sapere se ulteriori dettagli non specificati erano invece previsti.
Se poi questo task ha sforato nel tuo, cionci, non è affatto una cosa personale, o un rimprovero, o nulla del genere: andando avanti è del tutto normale che siano altri a modificare le porzioni di codice scritte da noi, in modo da adattarle alle aggiunte ;)
Il test è passato apportando le seguenti modifiche:
- Aggiunta una proprietà float gravity alla Grid
. . .
private boolean empty = true;
private Gem grid[][];
private float xStep;
private float yStep;
private float gravity = 1.0f;
private boolean gemCollided;
private Bounds bounds;
. . .
- modificato il metodo setGravity(float f)
public void setGravity(float f)
{
gravity = f;
}
- Modificato il metodo draw
public void draw(AbstractEngine engine)
{
gemCollided = false;
for (int y = 0; y < grid.length; y++)
{
for (int x = 0; x < grid[y].length; x++)
{
Gem gem = getGemAt(y, x);
if (gem != null)
{
gem.move(0, gravity);
gem.draw(engine);
}
}
}
}
cisc a te il prossimo test:
- verificare che la posizione di una Gem nella griglia venga aggiornata correttamente
quando la Gem passa da una cella a quella sottostante. Una Gem passa alla cella
sottostante quando la sua posizione Y, supera la metà della cella corrente.
public void testVerticalMove()
{
float yStep = grid.getHeight() / 14;
grid.setGravity(yStep);
assertTrue(!grid.isGemAt(11, 4) && grid.isGemAt(12, 4));
}
Guarda, che fossero state considerate collisioni e più gemme in movimento io l'ho saputo solo adesso, leggendo questo topic.
Siamo fuori task perchè non si parla di niente di tutto ciò, in realtà le cose erano molto più semplici, e questi fattori sarebbero stati considerati più avanti :)
Non so di chi sia la colpa, e non mi interessa, dato che la metodologia che ci siamo imposti ci obbliga tutti ad attenerci al 100% ai task, senza aggiungere nulla di nostro, ed eventualmente comunicando per sapere se ulteriori dettagli non specificati erano invece previsti.
Se poi questo task ha sforato nel tuo, cionci, non è affatto una cosa personale, o un rimprovero, o nulla del genere: andando avanti è del tutto normale che siano altri a modificare le porzioni di codice scritte da noi, in modo da adattarle alle aggiunte ;)
La colpa è mia, che ho capito male il task, chiedo scusa a tutti, vorrei sapere cortesemente se possiamo completarlo come specificato da fek, o dobbiamo fermarci, grazie;)
La colpa è mia, che ho capito male il task, chiedo scusa a tutti, vorrei sapere cortesemente se possiamo completarlo come specificato da fek, o dobbiamo fermarci, grazie;)
Fek per ora non c'è, comunque se siamo sicuri che i dettagli in questione sono stati chiariti, e tutti gli errori nati dalle nostre incomprensioni sono stati eliminati, credo che possiate andare avanti tranquillamente.
Se poi questo task ha sforato nel tuo, cionci, non è affatto una cosa personale, o un rimprovero, o nulla del genere: andando avanti è del tutto normale che siano altri a modificare le porzioni di codice scritte da noi, in modo da adattarle alle aggiunte ;)
Non è che ha sforato nel mio...il mio codice non esiste proprio più... Capisco che non sia una cosa personale...ma comunque credevo che fosse più giusto che fossi io a rimettere a posto il 3.2.6 (visto che come vedi dal titolo fpitt e cisc stanno facendo il 3.2.6 e non il 3.2.5 che gli era stato assegnato in cui io avevo sforato)...
Riguardo alle collisioni fra diamanti...io non le ho proprio tenute in cosiderazione...ne stavano parlando fpitt e cisc...
ok, allora continuiamo:
ho fatto passare il test di fpitt con le seguenti modifiche:
public void draw(AbstractEngine engine)
{
gemCollided = false;
for (int y = 0; y < grid.length; y++)
{
for (int x = 0; x < grid[y].length; x++)
{
Gem gem = getGemAt(y, x);
if (gem != null)
{
moveGem(gem);
gem.draw(engine);
}
}
}
}
private void moveGem(Gem gem)
{
Cell cell=findGemCell(gem);
gem.move(0, gravity);
if ((gem.getY()-cell.row*yStep-bounds.getTop())>gem.getHeight()/2)
{
grid[cell.row][cell.column]=null;
grid[cell.row+1][cell.column]=gem;
}
}
adesso penso sia opportuno far passare questo test fpitt:
public void testReactToInput()
{
float yStep = grid.getHeight() / 14;
grid.setGravity((yStep/2)+1);
grid.insertGem(11, 4, gem);
grid.draw(engine);
Input input=Input.createForTesting();
input.generateKey(KeyCode.vk_Left);
assertTrue(!grid.isGemAt(11, 3) && grid.isGemAt(12, 3));
}
a te
Non è che ha sforato nel mio...il mio codice non esiste proprio più... Capisco che non sia una cosa personale...ma comunque credevo che fosse più giusto che fossi io a rimettere a posto il 3.2.6 (visto che come vedi dal titolo fpitt e cisc stanno facendo il 3.2.6 e non il 3.2.5 che gli era stato assegnato in cui io avevo sforato)...
Riguardo alle collisioni fra diamanti...io non le ho proprio tenute in cosiderazione...ne stavano parlando fpitt e cisc...
Non so dirti nel dettaglio cosa sia successo, dato che io non mi occupo direttamente del codice ed avrei difficoltà a leggere il tutto, comunque non ti preoccupare: sono sicuro che sia stata una necessità data dall'urgenza di correggere la cosa (dopotutto stiamo sforando coi tempi), e faremo tutti in modo che cose del genere non succedano di nuovo.
piccola dimenticanza, il test precedente di fpitt è stato corretto in questo modo:
public void testVerticalMove()
{
float yStep = grid.getHeight() / 14;
grid.setGravity((yStep/2)+1);
grid.insertGem(11, 4, gem);
grid.draw(engine);
assertTrue(!grid.isGemAt(11, 4) && grid.isGemAt(12, 4));
}
il mio ultimo test invece in questo modo:
public void testReactToInput()
{
float yStep = grid.getHeight() / 14;
grid.setGravity((yStep/2)+1);
grid.insertGem(11, 4, gem);
grid.draw(engine);
Input input=Input.createForTesting();
input.generateKey(KeyCode.vk_Left);
grid.reactToInput(input);
assertTrue(!grid.isGemAt(11, 3) && grid.isGemAt(12, 3));
}
Passato il test di cisc apportando le seguenti modifiche:
- modificato il metodo insertGem di Grid, che all'atto dell'inserimento di una Gem, non
impostava la proprietà gemUnderControl = alla Gem appena inserita.
public void insertGem(int row, int column, Gem gem)
{
if (isGemAt(row, column))
{
throw new IllegalArgumentException();
}
empty = false;
gem.setPos(
bounds.getLeft() + column * xStep,
bounds.getTop() + row * yStep
);
grid[row][column] = gem;
gemUnderControl = gem;
}
cisc ci aggiorniamo a dopo pranzo ;)
Tutto giusto, ma le collisioni fra gemme non venivano controllate, e la collisione con i bordi veniva controllata da Gem...veniva solo controllato se una gemma era dropped in tal caso non la muovevo...
Muovevo tutte le gemme perchè già nel task precedente che ha creato grid si presupponeva che potessero esistere più gemme...
PS: i test li avevo postati...
cionci, gli errori fanno parte del gioco, bisogna accettarli e accettarne le conseguenze. E' giusto imparare anche questa lezione nel lavoro di gruppo.
Le specifiche erano tutt'altro che chiare e io e Vicius dobbiamo lavorare su questo punto e migliorare la descrizione dei task in futuro. La confusione si e' generata principalmente da qui.
Dicevo che fa parte del gioco vedere il proprio codice inutilizzato, figurati che io ho visto interi mesi del mio lavoro buttato via da un giorno all'altro. Ma ho imparato che fa parte del gioco e non e' mai tempo buttato: dai tuoi test e dal tuo codice tu e noi abbiamo comunque imparato qualcosa. Io sono partito da quel codice per implementare il task. La codebase che ne e' uscita e' piu' minimale e ci permette di andare avanti con piu' confidenza.
In generale non attaccatevi emotivamente al codice che scrivete. La cosa piu' importante e' il progetto e siamo direttamente responsabili di tutto il codice, non solo di quello che scriviamo.
Non hai capito...non mi interessa che il mio codice venga buttato via...ma invece mi aspettavo che mi si dicesse: "il tuo codice non è corretto, dovresti agggiungere questo, questo e quest'altro" ed io questa mattina l'avrei fatto...
Non hai capito...non mi interessa che il mio codice venga buttato via...ma invece mi aspettavo che mi si dicesse: "il tuo codice non è corretto, dovresti agggiungere questo, questo e quest'altro" ed io questa mattina l'avrei fatto...
Come ho scritto di la', implementare ieri sera il task e' stata una mia decisione. Ho preferito farlo subito piuttosto che aspettare la mattina. E non ho potuto comunicartelo direttamente perche' ho capito il problema troppo tardi.
Altro test:
- verificare che se ci si sposta in orizzontale, la posizione y della Gem venga conservata
public void testHorizontalMove()
{
grid.setGravity(1.0f);
grid.insertGem(11, 4, gem);
grid.draw(engine);
float oldYPosition = gem.getY();
Input input = Input.createForTesting();
input.generateKey(KeyCode.vk_Left);
grid.reactToInput(input);
grid.draw(engine);
assertTrue(gem.getY() == oldYPosition + 1.0f);
}
a te cisc :D
fatto, allora ho aggiunto il seguente metodo in Grid:
private void updateGemAfterInput(Cell cell, int columnOffset, Gem gem)
{
grid[cell.row][cell.column] = null;
float yGemOffset = gem.getY() - cell.row * yStep - bounds.getTop();
insertGem(cell.row, cell.column + columnOffset, gem);
gem.move(0, yGemOffset);
}
ed ho modificato il metodo reactToInput in questo modo:
public void reactToInput(Input input)
{
Gem gem = getGemUnderControl();
Cell cell = findGemCell(gem);
if(input.isKeyLeft())
{
if(cell.column() > 0)
{
updateGemAfterInput(cell, -1, gem);
}
}
if(input.isKeyRight())
{
if(cell.column() < grid[cell.row()].length - 1)
{
updateGemAfterInput(cell, +1, gem);
}
}
}
adesso passiamo alla gestione della collisione col fondo, passando sostanzialmente tale gestione dalla classe Gem alla classe Grid, ecco un test che non passa:
public void testBottomCollision()
{
float yStep = grid.getHeight() / 14;
grid.setGravity(yStep+yStep/2+1);
grid.insertGem(12, 4, gem);
float oldYPosition = gem.getY();
grid.draw(engine);
assertTrue(grid.isGemAt(13, 4));
assertEquals(grid.getGemAt(13, 4).getY(), oldYPosition + yStep, 0.0001f);
}
a te fpitt
Come mai questo test fallisce ?
public void testVerticalMove2()
{
grid.setGravity(32);
grid.insertGem(0, 4, gem);
grid.draw(engine);
assertTrue(!grid.isGemAt(0, 4));
assertTrue(grid.isGemAt(1, 4));
}
Ragazzi, se lasciate un test che fallisce nel repository per essere fissato scrivetelo. Altrimenti non lasciate alcun test fallito e non scrivete altri test se ci sono gia' test che falliscono.
Sono immerso anima e corpo nei doc dell'X360, ma questo non vuol dire che non vi tenga d'occhio... :p
Il test passa apportando le seguenti modifiche (thx to cisc :D)
- metodo updateGemPosition di Grid
private void updateGemPosition(Gem gem, Cell cell)
{
if (cell.row < grid.length -1)
{
grid[cell.row][cell.column] = null;
grid[cell.row + 1][cell.column] = gem;
gem.move(0, gravity);
}
else
{
if (gem.getY()+gravity > bounds.getTop()+cell.row * yStep)
{
gem.move(0, bounds.getTop()+cell.row * yStep-gem.getY());
}
else
{
gem.move(0, gravity);
}
}
}
- metodo moveGem di Grid
private void moveGem(Gem gem)
{
Cell cell = findGemCell(gem);
if(isGemOnBottomRow(gem, cell))
{
updateGemPosition(gem, cell);
}
else
{
gem.move(0, gravity);
}
}
L'ultimo test:
- se la Gem tocca il fondo bisogna emettere il suono
public void testBottomCollisionSound()
{
Audio audio = new Audio();
Sound sound = Sound.createForTesting("diamond");
float yStep = grid.getHeight() / 14;
grid.setSoundCollision(sound);
grid.setGravity(yStep);
grid.insertGem(12, 4, gem);
grid.draw(engine);
assertTrue(sound.wasPlayed());
audio.shutDown();
}
Ragazzi, se lasciate un test che fallisce nel repository per essere fissato scrivetelo. Altrimenti non lasciate alcun test fallito e non scrivete altri test se ci sono gia' test che falliscono.
Sono immerso anima e corpo nei doc dell'X360, ma questo non vuol dire che non vi tenga d'occhio... :p
No...quello è un test che ho scritto io...non è nel repository...e credo che sia un test importante...
No, infatti è importantissimo, lavoriamo su questo e poi andiamo avanti
No...quello è un test che ho scritto io...non è nel repository...e credo che sia un test importante...
Perfetto. E' passato ora?
Scusate se non riesco a seguire tutto ma ho la banda passante occupata al momento... (oddio, mi sta fumando il mono neurone a leggere quella roba...)
Per dirla tutta...la gemma si trova in fondo al grid in 13, 4 !!!
E' il test che mi ha fatto inserire la swapGrid...
grazie al test di cionci abbiamo trovato due bug in uno, allora il test di cionci non passava per un for in draw, che partiva ad aggiornare tutta la griglia dall'alto, eccolo dopo la modifica:
public void draw(AbstractEngine engine)
{
gemCollided = false;
for(int y = grid.length-1; y >= 0; y--)
{
for(int x = 0; x < grid[y].length; x++)
{
Gem gem = getGemAt(y, x);
if(gem != null)
{
moveGem(gem);
gem.draw(engine);
}
}
}
}
questa modifica ha fatto in modo di trovare un altro bug, in quanto un altro test non passava
allora ho fatto passare l'ultimo test di fpitt, aggiungendo un oggetto sound a Grid, e aggiunto il play nel codice che controlla la collisione, adesso andrebbe fatto un poderoso refactoring, ho provato a cominciare a farlo, ma mi sono perso :D
Io e cisc pensiamo di aver concluso, attendiamo conferma da fek.
Per intanto ecco un probabile bug:
- Che cosa accade se inserisco la stessa Gem in due posizioni differenti? :confused:
Io e cisc pensiamo di aver concluso, attendiamo conferma da fek.
Per intanto ecco un probabile bug:
- Che cosa accade se inserisco la stessa Gem in due posizioni differenti? :confused:
Buona osservazione. Direi che scatena un'eccezione. Scrivi il test e lo implementi?
Il task e' completato correttamente. Bravi :)
Ora c'e' spazio per un po' di refactoring abbastanza semplice:
1) Guardate questo metodo:
private boolean isGemOnNextRow(Gem gem, Cell cell)
{
return (gem.getY() + gravity - cell.row * yStep - bounds.getTop()) > gem.getHeight() / 2;
}
E' un'espressione piuttosto lunga, che ad un'occhiata veloce non vuol dire molto. Questo e' un male, perche' dovrei essere sempre in grado di capire a grandi linee che cosa fa un metodo semplicemente dando un'occhiata veloce al suo codice.
Spezzata questa espressioni in piu' sotto espressioni e aggiungete qualche variabili temporanea il cui nome esemplifichi meglio il significato della sotto espressione. Se necessario aggiungete qualche metodo di aiuto.
2)
Questo e' un ottimo metodo:
private void moveGem(Gem gem)
{
Cell cell = findGemCell(gem);
if(isGemOnNextRow(gem, cell))
{
updateGemCell(gem, cell);
}
else
{
gem.move(0, gravity);
}
}
E' chiarissimo, tutte le operazioni sono allo stesso livello di astrazione.
Questo no:
private void moveGemY(Gem gem, Cell cell)
{
if (gem.getY() + gravity > bounds.getTop() + cell.row * yStep)
{
gem.move(0, bounds.getTop() + cell.row * yStep - gem.getY());
if (sound != null)
{
sound.play();
}
}
else
{
gem.move(0, gravity);
}
}
Alcune operazioni sono ad un livello di astrazionie (gem.move(), sound.play()), altre ad un livello piu' basso come la complicata espressione all'interno dell'if. Quando guardo quell'if, non so che cosa significhi di preciso, devo leggere l'espressione e non voglio farlo. Ci vorrebbe un commento, anzi, meglio, ci vorrebbe un metodo che restituisse un valore vero/falso il cui nome indichi con chiarezza il significato dell'espressione.
Poi, il nome del metodo: moveGemY().
Prestate molta attenzione ai nomi dei metodi e delle variabili. Devono indicare in maniera inequivocabile il loro significato e il loro compito. Questo moveGemY non dice molto del metodo, che mi sembra piu' un applyGravityToGem(). Non siate parchi con le variabili, c'e' l'autocompletamento di Eclipse che ci permette di avere nomi molto lunghi senza perdere tempo.
E poi, se dopo aver scritto il metodo vi accorgete che un altro nome e' piu' appropriato e lo descrive meglio, non esitata a cambiarlo perche' anche qui Eclipse ci viene in aiuto: ALT-SHIT-R sul nome del metodo, digitate il nuovo nome ed Eclipse lo cambiera' ovunque per voi. Usatelo. Spesso.
Non potete immaginare quanto aiuto un buon nome da' a chi legge il codice. E' fondamentale.
3) Questo metodo soffre di un'espressione complessa e non e' chiarissimo ad una prima lettura:
private void updateGemAfterInput(Cell cell, int columnOffset, Gem gem)
{
grid[cell.row][cell.column] = null;
float yGemOffset = gem.getY() - cell.row * yStep - bounds.getTop();
insertGem(cell.row, cell.column + columnOffset, gem);
gem.move(0, yGemOffset);
}
Va semplificato e rivisto un po'. Anche i nomi di alcune variabili non sono molto autoesplicative (yGemOffset). Cell.row e Cell.column sono acceduti direttamente senza passare dal loro metodo getter; questo e' un male.
4) Il seguente metodo soffre del problema di avere espressioni a differenti livelli di astrazione:
private void updateGemCell(Gem gem, Cell cell)
{
if (cell.row < grid.length -1)
{
grid[cell.row][cell.column] = null;
cell.row += 1;
grid[cell.row][cell.column] = gem;
moveGemY(gem, cell);
}
else
{
moveGemY(gem, cell);
}
}
Da una parte moveGemY() e dall'altra le due espressioni:
grid[cell.row][cell.column] = null;
grid[cell.row][cell.column] = gem;
Queste due espressioni potrebbero essere isolate in due metodi:
clearCell(cell) o removeGemFromCell(cell)
addGemToCell(cell, gem)
Con un nome migliore per moveGemY() vedrete come aumentera' di molto la leggibilita' del metodo.
l'avevo detto che ci voleva un po di refactoring, solo che me ne sono scordato:D, me ne occupo io ;)
ho fatto il refactoring, trovando anche un bug, il problema è che adesso, nonostante i test vengano passati tutti, c'è un evidente bug lanciando Game, adesso non so se ce la faccio a correggerlo, perchè devo prepararmi per partire per il concerto dei dream :cool: , in caso lo può correggere qualcun altro?
ecco un test che non passa:
public void testTwoBottomCollision()
{
float yStep = grid.getHeight() / 14;
grid.setGravity(yStep+(yStep/2)+1);
grid.insertGem(12, 4, gem);
float oldYPosition = gem.getY();
grid.draw(engine);
assertTrue(grid.isGemAt(13, 4));
assertEquals(grid.getGemAt(13, 4).getY(), oldYPosition + yStep, 0.0001f);
grid.draw(engine);
grid.setGravity(1);
assertEquals(grid.getGemAt(13, 4).getY(), oldYPosition + yStep, 0.0001f);
}
Ragazzi, piu' attenzione con i nomi:
private boolean isGemOnNextRow(Gem gem, Cell cell)
{
final float gemNextPosition = gem.getY() + gravity;
final float rowBottomBound = cell.row() * yStep + bounds.top();
return (gemNextPosition - rowBottomBound) > gem.getHeight() / 2;
}
Che vuol dire rowBottomBound? :)
BlueDragon
31-10-2005, 22:08
Ragazzi, piu' attenzione con i nomi:
private boolean isGemOnNextRow(Gem gem, Cell cell)
{
final float gemNextPosition = gem.getY() + gravity;
final float rowBottomBound = cell.row() * yStep + bounds.top();
return (gemNextPosition - rowBottomBound) > gem.getHeight() / 2;
}
Che vuol dire rowBottomBound? :)
Direi che rowBottomBound indica il limite basso di una riga.
Se la gemma supera il limite basso della riga di più di metà della sua altezza, allora va considerata come se fosse nella prossima riga..isGemOnNextRow = true.
Direi che rowBottomBound indica il limite basso di una riga.
Se la gemma supera il limite basso della riga di più di metà della sua altezza, allora va considerata come se fosse nella prossima riga..isGemOnNextRow = true.
cellHeight? cellLimit?
DanieleC88
01-11-2005, 08:57
Direi che rowBottomBound indica il limite basso di una riga.
Al massimo bottomRowBound... :Prrr:
vBulletin® v3.6.4, Copyright ©2000-2025, Jelsoft Enterprises Ltd.