valutazione codice "migliore"

[newutente]

ok, grazie.

[newutente]

EDIT

[newutente]

Vorrei analizzare insieme a voi il codice di un esercizio che sto facendo.
In poche parole questo esercizio deve gestire un portamonete e le sue monete con aggiunte e rimozioni di monete.
Mi sono bloccato sulla rimozione di una moneta dal portamonete.

Vi riporto il codice della classi:

public class TestMonete
{
    public static void main(String args[])
    {
        
        Moneta moneta1 = new Moneta(20);
        Moneta moneta2 = new Moneta(1);
        Moneta moneta3 = new Moneta(100);
                

        //creazione portamonete con 8 monete
        PortaMonete portamonete1 = new PortaMonete(2,5,100,10,50,10,100,200);
        
        // creazione portamonete con 11 monete
        PortaMonete portamonete2 = new PortaMonete(2,5,100,10,50,10,100,200,10,5,2);
        
        // aggiunta moneta da 50 centesimi a portamonete1
        portamonete1.aggiungi(new Moneta(50));

        
        portamonete1.stato();
        
        
        portamonete1.preleva(50);
        
    }
}

public class Moneta 
{
    public final static String VALUTA="EURO";
    
    private final int valore;
    

    public Moneta(int valore)
    {
        this.valore = valore;
        System.out.println("Creata " + getDescrizione());
    }

  
    public String getDescrizione()
    {
        String descrizione = "moneta da: " + formattaStringaDescrittiva(valore) + VALUTA;
        return descrizione;
    }
    
    
    private static String formattaStringaDescrittiva(int valore)
    {
        String stringa = " centesimi di ";
        if (valore == 1)
        {
                 stringa = " centesimo di ";
                    }
        else if (valore > 99)
        {
            stringa = " ";
            valore /= 100;
                        
        }
        return valore + stringa;
    }

    

    public int getValore()    
    {
            return valore;
    }

}

Questo è il codice della classe PortaMonete fatta da me:

public class PortaMonete
{ 
    
    private final Moneta monete [] = new Moneta [10];
    
    
    public PortaMonete(int... valori)
    {
        int numeroMonete = valori.length;
        for(int i=0; i<numeroMonete; i++)
        {
            if (i>=10)
            {
                System.out.println("Sono state inserite 10 monete \n");
                break;
            }
            monete[i] = new Moneta(valori[i]);
        }
    }

    
    
    public void aggiungi(Moneta moneta)
    {    
        System.out.println("Proviamo ad aggiungere una " + moneta.getDescrizione());
        int indiceLibero = primoIndiceLibero();        
        if (indiceLibero == -1)
        {
            System.out.println("Portamonete pieno \n");        
        }
        else
        {
            monete[indiceLibero] = moneta;
            System.out.println("Moneta inserita correttamente in posizione " + indiceLibero + "\n");
        }
    }


    
    private int primoIndiceLibero()
    {
        int indice = -1;
        for(int i=0; i<10; i++)
        {
            if(monete[i]==null)
            {
                indice = i;
                break;
            }                
        }
        return indice;
    }



    public void stato()
    {
        for(Moneta moneta : monete)   
        {
            if(moneta==null)
            {
                System.out.println("scansione terminata \n");   
                break;
            }
            System.out.println(moneta.getDescrizione());       
        }
        
    }

        
    public void preleva(int valoreMoneta)
    {    
        for(int i=0; i<monete.length; i++)
        {
            if(monete[i].getValore()==valoreMoneta)
            {
                System.out.println("moneta trovata in posizione: " + i);
                monete[i]=null;
                System.out.println("moneta rimossa");
            }
        }    
    } 
    
}

Rispetto alla soluzione proposta dal manuale le differenze sono:

portaMonete.preleva(moneta1);

nel main al posto di:

portamonete1.preleva(50);

e:

    public Moneta preleva(Moneta moneta) {
        System.out.println("Proviamo a prelevare una "  + moneta.getDescrizione());
        Moneta monetaTrovata = null;
        int indiceMonetaTrovata = indiceMonetaTrovata(moneta);
        if (indiceMonetaTrovata == -1) {
            System.out.println("Moneta non trovata!");
        } else {
            monetaTrovata = moneta;
            monete[indiceMonetaTrovata] = null;
            System.out.println("Una " + moneta.getDescrizione() + " prelevata");
        }
        return monetaTrovata;
    }
    

    private int indiceMonetaTrovata(Moneta moneta) {
        int indiceMonetaTrovata = -1;
        for (int i = 0; i < 10; i++) {
            if (monete[i] == null) {
                break;
            }
            int valoreMonetaNelPortaMoneta = monete[i].getValore();
            int valore = moneta.getValore();
            if (valore == valoreMonetaNelPortaMoneta) {
                indiceMonetaTrovata = i;
                break;
            }
        }
        return indiceMonetaTrovata;
    }

al posto del mio metodo preleva.

La mia soluzione sembra funzionare, o meglio trova la moneta all'interno dell'array di monete però poi restituisce una NullPointerException.

A vostro avviso la mia soluzione è accettabile (non considerando la NullPointerException)?

[newutente]

EDIT

[valia]

Se hai una NullPointerException la soluzione NON è accettabile (il programma ha un errore che non sai o non vuoi correggere).
Secondo, tu aggiungi/prelevi una moneta, sai tu che 50 si riferisce a 50 cent, potrebbero essere euri, uova, pomodori o che so io.
La soluzione del libro resta ancora una volta migliore, perché tratta interazioni tra oggetti

[newutente]

ok, capito.

Dando un'occhiata alla soluzione del manuale (visto che ero fuori strada) ho provato a rifare la mia.
Vi posto solo il codice dei metodi preleva e del metodo utility indiceMonetaTrovata

