[CICLO 1] Il thread dei problemi del codice (aka "Vi tengo d'occhio")

[fek]

E finalmente inizio a divertirmi un po' anch'io :D

Qui vi segnalo i problemi nel codice riportati dai miei potenti mezzi investigativi e discuteremo eventuali soluzioni. La soluzione non dev'essere implementata necessariamente da chi ha scritto il codice.

it.diamonds.java
SoundException.java
getALErrorString()

Complessita' Ciclomatica di 7 (al limite).


private static String getALErrorString(int err)
{
switch(err)
{

case AL10.AL_INVALID_NAME:
return BASEALERRORMESSAGE + "AL_INVALID_NAME";

case AL10.AL_INVALID_ENUM:
return BASEALERRORMESSAGE + "AL_INVALID_ENUM";

case AL10.AL_INVALID_VALUE:
return BASEALERRORMESSAGE + "AL_INVALID_VALUE";

case AL10.AL_INVALID_OPERATION:
return BASEALERRORMESSAGE + "AL_INVALID_OPERATION";

case AL10.AL_OUT_OF_MEMORY:
return BASEALERRORMESSAGE + "AL_OUT_OF_MEMORY";

case AL10.AL_NO_ERROR:
default:
throw new IllegalArgumentException();
}
}


Consiglio di implementare una mappa e usare il codice d'errore come chiave per recuperare la stringa.

it.diamonds.java
Sound.java
initSource()

47 righe di codice.

private void initSource(String fileName) throws SoundException,
SoundNotFoundException
{
int error;
if(!AL.isCreated())
throw new SoundException("Sound System not initialized");
buffer.position(0).limit(1);
AL10.alGenBuffers(buffer);
if((error = AL10.alGetError()) != AL_NO_ERROR)
{
throw new SoundException(error);
}
FileInputStream inputFile = null;
try
{
inputFile = new FileInputStream(soundDir + fileName
+ soundExtension);
}
catch(IOException e)
{
throw new SoundNotFoundException();
}
WaveData waveFile = WaveData.create(inputFile);
if(waveFile == null)
{
throw new SoundNotFoundException();
}
AL10.alBufferData(buffer.get(0), waveFile.format, waveFile.data,
waveFile.samplerate);
waveFile.dispose();
source.position(0).limit(1);
AL10.alGenSources(source);

if((error = AL10.alGetError()) != AL_NO_ERROR)
{
throw new SoundException(error);
}

AL10.alSourcei(source.get(0), AL_BUFFER, buffer.get(0));
AL10.alSourcef(source.get(0), AL_PITCH, 1.0f);
AL10.alSourcef(source.get(0), AL_GAIN, 1.0f);
AL10.alSource3f(source.get(0), AL_POSITION, 0.0f, 0.0f, 0.0f);
AL10.alSource3f(source.get(0), AL_VELOCITY, 0.0f, 0.0f, 0.0f);

if((error = AL10.alGetError()) != AL_NO_ERROR)
{
throw new SoundException(error);
}
wasLoaded = true;
}


Consiglio di applicare "Extract Method" per renderla piu' leggibile.

it.diamonds.engine

Texture.java e Display.java contengono 12 metodi. Se il numero cresce andranno suddivise.

Texture.java
loadTextureFromFile()
37 righe di codice.


public void loadTextureFromFile(String fileName) throws TextureNotFoundException
{
try
{
IL.create();
ILU.create();

IntBuffer image = ByteBuffer.allocateDirect(4).order(
ByteOrder.nativeOrder()).asIntBuffer();
ilGenImages(image);
ilBindImage(image.get(0));

loaded=ilLoadImage(imageDir + fileName);

if (!isLoaded())
{
throw new TextureNotFoundException("Unable to find the texture "+imageDir + fileName);
}

if(ilGetInteger(IL_IMAGE_FORMAT) == IL_RGB)
{
ilConvertImage(IL_RGB, IL_BYTE);
format = GL_RGB;
}
else
{
if(ilGetInteger(IL_IMAGE_FORMAT) == IL_RGBA)
{
ilConvertImage(IL_RGBA, IL_BYTE);
format = GL_RGBA;
}
}

iluFlipImage();

height = ilGetInteger(IL_IMAGE_HEIGHT);
width = ilGetInteger(IL_IMAGE_WIDTH);
data = ilGetData();

}
catch(LWJGLException e)
{
System.err.println("Texture loading error due to +");
e.printStackTrace();
System.exit(0);
}
}

