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

[fek]

E finalmente inizio a divertirmi un po' anch'io

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]

Originariamente inviato da fek

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]

Originariamente inviato da 71104

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]

Originariamente inviato da 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

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]

Originariamente inviato da 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

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]

Originariamente inviato da 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:

Ottimo.

[cisc]

Originariamente inviato da fek

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...

[fek]

Originariamente inviato da cisc

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...

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

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

[NA01]

Originariamente inviato da fek

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]

Originariamente inviato da NA01

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
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... 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 )...

[cionci]

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

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

[fek]

Originariamente inviato da cdimauro

Spero che adesso vada bene... 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 )...

Mi hai dato del leone? Sono lusingato

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]

Originariamente inviato da 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 ?

Originariamente inviato da cionci

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/r...ymorphism.html
http://www.refactoring.com/catalog/r...thVisitor.html

Oppure questo sembra sia quello che ti serve:

http://www.refactoring.com/catalog/s...Algorithm.html