public void preleva(Moneta moneta)
    {
        System.out.println("Proviamo a prelevare una " + moneta.getDescrizione());
        int indiceDaCercare = indiceMonetaTrovata(moneta);
        if(indiceDaCercare==-1)
        {
            System.out.println("moneta non trovata \n");
        }
        else 
        {
            monete[indiceDaCercare]=null;
            System.out.println("E' stata prelevata una " + moneta.getDescrizione());
            for(int i=indiceDaCercare; i<monete.length; i++)
               {    
                      monete[indiceDaCercare]=monete[indiceDaCercare+1];
               }            
        }
    }
    
    
    private int indiceMonetaTrovata(Moneta moneta)
    {
        int indiceMonetaTrovata = -1;
        for(int i=0; i<10; i++)
        {
            if(monete[i]==null)
            {
                break;
            }
            else if(monete[i].getValore()==moneta.getValore())
            {
                indiceMonetaTrovata = i;
                break;
            }
        }
        return indiceMonetaTrovata;
    }

Ora funziona però ho un dubbio sul pezzo di codice che vi ho evidenziato.
Se io non faccio quel controllo (mi sa che neanche la soluzione del libro lo riporta) nel momento in cui vado a stampare, dopo l'eventuale rimozione della moneta, il contenuto del portamonete sarà stampato il contenuto fino all'elemento immediatamente precedente a quello rimosso e questo perchè (tramite il metodo stato()) appena viene riconosciuto l'elemento null all'interno dell'array si interrompe la stampa.
Ho provato così a inserire quelle righe però viene semplicemente copiato il valore dell'elemento successivo a quello cancellato nella posizione dell'elemento cancellato.
Ad esempio io ho questo array di oggetti Moneta con questi valori:

2
5
1
10
50
10
1
2
50

e voglio eliminare una moneta da 50 centesimi. Mandando in esecuzione il programma mi viene stampato questo array:

2
5
1
10
10
10
1
2
50

[valia]

intanto farei due modifiche al metodo cercaIndice

 private int indiceMonetaTrovata(Moneta moneta) {
        int indiceMonetaTrovata = -1;
        for(int i=0; i< monente.length && monete[i] != null; i++) {
             if(monete[i].getValore()==moneta.getValore()) {
                indiceMonetaTrovata = i;
                break;
            }
        }
        return indiceMonetaTrovata;
}

aumenti la leggibilità del codice.
Poi nota che di monete esiste una property che dice quanto è lungo l'array: usala.
La rimozione di una moneta comporta lo scorrimento di tutto l'array, quindi questa dovrebbe funzionare

public void preleva(Moneta moneta) {
        System.out.println("Proviamo a prelevare una " + moneta.getDescrizione());
        int indiceDaCercare = indiceMonetaTrovata(moneta);
        if(indiceDaCercare > -1){
            System.out.println("E' stata prelevata una " + moneta.getDescrizione());
            for(int i=indiceDaCercare; i < monete.length - 1; i++) {    
                monete[i]=monete[i+1];
            }            
            monete [monete.length -1 ] = null;
        }else {
            System.out.println("moneta non trovata \n");
        }
}

[luca200]

Vorrei un vostro parere su un altro esercizio.
La traccia è la seguente:
Creare una classe con un metodo main() che selezioni i primi 10 numeri divisibili per 3....

La tua soluzione qui ha un errore di base che nessuno ti ha fatto notare.
Ti si chiede di trovare i primi 10 numeri divisibili per 3, ed è quello che fa la soluzione del tuo libro. Il tuo codice invece trova i numeri divisibili per 3 contando fino a 30, senza preoccuparsi di quanti sono. Ovviamente il risultato è lo stesso, ma solo perché tu hai fatto preventivamente il calcolo a mente. Il tuo codice però non fa ciò che ti viene chiesto.

[newutente]

public void preleva(Moneta moneta) {
        System.out.println("Proviamo a prelevare una " + moneta.getDescrizione());
        int indiceDaCercare = indiceMonetaTrovata(moneta);
        if(indiceDaCercare > -1){
            System.out.println("E' stata prelevata una " + moneta.getDescrizione());
            for(int i=indiceDaCercare; i < monete.length - 1; i++) {    
                monete[i]=monete[i+1];
            }            
            monete [monete.length -1 ] = null;
        }else {
            System.out.println("moneta non trovata \n");
        }
}

Quindi più che settare l'elemento con indice corrispondente a indiceDaCercare a null hai copiato in quella posizione l'elemento successivo e a scorrere tutti gli altri.
L'output effettivamente è corretto e per la precisione è questo:

2
5
1
10
10
1
2
50
null
null

però io adesso mi chiedo, settando l'elemento in ultima posizione nell'array a null (tramite monete[monete.length-1] = null; ) non vado ad intervenire sull'elemento in posizione 9 che è già null (l'array ha 10 posizioni ma sono stati inseriti 9 elementi)?
In questo caso l'output dovrebbe essere:

2
5
1
10
10
1
2
50
50
null

Ripeto, l'esercizio così funziona, la mia è solo una precisazione per capire bene il funzionamento.

[newutente]

La tua soluzione qui ha un errore di base che nessuno ti ha fatto notare.
Ti si chiede di trovare i primi 10 numeri divisibili per 3, ed è quello che fa la soluzione del tuo libro. Il tuo codice invece trova i numeri divisibili per 3 contando fino a 30, senza preoccuparsi di quanti sono. Ovviamente il risultato è lo stesso, ma solo perché tu hai fatto preventivamente il calcolo a mente. Il tuo codice però non fa ciò che ti viene chiesto.

hai ragione, grazie per la precisazione.