[71104]

Complessita' Ciclomatica di 7 (al limite).


private static String getALErrorString(int err)
{
switch(err)
{

case AL10.AL_INVALID_NAME:
return BASEALERRORMESSAGE + "AL_INVALID_NAME";

case AL10.AL_INVALID_ENUM:
return BASEALERRORMESSAGE + "AL_INVALID_ENUM";

case AL10.AL_INVALID_VALUE:
return BASEALERRORMESSAGE + "AL_INVALID_VALUE";

case AL10.AL_INVALID_OPERATION:
return BASEALERRORMESSAGE + "AL_INVALID_OPERATION";

case AL10.AL_OUT_OF_MEMORY:
return BASEALERRORMESSAGE + "AL_OUT_OF_MEMORY";

case AL10.AL_NO_ERROR:
default:
throw new IllegalArgumentException();
}
}


Consiglio di implementare una mappa e usare il codice d'errore come chiave per recuperare la stringa. evvabbè però dai, ok quando si scherza, ok essere abbastanza pignoli per davvero, ma le esagerazioni sono inutili...
quando analizzi la complessità di un metodo l'unico obiettivo è semplificarlo al fine di evitare possibili errori, ma il solo fatto di implementare una mappa (immagino che ti riferissi alle hash tables) può portare errori nel codice... uno switch è molto più semplice, anche se dovesse arrivare a dividere il metodo in 30 possibili patterns!! se il metodo contiene solo lo swicth io direi proprio che va bene...
d'accordo che la mappa (o hash table che sia) può essere implementata con tutte le cautele possibili, d'accordo che puoi implementarla scrivendo UNA RIGA di codice per ogni metodo, ma sostituirla ad uno swicth mi sembra eccessivo...

[fek]

evvabbè però dai, ok quando si scherza, ok essere abbastanza pignoli per davvero, ma le esagerazioni sono inutili...

Se hai paura che rifattorizzare quel metodo possa introdurre errori, aggiungi i test che ti diano la sicurezza necessaria per proseguire con la semplificazione.

Nessuna eccezione: quel metodo va semplificato :)

[cisc]

ho fatto un primo refactoring per i problemi segnalati da fek, per quanto riguarda la classe SoundException io sono dell'idea che con lo switch andasse più che bene, cmq, l'ho modificata in questo modo:

posto solo il codice aggiunto o modificato


private static Map<Integer, String> errorMap = new HashMap<Integer, String>();
private static String getALErrorString(int err)
{
if(errorMap.isEmpty())
{
initMap();
}
String result = errorMap.get(err);
if(result == null)
{
throw new IllegalArgumentException();
}
return result;
}


private static void initMap()
{
errorMap.put(AL10.AL_INVALID_NAME, BASEALERRORMESSAGE
+ "AL_INVALID_NAME");
errorMap.put(AL10.AL_INVALID_ENUM, BASEALERRORMESSAGE
+ "AL_INVALID_ENUM");
errorMap.put(AL10.AL_INVALID_VALUE, BASEALERRORMESSAGE
+ "AL_INVALID_VALUE");
errorMap.put(AL10.AL_INVALID_OPERATION, BASEALERRORMESSAGE
+ "AL_INVALID_OPERATION");
errorMap.put(AL10.AL_OUT_OF_MEMORY, BASEALERRORMESSAGE
+ "AL_OUT_OF_MEMORY");
}




la classe Sound, il metodo in questione l'ho rinominato initSound:


