[CICLO 1] Esempi di Refactoring

[fek]

Qualche esempio di refactoring vale piu' di mille parole. Ecco come e' cambiata la classe Game.java nel tempo, a seguito di vari interventi di Raffele, Cesare, vicius, cisc e me:

Questa e' la prima versione:


package it.diamonds;

import static org.lwjgl.opengl.GL11.*;
import static org.lwjgl.opengl.GL13.*;
import it.diamonds.texture.Texture;

import org.lwjgl.LWJGLException;
import org.lwjgl.input.Keyboard;
import org.lwjgl.opengl.*;

public class Game
{
static private boolean finished = false;

static private float width = 640f;

static private float height = 480f;

static private float quadSize = 64f;

static private Texture texture;


/**
* @param args
* @throws LWJGLException
*/
public static void main(String[] args)
{
texture = new Texture();

texture.loadTextureFromFile("diamante.png");

System.out.println("File attributes");
System.out.println("width: " + texture.getWidth());
System.out.println("height: " + texture.getHeight());

init();

texture.initOpenGLStates();

texture.setupOpenGLStates();

initOpenGL();

while(!finished)
{
render();
Display.update();
processKeyboard();
}
}


private static DisplayMode findDisplayMode(final int width,
final int height, final int bpp, final int freq)
{
final DisplayMode[] modes;

try
{
modes = Display.getAvailableDisplayModes();

for(int i = 0; i < modes.length; i++)
{
if(modes[i].getWidth() == width
&& modes[i].getHeight() == height
&& modes[i].getBitsPerPixel() >= bpp
&& modes[i].getFrequency() >= 60)
return modes[i];

}
}
catch(LWJGLException e)
{
e.printStackTrace();
}
return null;

}


private static void init()
{
try
{
DisplayMode currentMode = findDisplayMode(640, 480, 24, 60);

Display.setFullscreen(false);
Display.setDisplayMode(currentMode);
Display.setTitle("Diamonds Project - Task 1.5");
Display.create(new PixelFormat(24, 0, 24, 0, 0));

}
catch(Exception e)
{
System.err.println("The current display mode " +
"is not available due to " + e);
System.exit(1);
}
}


private static void initOpenGL()
{
glEnable(GL_NORMALIZE);
glEnable(GL_CULL_FACE);
glHint(GL_PERSPECTIVE_CORRECTION_HINT, GL_NICEST);
glShadeModel(GL_SMOOTH);
glEnable(GL_DEPTH_TEST);
glClearColor(1.0f, 0.0f, 1.0f, 1.0f);
glClearDepth(1f);
glMatrixMode(GL_PROJECTION);
glLoadIdentity();
glOrtho(-width / 2, width / 2, -height / 2, height / 2, -1f, 1000f);
glMatrixMode(GL_MODELVIEW);
}


private static void render()
{
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
glLoadIdentity();

GL11.glEnable(GL11.GL_BLEND);

GL11.glBlendFunc(GL11.GL_SRC_ALPHA, GL11.GL_ONE);

texture.enable(GL_TEXTURE0);

glBegin(GL_TRIANGLES);

// Front
glTexCoord2f(1, 0);
glVertex3f(quadSize, -quadSize, 1);
glTexCoord2f(1, 1);
glVertex3f(quadSize, quadSize, 1);
glTexCoord2f(0, 1);
glVertex3f(-quadSize, quadSize, 1);

glTexCoord2f(1, 0);
glVertex3f(quadSize, -quadSize, 1);
glTexCoord2f(0, 1);
glVertex3f(-quadSize, quadSize, 1);
glTexCoord2f(0, 0);
glVertex3f(-quadSize, -quadSize, 1);

glEnd();
}


private static void processKeyboard()
{
Keyboard.poll();

if(Keyboard.isKeyDown(Keyboard.KEY_ESCAPE))
finished = true;
}

}


Cisc aggiunge l'Audio:


package it.diamonds;

import static org.lwjgl.opengl.GL11.*;
import static org.lwjgl.opengl.GL13.*;
import it.diamonds.audio.Audio;
import it.diamonds.audio.Sound;
import it.diamonds.audio.SoundException;
import it.diamonds.engine.Texture;
import it.diamonds.engine.TextureNotFoundException;

