Verbesserungsvorschläge für meine Klasse

Patrick

Hi,

ich habe eine Klasse für 2D Arrays geschrieben auf Templatebasis.

Jetzt würden mich folgende Punkte interessieren:

1. Meinung
2. Vorschläge zur Verbesserung
3. Vorschläge zur Erweiterung
4. evtl. Optimierungen?

Hier der Code:

// Dynamisches 2D Array
template<typename T> class array2d 
{ 
public: 
        // C'tor und D'tor
    array2d (unsigned long rows, unsigned long cols) : rows_(rows), cols_(cols)
    { 
            // 2-Dimensionales Array auf 1-Dimensionales abbilden 
        data_ = new T[rows_*cols_]; 
    } 
    array2d (void) : rows_(0), cols_(0), data_(NULL)
    { } 
    ~array2d (void) 
    { clear (); } 

        // Aufräumfunktion
    inline void clear (void)
    {
            // Nur aufräumen wenn es nötig ist
        if (data_ != NULL)
        {
            delete [] data_; 
            data_ = NULL;
        }
    }

        // Ausrichtungsfunktion
    inline void resize (unsigned long rows, unsigned long cols)
    { 
        clear ();

            // Angaben privatisieren
        rows_ = rows;
        cols_ = cols;

            // 2-Dimensionales Array auf 1-Dimensionales abbilden 
        data_ = new T[rows_*cols_]; 
    } 

        // Zugriffsoperator
    inline T* operator[] (unsigned long line) 
    { 
            // Array liegt "zeilenweise" im Speicher 
        return (&data_[line * cols_]); 
    } 

        // Leseoperator
    inline T* operator[] (unsigned long line) const
    { 
            // Array liegt "zeilenweise" im Speicher 
        return (&data_[line * cols_]); 
    } 

        // Zuweisungsoperator
    inline array2d<T>& operator = (const array2d<T>& other)
    {
            // Alten speicher freigeben
        clear ();

            // Größe ändern
        resize (other.rows_, other.cols_);

            // Überweisung
        for (unsigned long col=0; col<cols_; ++col) 
            for (unsigned long row=0; row<rows_; ++row) 
                (*this)[col][row] = other[col][row];

        return (*this);
    }

        // Diverse Getter
    inline unsigned long rows (void) { return (rows_); }
    inline unsigned long cols (void) { return (cols_); }
    inline unsigned long size (void) { return (rows_*cols_); }

private: 
        // Datenarray sowie Größenangaben
    T*              data_; 
    unsigned long   rows_;
    unsigned long   cols_; 
};

Danke im voraus!

das groebste auf einen blick:
die inline's kannst du weglassen, die funktionen sind ohnehin inline definiert. wenn ich das richtig sehe fehlt noch ein copy-ctor. in deinem zuweisungsoperator kannst du clear weglassen-das wird ja nochmal in resize aufgerufen. am indexoperator koennte noch sowas wie ein zugriffsschutz rein. resize koennte auch soweit wie moeglich vorhandene werte in ein neu angelegtes array mitnehmen(also mit temporaerer kopie), so dass nicht jedes mal wenn ich ein bereits vorhandenes array erweitern will alle daten weg sind.

MFK

- Die Abfrage auf != NULL kann man vor delele[] weglassen.
- Ein vector als interner Datencontainer würde vieles vereinfachen.
- Warum geben die op[] Zeiger zurück?

groovemaster

1. statt unsigned long besser size_t verwenden
2. du kannst im ctor generell data_ in die Initialisierungsliste setzen
3. lass das void in Funktionen mit keinem Parameter weg
4. in clear solltest du auch rows_ und cols_ auf 0 setzen
5. ändere die Reihenfolge bei der Speicherallokation - zuerst Speicher reservieren, dann rows_ und cols_ setzen - genauso bei der Speicherfreigabe
6. möglich wäre auch, das Speichermanagement grundsätzlich zu ändern - zuerst neuen Speicher anfordern und dann den alten Speicher freigeben
7. mit Punkt 6 könntest du damit auch resize richtig implementieren, dh bisherige Inhalte bleiben erhalten
8. biete noch eine weitere Funktion an (zB reserve), die im Grunde genau das macht, was resize jetzt macht
9. evtl. kann man später für die Rückgabe von op[] noch ein Proxy machen, momentan reichen die Zeiger aber aus
10. gib beim const op[] auch const T* zurück
11. in op[] gehört ein assert für die Indexprüfung
12. achte generell auf const-correctness (zB bei rows(), cols(), size())
13. wie schon erwähnt, lass das inline weg
14. ist zwar eher eine persönliche Geschmacksfrage, aber die Klammerung bei return kann man auch weglassen - imo etwas lesbarer
15. ebenfalls schon erwähnt, ein copy ctor fehlt

volkard

eigentlich wurde schon alles gesagt. aber naja, ich sag's nochmal.

template<typename T> 
class array2d 
{ 
public: 
        // C'tor und D'tor
    array2d (unsigned long rows, unsigned long cols) : rows_(rows), cols_(cols)
    { 
            // 2-Dimensionales Array auf 1-Dimensionales abbilden 
        data_ = new T[rows_*cols_]; 
//peng. ich schieße mal senkrecht in die luft, statt in meinen kopf, um 
//aufmerksamkeit zu erregen. 
//nimm echt ein array1d als konstruier- destruier- assert- copy- geschichte. 
//ist genug für eine klasse. 
    } 
    array2d (void) : rows_(0), cols_(0), data_(NULL)
//schreib doch 0 statt NULL
    { } 
    ~array2d (void) 
//und schreib kein (void)
    { clear (); } 
//hä, was macht denn clear()???