private void initSound(String fileName) throws SoundException,
SoundNotFoundException
{
if(!AL.isCreated())
{
throw new SoundException("Sound System not initialized");
}

buffer.position(0).limit(1);
AL10.alGenBuffers(buffer);
checkErrors();
readSoundFile(fileName);

source.position(0).limit(1);
AL10.alGenSources(source);
checkErrors();

initSource();
checkErrors();

wasLoaded = true;
}


private void readSoundFile(String fileName) throws SoundNotFoundException
{
FileInputStream inputFile = null;

try
{
inputFile = new FileInputStream(soundDir + fileName
+ soundExtension);
}
catch(IOException e)
{
throw new SoundNotFoundException();
}

WaveData waveFile = WaveData.create(inputFile);
if(waveFile == null)
{
throw new SoundNotFoundException();
}
AL10.alBufferData(buffer.get(0), waveFile.format, waveFile.data,
waveFile.samplerate);
waveFile.dispose();
}

private void checkErrors () throws SoundException
{
int error;
if((error = AL10.alGetError()) != AL_NO_ERROR)
{
throw new SoundException(error);
}
}

private void initSource ()
{
AL10.alSourcei(source.get(0), AL_BUFFER, buffer.get(0));
AL10.alSourcef(source.get(0), AL_PITCH, 1.0f);
AL10.alSourcef(source.get(0), AL_GAIN, 1.0f);
AL10.alSource3f(source.get(0), AL_POSITION, 0.0f, 0.0f, 0.0f);
AL10.alSource3f(source.get(0), AL_VELOCITY, 0.0f, 0.0f, 0.0f);
}




infine nella classe Texture:



public void loadTextureFromFile(String fileName)

throws TextureNotFoundException

{

try

{

IL.create();

ILU.create();



IntBuffer image = ByteBuffer.allocateDirect(4).order(

ByteOrder.nativeOrder()).asIntBuffer();

ilGenImages(image);

ilBindImage(image.get(0));



loaded = ilLoadImage(imageDir + fileName);



if(!isLoaded())

{

throw new TextureNotFoundException(

"Unable to find the texture " + imageDir + fileName);

}



convertImage();



iluFlipImage();



height = ilGetInteger(IL_IMAGE_HEIGHT);

width = ilGetInteger(IL_IMAGE_WIDTH);

data = ilGetData();



}

catch(LWJGLException e)

{

System.err.println("Texture loading error due to +");

e.printStackTrace();

System.exit(0);

}

}

private void convertImage()

{

if(ilGetInteger(IL_IMAGE_FORMAT) == IL_RGB)

{

ilConvertImage(IL_RGB, IL_BYTE);

format = GL_RGB;

}

else

{

if(ilGetInteger(IL_IMAGE_FORMAT) == IL_RGBA)

{

ilConvertImage(IL_RGBA, IL_BYTE);

format = GL_RGBA;

}

}

}

[fek]

ho fatto un primo refactoring per i problemi segnalati da fek, per quanto riguarda la classe SoundException io sono dell'idea che con lo switch andasse più che bene, cmq, l'ho modificata in questo modo:

posto solo il codice aggiunto o modificato

Ottimo. Solo un piccolo dubbio: in C++ se lanci un eccezione dall'interno di un'altra eccezione esplode il PC. Non conosco le specifiche Java in tal senso, ma non penso siano diverse.
Secondo me e' meglio tornare una stringa come "Illegal Error code". Hai scritto un test per verificare il problema?

[cisc]

penso che non ci sia nessun problema, infatti all'interno di una eccezione checked (ovvero eccezione che bisogna catturare per forza da qualche parte con un catch), lancio un'eccezione unchecked (che non obbligatoriamente deve essere catturata da un catch e che viene usata per segnalare errori di programmazione generalmente) che in pratica mi dice che non esiste un errore col codice passato, penso che sia la soluzione più ovvia ed elegante..., poi il test testSoundException() verifica proprio il comportamente dell'eccezione nel caso di codice di errore non corretto:



public void testSoundException()

{

try

{

throw new SoundException (AL10.AL_NO_ERROR);

}

catch (SoundException e)

{

assertTrue(false);

}

catch (IllegalArgumentException e1)

{

assertTrue(true);

}

}

[cisc]