import org.lwjgl.LWJGLException;
import org.lwjgl.input.Keyboard;
import org.lwjgl.opengl.*;

public class Game
{
static private boolean finished = false;

static private float width = 640f;

static private float height = 480f;

static private float quadSize = 64f;

static private Texture texture;

static private int windowHeight = 600;

static private int windowWidth = 800;


/**
* @param args
* @throws LWJGLException
*/
public static void main(String[] args)
{


Audio audio=new Audio();
Sound sound=null;

try
{
sound=new Sound("diamond");
sound.play();
}
catch(SoundException e1)
{
e1.printStackTrace();
}


texture = new Texture();

try
{
texture.loadTextureFromFile("diamond.png");
}
catch(TextureNotFoundException e)
{
e.printStackTrace();
System.exit(0);
}

init();

texture.initOpenGLStates();

texture.setupOpenGLStates();

initOpenGL();

while(!finished)
{
render();
Display.update();
processKeyboard();
}

texture.cleanup();
}


private static DisplayMode findDisplayMode(final int width,
final int height, final int bpp, final int freq)
{
final DisplayMode[] modes;

try
{
modes = Display.getAvailableDisplayModes();

for(int i = 0; i < modes.length; i++)
{
if(modes[i].getWidth() == width
&& modes[i].getHeight() == height
&& modes[i].getBitsPerPixel() >= bpp
&& modes[i].getFrequency() >= 60)
{
return modes[i];
}
}
}
catch(LWJGLException e)
{
e.printStackTrace();
}
return null;

}


private static void init()
{
try
{
DisplayMode currentMode = findDisplayMode(windowWidth, windowHeight,
24, 60);

Display.setFullscreen(false);
Display.setDisplayMode(currentMode);
Display.setTitle("Diamonds Project - Task 1.5");
Display.create(new PixelFormat(24, 0, 24, 0, 0));

}
catch(Exception e)
{
System.err.println("The current display mode " +
"is not available due to " + e);
System.exit(1);
}
}


private static void initOpenGL()
{
glEnable(GL_NORMALIZE);
glEnable(GL_CULL_FACE);
glHint(GL_PERSPECTIVE_CORRECTION_HINT, GL_NICEST);
glShadeModel(GL_SMOOTH);
glEnable(GL_DEPTH_TEST);
glClearColor(0.0f, 0.0f, 0.0f, 1.0f);
glClearDepth(1f);
glMatrixMode(GL_PROJECTION);
glLoadIdentity();
glOrtho(-width / 2, width / 2, -height / 2, height / 2, -1f, 1000f);
glMatrixMode(GL_MODELVIEW);
}


private static void render()
{
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
glLoadIdentity();

GL11.glEnable(GL11.GL_BLEND);

GL11.glBlendFunc(GL11.GL_SRC_ALPHA, GL11.GL_ONE);

texture.enable(GL_TEXTURE0);

final float size = quadSize/2f;

glBegin(GL_TRIANGLES);

// Front
glTexCoord2f(1, 0);
glVertex3f(size, -size, 1);
glTexCoord2f(1, 1);
glVertex3f(size, size, 1);
glTexCoord2f(0, 1);
glVertex3f(-size, size, 1);

glTexCoord2f(1, 0);
glVertex3f(size, -size, 1);
glTexCoord2f(0, 1);
glVertex3f(-size, size, 1);
glTexCoord2f(0, 0);
glVertex3f(-size, -size, 1);

glEnd();
}


private static void processKeyboard()
{
Keyboard.poll();

if(Keyboard.isKeyDown(Keyboard.KEY_ESCAPE))
finished = true;
}

}


Qui il primo refactoring corposo, con la separazione della logica del gioco (poca per ora) dall'engine grafico di presentazione. Notare come il loop principale del gioco sia stato semplificato e sia ora piu' chiaro da leggere, dopo aver implementato il pattern "Template Method":


package it.diamonds;

