[C++] Classe che genera numeri a caso

[The_ouroboros]

E da un po che non programmavo e oggi ho ripreso...

#include <cstdlib>
#include <ctime>

class Gen
{
private:
    int  *list;
    int  range;
    int  seed;
    int  gen_casual();
    void SetRange(int v=100){ range=v;  };
    int  GetRange(){ return range;  };
public:
    Gen(int c, int s=time(NULL));
    ~Gen();
    int *GetList(int n=50);
};

Gen::Gen(int c, int s)
{
    seed = s;
    SetRange(c);
    srand(seed);
}

Gen::~Gen()
{
    // TODO
    delete list;
}
int Gen::gen_casual()
{
    return (int)( rand() % GetRange() + 1 );
}

int *Gen::GetList(int n)
{
    list =  new int[n];
    for(int i=0; i < n; i++)
    {
        list[i] = gen_casual();
    }
    return list;
}

Va bene come progettazione di classe o è troppo incasinata???

[Xaratroom]

E da un po che non programmavo e oggi ho ripreso...

codice:#include <cstdlib>
#include <ctime>

class Gen
{
private:
int *list;
int range;
int seed;
int gen_casual();
void SetRange(int v=100){ range=v; };
int GetRange(){ return range; };
public:
Gen(int c, int s=time(NULL));
~Gen();
int *GetList(int n=50);
};

Gen::Gen(int c, int s)
{
seed = s;
SetRange(c);
srand(seed);
}

Gen::~Gen()
{
// TODO
delete list;
}
int Gen::gen_casual()
{
return (int)( rand() % GetRange() + 1 );
}

int *Gen::GetList(int n)
{
list = new int[n];
for(int i=0; i < n; i++)
{
list[i] = gen_casual();
}
return list;
}

Va bene come progettazione di classe o è troppo incasinata???

Secondo me non va bene ...
Il distruttore è fatto male. devi deallocare un array con delete [] ptr ;
e non delete ptr ...
Inoltre se GetList viene invocata più volte ?
Ci devi mettere qualche controllo ( sia nel distruttore che in GetList )...

Come idea o come classe astratta sarebbe ok ...

EDIT:
per convenzione si uilizza l'identificatore di classe con prima lettra maiuscola e dentificatore di metodo o attributo con prima lettere minuscola ...
In entrambi i casi eventuali parole di senso compiuto, consecutive alla prima, vanno poste con iniziale maiusole:
GeneraNumeriCasuali -> id classe
ottieniValoreCasuale -> id metodo

[shodan]

Oltre a quello che ha detto Xaratroom, personalmente la trovo proprio assurda!

Domanda: è solo un esercizio o dovrebbe avere un uso pratico?

In ogni caso:

così come l'hai scritta, SetRange e GetRange avrebbero senso se fossero pubbliche, non private. Puoi usare direttamente le variabili a prescindere dalla visibilità delle funzioni.

In genere poi è meglio settare le variabili nella lista del costruttore.

Gen::Gen(int c, int s) : range(c) , seed(s) 
{
//    seed = s;
//    SetRange(c);  /* qui è inutile a prescindere se pubblica o privata */
    srand(seed);
}

Poi allochi un puntatore *list e non metti un costruttore di copia per evitare che venga compiuta una delete due volte al termine dello scope di chiamata.

Gen a(10, 10);
Gen b = a; // copia bit a bit. Alla fine *list viene distrutto due volte

Infine la *GetList();

Gen a(10,20);

int* result = a.GetList(50);
delete result; //  <-- chi dice che un int* sia un array?

E poi ogni chiamata a GetList alloca memoria che non verrà liberata che nel distruttore.

int *Gen::GetList(int n)
{
    list =  new int[n];   // <-- errore!!!
    for(int i=0; i < n; i++)
    {
        list[i] = gen_casual();
    }
    return list;
}
.....................

Gen a(10,20);

int* result = a.GetList(50);
int* result1 = a.GetList(30);
int* result2 = a.GetList(5);
int* result3 = a.GetList(56);

Quattro allocazioni e solo una distruzione (forse)
per evitare sto spreco basta fare

std::vector<int> Gen::GetList(int n)
{

    std::vector<int> v(n);
    for(int i=0; i < n; i++)
    {
        v[i] = gen_casual();
    }

    return v;
}

e non si hanno memory leak.

[Xaratroom]

Arrg non mi ero accorto dei metodi private (set range può pure andare, ma gli altri no e cmq è buona regola lasciarlo public) ...
Cmq int * secondo me va più che bene ...
Naturalmente

#include <cstdlib>
#include <ctime>

class Gen
{
private:
    int  *list;
    int  range;
    int  seed;
public:
    void SetRange(int v=100){ range=v;  };
    int  gen_casual();
    int  GetRange(){ return range;  };
    Gen(int c, int s=time(NULL));
    ~Gen();
    int *GetList(int n=50);
};

Gen::Gen(int c, int s)
{
    seed = s;
    SetRange(c);
    srand(seed);
}

Gen::~Gen()
{
    // TODO
    if (list) 
       delete [] list;
}
int Gen::gen_casual()
{
    return (int)( rand() % GetRange() + 1 );
}

int *Gen::GetList(int n)
{
   if (n > 0)
    {if (list)
       delete [] list;
    list =  new int[n];
    for(int i=0; i < n; i++)
    {
        list[i] = gen_casual();
    }
    return list;}
   return NULL;
}

EDIT
Mancava anche il controllo su n in get list
Inoltre potresti fare anche la sranf di time(NULL) (con casting ad unsigned)...

[shodan]

Originariamente inviato da Xaratroom
Cmq int * secondo me va più che bene ...

Purtroppo no.

Gen a(10,20);

int* result = a.GetList(50);
delete result; //  <-- chi dice che un int* sia un array?

Il rischio di un delete esplicito è troppo elevato. Se si vuole tenere un puntatore come valore di ritorno, allora deve essere specificato che deve essere distrutto al di fuori della classe, lasciando poi il distruttore vuoto.
Inoltre anche il tipo di delete deve essere documentato chiaramente. (delete o delete[] ?)

Si potrebbe essere tentati di inserire il puntatore in un std::auto_ptr per evitare memory leak a causa di eccezioni non previste, e lo std::auto_ptr serve per oggetti, non per array.

Non servirebbe nemmeno dichiarare il prototipo in questo modo:

const int *Gen::GetList(int n) const

Il delete verrebbe invocato comunque come prevede lo standard (se non ricordo male).
Poi ognuno è libero di fare come vuole ovviamente.

[Xaratroom]

Non ci avevo pensato XD...
neanche al delete [] ...
Vabbè c'è da lavorare ancora