cmq, nella classe SoundException, la soluzione dello switch era molto più semplice ed efficiente di quella attuale che usa una Map, infatti vorrei far notare come la chiave della Map è un oggetto Integer, quindi qualcosa del tipo errorMap.get(err) viene tradotto in errorMap.get(new Integer(err)) per effetto dell'autoboxing, che è una nuova funzionalità di java 5.0 che però non è molto efficiente

[fek]

cmq, nella classe SoundException, la soluzione dello switch era molto più semplice ed efficiente di quella attuale che usa una Map, infatti vorrei far notare come la chiave della Map è un oggetto Integer, quindi qualcosa del tipo errorMap.get(err) viene tradotto in errorMap.get(new Integer(err)) per effetto dell'autoboxing, che è una nuova funzionalità di java 5.0 che però non è molto efficiente

Per quanto riguarda la semplicita', abbiamo metriche abbastanza precise, e secondo le metriche la nuova versione e' piu' semplice. Ci atteniamo a queste.

Per quanto riguarda la efficienza, beh, portami un profiling del gioco che mostra l'autoboxing come una delle tre funzioni che portano via piu' tempo cpu durante il main loop e possiamo parlarne :)

Altrimenti teniamo la versione piu' semplice, perche' non facciamo mai considerazioni di carattere prestazionale durante il design. Soprattutto se non sono supportate da misure.

La logica dietro a questa scelta e' che non e' sensato perdere in leggibilita' e qualita' del codice per guadagnare qualche microsecondo se e quando un file sonoro non verra' trovato, situazione dopo la quale bloccheremo comunque l'esecuzione. Mentre il codice verra' letto in continuazione.

[fek]

penso che non ci sia nessun problema, infatti all'interno di una eccezione checked (ovvero eccezione che bisogna catturare per forza da qualche parte con un catch), lancio un'eccezione unchecked (che non obbligatoriamente deve essere catturata da un catch e che viene usata per segnalare errori di programmazione generalmente) che in pratica mi dice che non esiste un errore col codice passato, penso che sia la soluzione più ovvia ed elegante..., poi il test testSoundException() verifica proprio il comportamente dell'eccezione nel caso di codice di errore non corretto:

Ottimo.

[cisc]

Per quanto riguarda la semplicita', abbiamo metriche abbastanza precise, e secondo le metriche la nuova versione e' piu' semplice. Ci atteniamo a queste.

Per quanto riguarda la efficienza, beh, portami un profiling del gioco che mostra l'autoboxing come una delle tre funzioni che portano via piu' tempo cpu durante il main loop e possiamo parlarne :)

Altrimenti teniamo la versione piu' semplice, perche' non facciamo mai considerazioni di carattere prestazionale durante il design. Soprattutto se non sono supportate da misure.

La logica dietro a questa scelta e' che non e' sensato perdere in leggibilita' e qualita' del codice per guadagnare qualche microsecondo se e quando un file sonoro non verra' trovato, situazione dopo la quale bloccheremo comunque l'esecuzione. Mentre il codice verra' letto in continuazione.

Be, effettivamente non posso che darti ragione, in fondo il codice "incriminato" si trova all'interno di un'eccezione, quindi sarà codice eseguito in casi "eccezionali".... ;)

P.S: non sai quanto sono curioso di conoscere i tuoi potenti mezzi investigativi... :) :D

[fek]

Be, effettivamente non posso che darti ragione, in fondo il codice "incriminato" si trova all'interno di un'eccezione, quindi sarà codice eseguito in casi "eccezionali".... ;)

P.S: non sai quanto sono curioso di conoscere i tuoi potenti mezzi investigativi... :) :D

Eccoli!
http://metrics.sourceforge.net/

Update site di Eclipse:
http://metrics.sourceforge.net/update

[NA01]

Eccoli!
http://metrics.sourceforge.net/

Update site di Eclipse:
http://metrics.sourceforge.net/update
non ho nulla a che fare con il progetto, ma mi interesserebbero i safe range che hai impostato. :)

grazie, ciao!

[fek]