import it.diamonds.audio.Audio;
import it.diamonds.audio.Sound;
import it.diamonds.audio.SoundException;

import it.diamonds.engine.Engine;
import it.diamonds.engine.Texture;
import it.diamonds.engine.TextureNotFoundException;

import org.lwjgl.input.Keyboard;

public class Game
{
static private boolean finished = false;

static private int windowWidth = 800;
static private int windowHeight = 600;

static private Audio audio;
static private Engine engine;

static private Texture texture;


public static void main(String[] args)
{
createEngine();

playSound();
loadTexture();

while(!finished)
{
render();
update();

processKeyboard();
processWindow();
}

close();
}

private static void createEngine()
{
engine = new Engine(windowWidth, windowHeight);
engine.setWindowTitle("Diamonds Project");
}


private static void close()
{
texture.cleanup();
audio.shutDown();
engine.shutDown();
}


private static void loadTexture()
{
try
{
texture = new Texture("diamond");
}
catch(TextureNotFoundException e)
{
e.printStackTrace();
System.exit(0);
}

texture.initOpenGLStates();
texture.setupOpenGLStates();
}


private static void playSound()
{
audio = new Audio();

Sound sound = null;
try
{
sound = new Sound("diamond");
sound.play();
}
catch(SoundException e1)
{
e1.printStackTrace();
}
}

private static void render()
{
texture.render();
}

private static void update()
{
engine.updateDisplay();
}

private static void processKeyboard()
{
Keyboard.poll();

if(Keyboard.isKeyDown(Keyboard.KEY_ESCAPE))
finished = true;
}

private static void processWindow()
{
if (engine.isWindowClosed())
finished = true;
}

}


A seguito dei problemi nel testing della classe Engine, e' stata qui introdotta una classe DisplayImpl che racchiude il codice OpenGL non testabile automaticamente:


package it.diamonds;

import it.diamonds.audio.Audio;
import it.diamonds.audio.Sound;
import it.diamonds.audio.SoundException;

import it.diamonds.engine.DisplayImpl;
import it.diamonds.engine.Engine;
import it.diamonds.engine.Texture;
import it.diamonds.engine.TextureNotFoundException;

import org.lwjgl.input.Keyboard;

public class Game
{
static private boolean finished = false;

static private int windowWidth = 800;
static private int windowHeight = 600;

static private Audio audio;
static private DisplayImpl display;
static private Engine engine;

static private Texture texture;


public static void main(String[] args)
{
createEngine();

playSound();
loadTexture();

while(!finished)
{
render();
update();

processKeyboard();
processWindow();
}

quit();
}

private static void createEngine()
{
display = new DisplayImpl(windowWidth, windowHeight);
engine = new Engine(windowWidth, windowHeight, display);
engine.setWindowTitle("Diamonds Project");
}


private static void quit()
{
texture.cleanup();
audio.shutDown();
engine.shutDown();
}


private static void loadTexture()
{
try
{
texture = new Texture("diamond");
}
catch(TextureNotFoundException e)
{
e.printStackTrace();
System.exit(0);
}

texture.initOpenGLStates();
texture.setupOpenGLStates();
}


private static void playSound()
{
audio = new Audio();

Sound sound = null;
try
{
sound = new Sound("diamond");
sound.play();
}
catch(SoundException e1)
{
e1.printStackTrace();
}
}

private static void render()
{
texture.render();
}

private static void update()
{
engine.updateDisplay();
}

private static void processKeyboard()
{
Keyboard.poll();

if(Keyboard.isKeyDown(Keyboard.KEY_ESCAPE))
finished = true;
}

private static void processWindow()
{
if (engine.isWindowClosed())
finished = true;
}

}


Ultimo refactoring, con l'introduzione del pattern "Factory Method", la logica di creazione della classe Engine e' stata spostata nella classe stessa, semplificando ulteriormente la classe Game, per l'ultima versione:


package it.diamonds;

import it.diamonds.audio.Audio;
import it.diamonds.audio.Sound;
import it.diamonds.audio.SoundException;

import it.diamonds.engine.Engine;
import it.diamonds.engine.Texture;
import it.diamonds.engine.TextureNotFoundException;

