wie kann man das optimieren?

otze

so, hoffe ihr seid zufriedener mit folgendem code:

IBuffer* operator-=(IBuffer* buffer){
    if(buffer->getUser()==this){
        buffer->UnUse();
        removeTree(buffer);
    }
    return this;
}

void removeTree(IBuffer* buffer){
    if(buffer!=this&&buffer!=User){
        if(User!=NULL){
            User->removeTree(buffer);
        }
        Container::const_iterator buffer_map_iterator;
        Container& buffer_map=*(buffer->getTree());
        for(buffer_map_iterator=buffer_map.begin();buffer_map_iterator!=buffer_map.end();++buffer_map_iterator){

            Desc& description=buffer_map_iterator->first;
            List& buffer_data=buffer_map_iterator->second;
            List& this_data=Tree.find(description)->second;
            Container::const_iterator& data_pos=Tree.find(description);

            if(data_pos!=Tree.end()){
                if(this_data.size()==buffer_data.size()){
                    Tree.erase(description);
                }
                else
                {
                    List::iterator this_iterator=this_data.begin();
                    List::iterator buffer_iterator=buffer_data.begin();

                    for(;buffer_iterator!=buffer_data.end();++buffer_iterator){
                        while(this_iterator!=this_data.end()){
                            if(*this_iterator==*buffer_iterator){
                                this_data.erase(this_iterator);
                            }
                            ++this_iterator;
                        }
                    }
                }
            }
        }
    }
}

Nein, eigentlich nicht. Wenn ich mehr als drei Einrückungen innerhalb einer Funktion sehe, wird mir immer so mulmig im Magen...
Warum lagerst du nicht einzelne Teilbereiche in Funktionen aus? So erkennst du auch noch viel leichter, ob du evtl. was zuviel machst...
Und buffer_map_iterator ist ein ur-hässlicher Name. iterator würde reichen.

Jester

Schon besser, aber die innere for-schleife (im else-Fall) würde ich in ne Funktion auslagern. Und dann solltest Du wirklich noch Shades rat befolgen und die if rumdrehen:

statt

if(a)
{
  if(b)
  {
    if(c)
    {
      ...
    }
  }
}

lieber

if(!a) return;
if(!b) return;
if(!c) return;
...

Denn der else Teil ist ja sowieso leer. Dann mußt Du nicht so stark einrücken, das mindert die Komplexität.

MfG Jester

Jester

Ich bin so frei und passe mal ein bißchen was an. Achtung, ist nicht getestet und möglicherweise fehlerhaft. Aber es macht die Sache deutlich einfacher.

void removeTree(IBuffer* buffer){
  if(buffer==this || buffer==User) return;

  if(User)
  {
    User->removeTree(buffer);
  }

  Container& buffer_map=*(buffer->getTree());

  for(Container::const_iterator p=buffer_map.begin();p!=buffer_map.end();++p)
  {
    Desc& description=p->first;
    List& buffer_data=p->second;
    List& this_data=Tree.find(description)->second;
    Container::const_iterator& data_pos=Tree.find(description);

    if(data_pos==Tree.end()) continue;

    if(this_data.size()==buffer_data.size()){
      Tree.erase(description);
    }
    else
    {
      List::iterator this_iterator=this_data.begin();
      List::iterator buffer_iterator=buffer_data.begin();

      // Durch Funktionsaufruf und/oder Algorithmen aus der Standardbibliothek erstzen   
      /*
      for(;buffer_iterator!=buffer_data.end();++buffer_iterator)
      {
        while(this_iterator!=this_data.end())
        {
          if(*this_iterator==*buffer_iterator)
          {
            this_data.erase(this_iterator);
          }
          ++this_iterator;
        }
      }
      */
    }
  }
}

MfG Jester

edit: mir fällt gerade auf: warum nicht den kompletten Teil innerhalb der äußeren for-Schleife in eine Funktion packen?
Dann wird die Hauptfunktion sehr kurz und einfach und die Unterfunktion ist auch nicht mehr so lang und kaum eingerückt.

otze

hmm da kamen wir fast aufs selbe ergebnis:

void delete_similar(List& this_data,List& buffer_data){
    List::iterator this_iterator=this_data.begin();
    List::iterator buffer_iterator=buffer_data.begin();

    for(;buffer_iterator!=buffer_data.end();++buffer_iterator){
        while(this_iterator!=this_data.end()){
            if(*this_iterator!=*buffer_iterator)continue;
            this_data.erase(this_iterator);
            ++this_iterator;
        }
    }
}

void removeTree(IBuffer* buffer){
    if(buffer==this||buffer==User)return;
    if(User){
        User->removeTree(buffer);
    }
    Container& buffer_map=*(buffer->getTree());
    for(Container::const_iterator p=buffer_map.begin();p!=buffer_map.end();++p){

        Desc& description=p->first;
        List& buffer_data=p->second;
        List& this_data=Tree.find(description)->second;
        Container::const_iterator& data_pos=Tree.find(description);

        if(data_pos==Tree.end())continue;
        if(this_data.size()==buffer_data.size()){
            Tree.erase(description);
        }
        else
        {
            delete_similar(this_data,buffer_data);
        }
    }
}

mehr würde ich da jetzt auch nichmehr rauspacken, weil das dann eher an fledderei grenzen würde denn was bliebe von deleteTree, wenn ich jetzt alles rauspacken würde, was innerhalb der for schleife wär-das wird dann wieder unübersichtlicher, weil man sich x funktionen aus dem qc suchen müsste...

phlox81

Interessant was ihr hier so macht

Bringts denn was in der Performance ?

Devil

otze

die referenzen sparen gut was ein,teilweise statt 4x aufrufen der funktionen mit dereferenzierungen nurnoch einmal^^

...
        Desc& description=p->first;
        List& buffer_data=p->second;
        List& this_data=Tree.find(description)->second;
        Container::const_iterator& data_pos=Tree.find(description);
...

Das ist nicht dein ernst.

...
        Desc& description=p->first;
        List& buffer_data=p->second;
        Container::const_iterator& data_pos=Tree.find(description);
        List& this_data=data_pos->second;
...

Und schwups ist das ganze duetlich schneller.

Vellas

Hi!

otze schrieb:

hmm da kamen wir fast aufs selbe ergebnis:

void delete_similar(List& this_data,List& buffer_data){
    List::iterator this_iterator=this_data.begin();
    List::iterator buffer_iterator=buffer_data.begin();

    for(;buffer_iterator!=buffer_data.end();++buffer_iterator){
        while(this_iterator!=this_data.end()){
            if(*this_iterator!=*buffer_iterator)continue;
            this_data.erase(this_iterator);
            ++this_iterator;
        }
    }
}

Die while-Schleife landet bei continue in einer Endlosschleife. Denn wenn das continue ausgeführt wird, wird doch das ++this_iterator nicht mehr ausgeführt und somit wird die if-Abfrage immer mit true ausgewertet. Mit einer for-Schleife klappt das aber wunderbar, wodurch es dann mit dem continue funktioniert.

for(;this_iterator!=this_data.end();++this_iterator){
            if(*this_iterator!=*buffer_iterator)continue;
            this_data.erase(this_iterator);
        }

Code-Hacker

Shade Of Mine

devil81 schrieb:

Bringts denn was in der Performance ?

Make it work, make it right, make it fast.

otze

so, hoffe, alle beteiligten sind nun halbwegs mit dem ergebnis zufrieden, hab zuvor noch nie komplexere verschachtelungen programmiert, konnte also am anfang nicht ahnen, wie Krank das am ende werden würde...