non ho nulla a che fare con il progetto, ma mi interesserebbero i safe range che hai impostato. :)

grazie, ciao!

Ho tenuto grosso modo quelli di default.
Ho ristretto un po' sul numero delle righe di codice per metodo (40), e sul numero massimo di parametri (5). La complessita' ciclomatica e' impostata a 8 e il numero massimo di blocchi annidati e' 4.

Alla fine i numeri non sono importantissimi, l'importante e' sapere che ci sono dei limiti a questi parametri e bisogna starci attenti mentre si scrive il codice.

[fek]

Ed abbiamo una new entry: parseConfigEntry in Config.java scala prepotentemente le classifiche e si piazza al primo posto a parimerito con una complessita' ciclomatica di 7 :D
Le sue 40 righe di codice, inoltre, non lo aiutano nell'operazione simpatia.

Da semplificare e dividere in piu' metodi al piu' presto:


private void parseConfigEntry(Node node)
throws ConfigException
{
try
{
NodeList propertyList = ((Element)node).getElementsByTagName("property");
String property = parseConfigElement(propertyList);
NodeList valueList = ((Element)node).getElementsByTagName("value");
String value = parseConfigElement(valueList);

if(node.hasAttributes())
{
NamedNodeMap attributes = node.getAttributes();

if(attributes.getLength() != 1)
{
throw new ConfigException("Malformed XML config file");
}

Node type = attributes.item(0);
if(type.getNodeName() != "type")
{
throw new ConfigException("Malformed XML config file");
}

if(type.getNodeValue().compareToIgnoreCase("int") == 0)
{
addIntProperty(property, Integer.parseInt(value));
}
else if(type.getNodeValue().compareToIgnoreCase("string") == 0)
{
addStringProperty(property, value);
}
else
{
throw new ConfigException("Malformed XML config file");
}
}
else
{
addStringProperty(property, value);
}
}
catch(Exception e)
{
throw new ConfigException(e.getMessage());
}

}


Da una settimana ormai in classifica di complessita' (7), findDisplayMode() rischia il refactoring coatto. O lo semplificate voi o ne faccio scempio io:


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

try
{
modes = org.lwjgl.opengl.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();
System.exit(0);
}

return null;
}


Se questi due metodi non subiscono un restyling per la fine di questa storia, entrano di diritto come task della prossima Storia, il che significa che se i task non vengono completati, non si passa alla Storia successiva. Fate i bravi e fatemi contento :)

[cdimauro]

private static Boolean displayModeMatches(org.lwjgl.opengl.DisplayMode mode,
final int width, final int height, final int bpp, final int freq)
{
return mode.getWidth() == width && mode.getHeight() == height
&& mode.getBitsPerPixel() >= bpp && mode.getFrequency() >= 60;
}


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

try
{
modes = org.lwjgl.opengl.Display.getAvailableDisplayModes();

for(int i = 0; i < modes.length; i++)
{
if (displayModeMatches(modes[i], width, height, bpp, 60))
{
return modes[i];
}
}
}
catch(LWJGLException e)
{
e.printStackTrace();
System.exit(0);
}

return null;
}

Spero che adesso vada bene... :fiufiu: Altrimenti ci sarà da spostare l'intero blocco del "for" in una routine a parte...

Non sarebbe possibile integrare in build.xml il controllo della complessità ciclomatica e far fallire la build di conseguenza?
Sarebbe utile per rendersi immediatamente conto del "problema" e metterci mano prima di essere dato in pasto ai leone (anzi, al leone :D)... :p

[cionci]

Ho il sospetto che un metodo della classe Config abbia complessità ciclomatica troppo alta :stordita: :fiufiu:

Edit: nona vevo visto che l'avevi già trovata...

[fek]

Spero che adesso vada bene... :fiufiu: Altrimenti ci sarà da spostare l'intero blocco del "for" in una routine a parte...

Non sarebbe possibile integrare in build.xml il controllo della complessità ciclomatica e far fallire la build di conseguenza?
Sarebbe utile per rendersi immediatamente conto del "problema" e metterci mano prima di essere dato in pasto ai leone (anzi, al leone :D)... :p