import org.lwjgl.input.Keyboard;

public class Game
{
static private boolean finished = false;

static private int windowWidth = 800;
static private int windowHeight = 600;

static private Audio audio;
static private Engine engine;

static private Texture texture;


public static void main(String[] args)
{
createEngine();

playSound();
loadTexture();

while(!finished)
{
render();
update();

processKeyboard();
processWindow();
}

quit();
}

private static void createEngine()
{
engine = Engine.create(windowWidth, windowHeight, "Diamonds Project");
}


private static void quit()
{
texture.cleanup();
audio.shutDown();
engine.shutDown();
}


private static void loadTexture()
{
try
{
texture = new Texture("diamond");
}
catch(TextureNotFoundException e)
{
e.printStackTrace();
System.exit(0);
}

texture.initOpenGLStates();
texture.setupOpenGLStates();
}


private static void playSound()
{
audio = new Audio();

Sound sound = null;
try
{
sound = new Sound("diamond");
sound.play();
}
catch(SoundException e1)
{
e1.printStackTrace();
}
}

private static void render()
{
texture.render();
}

private static void update()
{
engine.updateDisplay();
}

private static void processKeyboard()
{
Keyboard.poll();

if(Keyboard.isKeyDown(Keyboard.KEY_ESCAPE))
{
finished = true;
}
}

private static void processWindow()
{
if (engine.isWindowClosed())
{
finished = true;
}
}
}


Siamo soddisfatti di questa versione che e' una buona base per proseguire con l'implementazione della logica del gioco. Notare come abbiamo evoluto la prima versione verso un design logico e razionale, che separa logica da presentazione e fornisce ad ogni classe del sistema una e una sola responsabilita'. Il codice dell'ultima versione e' senza dubbio piu' semplice da leggere e quindi da mantenere rispetto al codice della prima versione.

Non e' servito fare un design up front dell'architettura, il lavoro e' stato svolto a piccoli passi da piu' persone che hanno collaborato nel design. Ognuno di noi che ha lavorato su questa classe ora la conosce e potra' intervenire in futuro, per aggiungere funzionalita' o semplificarne il design. Il tutto e' stato coperto da una decina di test che ci danno confidenza sul corretto funzionamento dell'applicazione, e ci hanno avvertito di eventuali errori durante i vari passi di refactoring.

[cdimauro]

Template method ed Extract method sono la stessa cosa?

[fek]

Mi hai dato un'ottima idea.

Quale modo migliore di imparare i Design Pattern e i Refactoring che vederli applicati e studiarli nel nostro progetto?

E' interessante notare come in entrambi i casi non abbiamo implementato l'intero Design Pattern, ma ci siamo fermati al punto che e' utile nel nostro progetto e per le nostre esigenze. E' possibile che le esigenze cambino in futuro e completeremo uno di questi Design Pattern. Il punto interessante e' che i Design Pattern rappresentano dei chiari e ben codificati punti di arrivo (o di partenza) e obiettivi dei refactoring.