        // Aufräumfunktion
    inline void clear (void)
    {
            // Nur aufräumen wenn es nötig ist
        if (data_ != NULL)
        {
            delete [] data_; 
            data_ = NULL;
        }
//ähm. *hüstel*. prima. aber kein mensch braucht sowas. 
    }

        // Ausrichtungsfunktion
    inline void resize (unsigned long rows, unsigned long cols)
//inline kann weg, wie gesagt. 
    { 
        clear ();

            // Angaben privatisieren
//lustiger kommentar
        rows_ = rows;
        cols_ = cols;

            // 2-Dimensionales Array auf 1-Dimensionales abbilden 
//merken wir uns mal diese absicht
//*notizmach*
        data_ = new T[rows_*cols_]; 
    } 

        // Zugriffsoperator
    inline T* operator[] (unsigned long line) 
//T* ist ungeschickt. damit verlierste assert-möglichkeit bei 
//indexgrenzenfehler. und das ist immerhin einer der häufigsten fehler, die 
//überhaupt gemacht werden. 
    { 
//und warum unsigned long statt size_t?
            // Array liegt "zeilenweise" im Speicher 
        return (&data_[line * cols_]); 
    } 

        // Leseoperator
    inline T* operator[] (unsigned long line) const
//rückgabe eher T const*
    { 
            // Array liegt "zeilenweise" im Speicher 
        return (&data_[line * cols_]); 
    } 

        // Zuweisungsoperator
//autsch. die kugel kam runter und ist mir auf den fuss gefallen. 
    inline array2d<T>& operator = (const array2d<T>& other)
//weg! sowas darf nicht passieren. es muss wirklich ein array1d her. 
    {
            // Alten speicher freigeben
        clear ();

            // Größe ändern
        resize (other.rows_, other.cols_);

            // Überweisung
        for (unsigned long col=0; col<cols_; ++col) 
            for (unsigned long row=0; row<rows_; ++row) 
                (*this)[col][row] = other[col][row];

        return (*this);
    }

        // Diverse Getter
    inline unsigned long rows (void) { return (rows_); }
    inline unsigned long cols (void) { return (cols_); }
    inline unsigned long size (void) { return (rows_*cols_); }

private: 
        // Datenarray sowie Größenangaben
    T*              data_; 
    unsigned long   rows_;
    unsigned long   cols_; 
};

so, um was ging es eigentlich?

2-Dimensionales Array auf 1-Dimensionales abbilden

stimmt.

dann bauen wir doch mal ne klasse, die NUR das macht.

//ungetestet, nur so reingehackt.
temlate<typename T>
class array2d{
private:
   array1d<T> data;//wir wollen ja nur abbilden
   size_t sizex;//mehr attribute sind nicht nötig
public:
   array2d(size_t _sizey,size_t _sizex):
      data(_sizey*_sizex),
      _sizex(sizex){//nur abbilden
   }
   T& at(size_t y,size_t x){//nur abbilden
      assert(x<sizex);
      return data[y*sizex+x];
   }
//so, fertig ist die klasse. sie kann jetzt mit at() 2d-zugriffe und sie 
//kann ctor, dtor, copy-ctor, zuweisungsop, sie testet auf 
//indexgrenzenüberschreintung in x- und in y-richtung. 
//jetzt kommt noch ein wenig streuselzucker drauf und dann pack ich sie ein 
//und verkauf sie. 
   class proxy{
      private:
      array2d<T>& array;
      size_t y;
      public:
         proxy(array2d<T>& _array,size_t _y):
            array(_array),
            y(_y){
         }
         T& operator[](size_t x){
            return array.at(y,x);
         }
   }
   proxy operator[](size_t y){
      return proxy(*this,y);
   }
};

so, fertig.

und wenn du das array1d schreibst, dann kannste prima die sachen mit eindimensionalen schleifen implementieren, das ist doch viel angenehmer.

groovemaster

volkard schrieb:

so, fertig.

Fast. Mir fehlt noch die Indexprüfung für y. Also sowas in der Art

assert(y < data.size() / sizex);

Aber ich will ja nicht meckern. :p

volkard

groovemaster schrieb:

volkard schrieb:

so, fertig.

Fast. Mir fehlt noch die Indexprüfung für y. Also sowas in der Art
assert(y < data.size() / sizex);
Aber ich will ja nicht meckern. :p

wir vergessen wohl, daß das array1d bereits dagegen prüft.

magst sagen, daß cobst fehlt. ja, das überlasse ich mal als übung dem geneugten leser.

groovemaster

volkard schrieb:

wir vergessen wohl, daß das array1d bereits dagegen prüft.

Ups, hast Recht. Sry, vergessen Gehirn wieder einzuschalten.

Thx für die Hilfreichen Tipps! naja size_t ist nie mein fall gewesen aber ich guck mal ob ich mich nicht doch dran gewöhnen kann

wegen der if-abfrage: Was wenn der D'tor 2x aufgerufen wird? 1x wird der Speicher geleert und ein anderes mal wieder? Kann das nicht etwas "weh" tun? oder macht das 2. delete nichts mehr?

volkard

Patrick@Friends schrieb:

wegen der if-abfrage: Was wenn der D'tor 2x aufgerufen wird?

kann nicht passieren.

aber clear() kann 2x aufgerufen werden. desdawega hat mein vorschlag auch kein clear. bei mir hat der zeiger niemals den wert 0.

[/quote]1x wird der Speicher geleert und ein anderes mal wieder? Kann das nicht etwas "weh" tun? oder macht das 2. delete nichts mehr? [/quote]
in der tat ist es sicher, denn im delete steht dummerweise auch ein if(p!=0) drin. also int* p=0;delete p; ist gültig und geschieht ohne jeden fehler.