Mi hai dato del leone? Sono lusingato :D

C'e' gia' il controllo sulla complessita' ed e' settato a 8. Ma quando arriva a 7 io vi avverto :)

[cionci]

Faccio il refactoring... Il problema è che ci sono tutti quei controlli per generare le eccezioni... Quanto è il limite di righe di codice per metodo ?

[cionci]

Mi dai tutti i parametri ? Ho messo un plugin per Eclipse che mi controlla tutto...

[fek]

Faccio il refactoring... Il problema è che ci sono tutti quei controlli per generare le eccezioni... Quanto è il limite di righe di codice per metodo ?

Mi dai tutti i parametri ? Ho messo un plugin per Eclipse che mi controlla tutto...

Il plugin delle metriche?

Ho impostato a 8 il limite di complessita' e 40 il numero di righe di codice. Il resto e' impostato ai valori di default.

Hmmm... controlli per generare le eccezioni... hmmm... fammi pensare. Prova a giocare con questi:

http://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html
http://www.refactoring.com/catalog/replaceConditionalWithVisitor.html

Oppure questo sembra sia quello che ti serve:

http://www.refactoring.com/catalog/substituteAlgorithm.html

[cionci]

Ho già fatto...almeno credo...

[cdimauro]

Mi hai dato del leone? Sono lusingato :D
Non potevo mica dirti pubblicamente che sei un avvolt... Oops... Ehm, un buon padre di famiglia... :asd: :) :mano:
C'e' gia' il controllo sulla complessita' ed e' settato a 8. Ma quando arriva a 7 io vi avverto :)
Buono. Non sarebbe possibile magari avere qualche warning? Così evitiamo che tu ci avverta... :angel:

[cionci]

Il plugin che ho usato è questo:
http://sourceforge.net/project/showfiles.php?group_id=128057&package_id=162704&release_id=355559

[cionci]

Buono. Non sarebbe possibile magari avere qualche warning? Così evitiamo che tu ci avverta... :angel:
Se ti installi il plugin ti da anche il warning...

[cionci]

Oppure questo sembra sia quello che ti serve:

http://www.refactoring.com/catalog/substituteAlgorithm.html
Ho spostato il controllo sul tipo di entry in un altro metodo...al limite se non va ancora bene uso quello sopra, ma per il controllo di soli due valori mi sembra sprecato...

[cionci]

private String parseConfigEntryType(Node node)
throws ConfigException
{
NamedNodeMap attributes = node.getAttributes();

if(attributes.getLength() == 0)
{
return "string";
}

Node type = attributes.item(0);
if(type.getNodeName() != "type")
{
throw new ConfigException("Malformed XML config file");
}

if(type.getNodeValue().compareToIgnoreCase("int") == 0)
{
return "int";
}
else if(type.getNodeValue().compareToIgnoreCase("string") == 0)
{
return "string";
}

throw new ConfigException("Malformed XML config file");
}

private void parseConfigEntry(Node node) throws ConfigException
{
NodeList propertyList = ((Element)node).getElementsByTagName("property");
String property = parseConfigElement(propertyList);
NodeList valueList = ((Element)node).getElementsByTagName("value");
String value = parseConfigElement(valueList);

if(parseConfigEntryType(node) == "int")
{
addIntProperty(property, Integer.parseInt(value));
}

addStringProperty(property, value);
}

[cionci]

Un else non mi serve...ora lo tolgo...
Edit: modificato il codice sopra
Edit2: ulteriore modifica, fra poco incollo il codice sopra

[cionci]

Suppongo di essere arrivato al capolinea, a meno di implementare l'algoritmo sopra per "int" e "string"...

[fek]

Ho spostato il controllo sul tipo di entry in un altro metodo...al limite se non va ancora bene uso quello sopra, ma per il controllo di soli due valori mi sembra sprecato...

Perfetto, e' un ottimo ragionamento.

Per due soli valori, magari il risultato del refactoring sarebbe piu' complicato che l'implementazione scelta, quindi non ne vale la pena, perche' al momento non ci serve e non vogliamo pagarne il costo.