Template Method (http://www.dofactory.com/Patterns/PatternTemplate.aspx)

Definition

Define the skeleton of an algorithm in an operation, deferring some steps to subclasses. Template Method lets subclasses redefine certain steps of an algorithm without changing the algorithm's structure.

http://www.dofactory.com/Patterns/Diagrams/template.gif




Factory Method (http://www.dofactory.com/Patterns/PatternFactory.aspx[/url)

Definition

Define an interface for creating an object, but let subclasses decide which class to instantiate. Factory Method lets a class defer instantiation to subclasses.

http://www.dofactory.com/Patterns/Diagrams/factory.gif



Extract Method (http://www.refactoring.com/catalog/extractMethod.html)

You have a code fragment that can be grouped together.

Turn the fragment into a method whose name explains the purpose of the method.

[cionci]

Ha senso secondo te fek creare una classe Resources che serve per caricare le varie risorse dai file ?

Qualcosa tipo questo (te o scrivo in C++):

class Resources {

Resources *_instance;

Resources () { };

public:

static Resources * getResources ()
{
return (_instance == NULL) ? (_instance = new Resources ()) : _instance;
};

TextureRes LoadTexture (string fileName);
SoundRes LoadTexture (string fileName);
...
};

Resources * Resources::_istance = NULL;


Inoltre pensavo ad un altro singleton...che contiene la configurazione del gioco (bpp, risoluzione, varii path)...
In pratica si eliminerebbero tutte quelle variabili locali...

[fek]

Ha senso secondo te fek creare una classe Resources che serve per caricare le varie risorse dai file ?

Si', avra' sicuramente senso quando avremo piu' di una texture. Al momento sarebbe over-engineering, visto che scriviamo strettamente il codice che ci serve per completare i task e nient'altro, seguendo religiosamente il principio YAGNI (You Aren't Gonna Need It).

Inoltre pensavo ad un altro singleton...che contiene la configurazione del gioco (bpp, risoluzione, varii path)...
In pratica si eliminerebbero tutte quelle variabili locali...

http://blog.dotnetwiki.org/PermaLink,guid,777358ea-a106-421b-b0f5-607ed06c0c98.aspx


The problem

The application you are developping is using a Singleton, that you need to test. How do you test a singleton ?

SingletonIsBad approach

1. Singleton are evil! If you have the opportunity to refactory the code, make sure you definitely need this singleton, otherwize get rid of it.
2. You have no choice and you cannot get rid of the singleton. Make sure you split the singleton functionalities in two classes: The class that does all the work and the singleton aspect that prevents multiple instances. Doing so, you will not have to cheat to test the singleton.
3. You could not fullfill any of the point above, and you will need to cheat...

The two first point came from Len Holgate and I totally agree with him. The third point is for tester who do not have the possiblity of avoiding or refactoring the singleton. To tackle this problem, I and Omer have proposed 2 cheats and Darrel Oakley proposed a wider solution to the problem.

Ti quoto che cosa dice Kent Beck riguardo al Singleton:


'Don't use global variables, your programs will be happier,'

Incapsulare tutte le variabili di configurazione in una classe, invece, e' un'ottima idea. Direi che abbiamo un volontario :)

[cionci]

Al limite ci potrebbero essere anche altre soluzioni oltre al singleton, ma fanno uso di varibili static...

Come sai non conosco Java, ma pensavo a qualcosa di simile ai map del C++, però il map dovrebbe essere creato una sola volta all'interno di tutto il programma...

[fek]

Al limite ci potrebbero essere anche altre soluzioni oltre al singleton, ma fanno uso di varibili static...

Si', potremmo aggiungere una storia tecnica che gestisca le configurazioni, magari leggendo da un file di testo in xml. Io di solito creo una classe Config o Environment e poi la passo a chi ne ha bisogno. Sembra noioso passare un oggetto in giro solo per evitare un Singleton, ma una volta che lo implementi e' molto meno noioso di quanto si pensa e risolve cosi' tanti problemi che si hanno con i Singleton, anche nel testare il codice che li usa.

Io soffrivo di una forma acuta di Singletonitis, ora che e' stata curata vivo meglio :p

[cionci]

Ma di una cosa del genere... Forse è troppo vicina al singleton ?

class Config
{
static Map<string, int> intMap;
static Map<string, string> stringMap;
public:
Map ();
int getInt (string id);
string getString (string id);
};

Altrimenti si propaga il Config e festa finita :)

[fek]

Togli gli static, si propaga il config!

Vuoi provare a scriverla? Mi raccomando i test.

[cionci]

Io provo, ma ti avverto che l'ultimo programma Java che ho scritto è stato circa 10 anni fa :)

[fek]

Io provo, ma ti avverto che l'ultimo programma Java che ho scritto è stato circa 10 anni fa :)

Pure io :D

Mi sto letteralmente inventando la sintassi fra C# e C++, quando Eclipse si lamenta mi faccio correggere da lui e poi riparto. Un gran divertimento.

Mi raccomandi i test! Ancora meglio se li scrivi prima del codice.

Esempio:


void testIsEmpty()
{
Config config = new Config();
assertTrue(config.isEmpty());
}

void testAddIntProperty()
{
Config config = new Config();
config.addIntProperty("test", 10);
assertFalse(config.isEmpty());
}

void testGetIntProperty()
{
Config config = new Config();
config.addIntProperty("test", 10);
assertAreEqual(10, config.getIntProperty("test"));
}


void testTwoProperties()
{
Config config = new Config();
config.addIntProperty("test", 10);
config.addIntProperty("test2", 15);

assertAreEqual(2, config.getNumberOfProperties());
assertAreEqual(15, config.getIntProperty("test2"));
}


... etc etc... ad ogni test scrivi il codice che lo soddisfa, e passi al test successivo...

[cionci]

Oki ;)

[fek]

Ferma tutto!
Ho un'idea!

La scriviamo assieme come se facessimo pair programming qui sul forum. Apriamo un topic, io scrivo il test, tu lo implementi, io ti do' i commenti, e poi un altro test e poi tu scrivi il codice e cosi' via.
Se esce bene ne viene fuori un esempio di TDD da leggere come un topic e magari viene fuori una cosa educativa e divertente, che ne dici?

Solo che ora devo uscire, torno fra qualche ora.

[cionci]

Io mi sa che fra qualche ora non ci sono... Se ne parla lunedì in tal caso... Comunque io intantos crivo qualcosa a prescindere, così prendo un po' di confidenza...

[cionci]

Io l'avrei fatta...per ora non legge da file (per XML mi devo ancora attrezzare)...ma inserimento e recupero lo fa...e sembra correttamente (sì il test l'ho fatto)...