Se e quando ci serviranno piu' valori potrai completare il refactoring.

Ora a occhio la complessita' dei due metodi e' attorno a 4, che va benissimo.

[cionci]

Ho già fatto il commit...

Potresti dare un'occhio al quel plug-in di Eclipse e suggerire i valori di tutti i parametri ? Mi sa che non basta modificare il valore della complessità ciclometrica per quel plug-in... Fa una marea di controlli... Al limite forse sarebbe bene fare qualche screen delle impostazioni e metterli nel thread degli strumenti...

[fek]

Suppongo di essere arrivato al capolinea, a meno di implementare l'algoritmo sopra per "int" e "string"...

Questa e' una scelta che spetta a te. Devi valutare se il risultato e' piu' semplice e piu' leggibile dell'implementazione corrente. Puoi sempre provarci e vedere come esce e come il codice vuole essere strutturato. Se non ti piace, fai un revert.

[cionci]

Questa e' una scelta che spetta a te. Devi valutare se il risultato e' piu' semplice e piu' leggibile dell'implementazione corrente. Puoi sempre provarci e vedere come esce e come il codice vuole essere strutturato. Se non ti piace, fai un revert.
Ho già immmaginato mentalmente il codice e senza dubbio è più leggibile così...

[fek]

Ho già fatto il commit...

Potresti dare un'occhio al quel plug-in di Eclipse e suggerire i valori di tutti i parametri ? Mi sa che non basta modificare il valore della complessità ciclometrica per quel plug-in... Fa una marea di controlli... Al limite forse sarebbe bene fare qualche screen delle impostazioni e metterli nel thread degli strumenti...

Sono in ufficio ora, non posso guardarlo ora, mi spiace.

Build VERDE :)

[fek]

Un "code smell" (http://xp.c2.com/CodeSmell.html) e una "puzza" nel codice. La puzza e' un sintomo di qualcosa che non va, e anche nel codice una puzza e' un sintomo di qualcosa che magari funziona bene, pero' non ci piace, non ci convince appieno, il codice ci sta dicendo che vuole essere sistemato in questa maniera. E allora puzza :)

Ecco qui un esempio Input.java:


private void updateKeyboardState()
{
keyboard.poll();
while (keyboard.next())
{
keys.set(keyboard.getEventKey(), keyboard.getEventKeyState());
}
}


Qui abbiamo un metodo di una classe che chiama uno dietro l'altro quattro messaggi ad un oggetto di un altra classe. Questo codice "puzza" di Feature Envy (http://c2.com/cgi/wiki?FeatureEnvy), perche' il metodo updateKeyboardState sta letteralmente invidiando la classe keyboard.

Questo metodo urla per essere mosso nella classe Keyboard e vedi mai che muoverlo li' non semplifichi qualche altro pezzo di codice. Un bel Move Method (http://c2.com/cgi/wiki?MoveMethod) qui non ce lo toglie nessuno.

(Sta cosa delle puzze non me la sono inventata io eh...)

[fek]

Ho fatto io il refactoring di questo metodo e come spesso accade un refactoring tira l'altro come le ciliege.

Ecco il risultato:


public void update(BitSet keys)
{
poll();
while (next())
{
keys.set(getEventKey(), getEventKeyState());
}
}


E grazie a questo refactoring ho potuto eliminare quattro metodi dall'interfaccia Keyboard che si e' ridotta cosi':


public interface Keyboard
{
Boolean isCreated();

void shutDown();

void update(BitSet keys);
}


Ed ho fatto fuori l'inutile campo keys in KeyboardImpl.java. Il codice si comporta allo stesso modo di prima, ma con una ventina di riga di codice in meno :)

Ora c'e' la classe MockKeyboard.java nel mirino. Ha le ore contate...

[cdimauro]

private void updateKeyboardState()
{
keyboard.poll();
while (keyboard.next())
{
keys.set(keyboard.getEventKey(), keyboard.getEventKeyState());
}
}


Qui abbiamo un metodo di una classe che chiama uno dietro l'altro quattro messaggi ad un oggetto di un altra classe. Questo codice "puzza" di Feature Envy (http://c2.com/cgi/wiki?FeatureEnvy), perche' il metodo updateKeyboardState sta letteralmente invidiando la classe keyboard.

Questo metodo urla per essere mosso nella classe Keyboard e vedi mai che muoverlo li' non semplifichi qualche altro pezzo di codice. Un bel Move Method (http://c2.com/cgi/wiki?MoveMethod) qui non ce lo toglie nessuno.

(Sta cosa delle puzze non me la sono inventata io eh...)
:) Interessante.