Fino a domani sera o addirittura lunedì non so se torno...

[fek]

Va benissimo cosi'. Per il topic sul TDD ci sara' sicuramente un'altra occasione. Se hai i test, fai pure il commit.

[cionci]

Come si fa a formattare il codice ? Mi fallisce il check style... 5 minuti e devo andare...

[cionci]

Ottengo questi errori con la build di ant...
[checkstyle] Running Checkstyle 4.0-beta5 on 20 files
[checkstyle] E:\Java\diamonds\trunk\src\it\diamonds\config\Config.java:42: 'if'
construct must use '{}'s.
[checkstyle] E:\Java\diamonds\trunk\src\it\diamonds\config\Config.java:42:36: Ex
pression can be simplified.
[checkstyle] E:\Java\diamonds\trunk\src\it\diamonds\config\Config.java:52: 'if'
construct must use '{}'s.
[checkstyle] E:\Java\diamonds\trunk\src\it\diamonds\config\Config.java:52:39: Ex
pression can be simplified.

Allego lo zip con i file...

[cionci]

Il problema dell'if l'ho risolto, ma l'altro rimane...

[fek]

I miei nuovi check funzionano :D

Guarda gli errori, ti indicano la riga di codice e il tipo di problema. Sono semplici da correggere.

[fek]

Prova cosi':


public int getIntProperty(String key)
throws ConfigPropertyNotFoundException
{
if(!intMap.containsKey(key))
{
throw new ConfigPropertyNotFoundException();
}

return intMap.get(key);
}


public String getStringProperty(String key)
throws ConfigPropertyNotFoundException
{
if(!stringMap.containsKey(key))
{
throw new ConfigPropertyNotFoundException();
}

return stringMap.get(key);
}

[cionci]

Mi sembrava più chiaro con il false...per questo non avevo messo il not... Non è meno leggibile in questo modo ?

[fek]

Mi sembrava più chiaro con il false...per questo non avevo messo il not... Non è meno leggibile in questo modo ?

Credo che sia piu' questione di abitudine che di altro in realta'. In genere l'occhio del programmatore medio e' piuttosto allenato a riconoscere al volo espressioni come !flag. Ma anch'io in C++, per i test dei puntatori, uso ptr == 0 e non !ptr, perche' lo trovo piu' leggibile.

[cionci]

Allora faccio il commit...poi lavoro sul file XML...

[fek]

Allora faccio il commit...poi lavoro sul file XML...

Scriviamo il topic sul TDD adesso ? Ne hai voglia? Usi MSN?

[cionci]

Vado via :cry: A domani...