Io avevo fatto di tutto per cercare di non passare un dettaglio implementativo (keys, in questo caso) all'interfaccia Keyboard, e questo ha comportato una complicazione dell'interfaccia e delle classi che la implementavano.

Devo ancora abituarmi a entrare nell'ottica di risolvere il problema nel più breve e semplice modo possibile, mettendo da parte le convinzioni che ho accumulato finora... :p

[cionci]

fek: hai epr caso controlalto quel plugin ?

[fek]

fek: hai epr caso controlalto quel plugin ?

Totalmente dimenticato. Scusami.

[cionci]

No problem, lo ricordavo solo perchè potrebbe essere utile per tutti averlo installato...

[fek]

No problem, lo ricordavo solo perchè potrebbe essere utile per tutti averlo installato...

Giurin giuretto, nel fine settimana ci guardo.

[fek]

parseConfigEntryType (complessita' 5)
parseConfigDocument (complessita' 6)

Indovinate? :)

[cionci]

Io no le ho toccate...mi avevi detto che andavano bene :D

[fek]

Io no le ho toccate...mi avevi detto che andavano bene :D

Poi ho riguardato le metriche ed erano le prime due :p

Mi riposti il link del plugin per favore?

[cionci]

http://sourceforge.net/project/showfiles.php?group_id=128057&package_id=162704&release_id=355559

Devo fare il refactoring di quel codice ?

[fek]

http://sourceforge.net/project/showfiles.php?group_id=128057&package_id=162704&release_id=355559

Devo fare il refactoring di quel codice ?

Non e' strettamente necessario. La decisione sta a te qui.

[cionci]

Per parseConfigDocument c'è una suddivisione logica abbastanza ovvia...per l'altro no...concordi ?

private String parseConfigEntryType(Node node) throws ConfigException
{
NamedNodeMap attributes = node.getAttributes();

if(attributes.getLength() == 0)
{
return "string";
}

Node type = attributes.item(0);
if(type.getNodeName() != "type")
{
throw new ConfigException("Malformed XML config file");
}

if(type.getNodeValue().compareToIgnoreCase("int") == 0)
{
return "int";
}
else if(type.getNodeValue().compareToIgnoreCase("string") == 0)
{
return "string";
}

throw new ConfigException("Malformed XML config file");
}

[cionci]

Ecco la suddivisione di parseConfigDocument:

private void parseConfigEntryList(Document doc) throws ConfigException
{
NodeList listOfEntry = doc.getElementsByTagName("entry");
for(int i = 0; i < listOfEntry.getLength(); i++)
{
Node entry = listOfEntry.item(i);
if(entry.getNodeType() == Node.ELEMENT_NODE)
{
parseConfigEntry(entry);
}
}
}


private void parseConfigDocument(Document doc) throws ConfigException
{
try
{
doc.getDocumentElement().normalize();
Node root = doc.getDocumentElement();

if(root.getNodeName() != "config" || root.hasAttributes())
{
throw new ConfigException("Malformed XML config file");
}

parseConfigEntryList(doc);
}
catch(Exception e)
{
throw new ConfigException(e.getMessage());
}
}

[fek]

Per parseConfigDocument c'è una suddivisione logica abbastanza ovvia...per l'altro no...concordi ?

Per l'altro avevamo deciso di aspettare che avessimo altri tipi prima di fare refactoring verso un pattern. In fondo la complessita' e' 5 ed e' ancora gestibile.

[cionci]

Ok... Ho fatto il commit...