wie kann man das optimieren?

otze

so in etwa?

IBuffer* operator-=(IBuffer* buffer){
            buffer->UnUse();
                if(buffer!=this&&buffer!=User){
                    if(User!=NULL){
                        *User-=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& first=buffer_map_iterator->first;
                        List& second=buffer_map_iterator->second;
                        if(Tree.find(first)!=Tree.end()){
                            if(Tree.find(first)->second.size()==second.size()){
                                Tree.erase(first);
                            }
                            else
                            {
                                List::iterator this_list=Tree.find(first)->second.begin();
                                List::const_iterator buffer_list=second.begin();
                                bool found;
                                for(;buffer_list!=second.end();++buffer_list){
                                    found=false;
                                    while(this_list!=Tree.find(first)->second.end()&&found==false){
                                        if(*this_list==*buffer_list){
                                            Tree.find(first)->second.erase(this_list);
                                            found=true;
                                        }
                                        ++this_list;
                                    }
                                }
                            }
                        }
                    }
                }
            }

Jester

Nö. Deine Namen für die Hilfsobjekte beschreiben alle nur deren Typ, aber nicht deren Sinn.
Warum first/second schreiben, wenn Du dem ganzen einfach nen sinnvollen Namen geben kannst?

buffer_map_iterator ist zu lang, beschreibt auch nur den Typ. Dann lieber einfach ein p oder sowas.
Versuch die Namen mal so zu wählen, daß sie beschreiben wozu das Objekt gut ist. Und der Name Container ist zu allgemein. list, vector, map das sind alles container...

Shade Of Mine

Also Performance ist hier wohl das mindeste Problem.

erstmal die typedefs wie Jester schon gesagt hat.

Statt einem

for(...)
{
  if(..)
  {
  }
}

kann man ein schnuckeliges

for(...)
{
  if(!..) continue;
}

machen. dann bleibt der code so schön links.

und ne schleife in einer schleife? sowas kann oft als ne neue Funktion angelegt werden - dann ist der code auch wieder viel schöner.

buffer_map=buffer->getTree()->begin();buffer_map!=buffer->getTree()->end();++buffer_map

wie wäre es mit for_each?

und mit den namen kann man auch nix anfangen
buffer_list, this_list, buffer_map und zwischendurch ein Tree

while(this_list!=Tree.find(buffer_map->first)->second.end()&&found==false){
  if(*this_list==*buffer_list){
    Tree.find(buffer_map->first)->second.erase(this_list);
    found=true;
  }
  ++this_list;
}

wie wäre es mit

while(this_list!=Tree.find(buffer_map->first)->second.end())
{
  if(*this_list==*buffer_list)
  {
    Tree.find(buffer_map->first)->second.erase(this_list);
    break;
  }
  ++this_list;
}

und irgendwie testest du find() nie auf auf end()...

Helium

@Shade:Doch

if(Tree.find(buffer_map->first)!=Tree.end()){ 
   if(Tree.find(buffer_map->first)->...

Siehst du, er prüft. Aber er lässt interessanterweise zweimal suchen, warum auch immer. Er lässt dann noch zweimal suchen. Das das Element vorhanden ist ist sicher, da der Code nur erreicht wird, wenn das Element bei der Äußeren Abfrage vorhanden ist.

Gut, irgendwie kann man es verstehen, das du es nicht erkannt hast, so wie der Code aussieht.

@otze: Merk dir, das gefundene element. Wenn es nicht end() ist rufst du damit 'ne Funktion auf, die dann den ganzen inneren Kram macht. Dann erkennt man zumindest schonmal etwas. Und da dann drei Suchläufe wegfallen ist der Code vermutlich schonmal um einiges schneller.

otze

Shade Of Mine schrieb:

Statt einem
for(...)
{
  if(..)
  {
  }
}
kann man ein schnuckeliges
for(...)
{
  if(!..) continue;
}
machen. dann bleibt der code so schön links.

ich mag die links verschiebung, ohne die Krieg ich sofort orientierungsprobleme....

und ne schleife in einer schleife? sowas kann oft als ne neue Funktion angelegt werden - dann ist der code auch wieder viel schöner.
buffer_map=buffer->getTree()->begin();buffer_map!=buffer->getTree()->end();++buffer_map
wie wäre es mit for_each?

for_each hab ich mir angeschaut,aber ich kam zum schluss, dass es sich nicht so lohnt, dafür hab ich aber den hauptteil der Funktion in eine methode removeTree geparkt, das schien mir ein guter kompromis(und brachte mir die lösung für ein andreres problem)

und mit den namen kann man auch nix anfangen
buffer_list, this_list, buffer_map und zwischendurch ein Tree

hab jetzt was dran geändert, hoffe es is jetzt besser, ansonsten tu ich mich immer mit den namen unheimlich schwer.

while(this_list!=Tree.find(buffer_map->first)->second.end()&&found==false){
  if(*this_list==*buffer_list){
    Tree.find(buffer_map->first)->second.erase(this_list);
    found=true;
  }
  ++this_list;
}

wie wäre es mit

while(this_list!=Tree.find(buffer_map->first)->second.end())
{
  if(*this_list==*buffer_list)
  {
    Tree.find(buffer_map->first)->second.erase(this_list);
    break;
  }
  ++this_list;
}

Angesehen, gekauft,benutzt, und zufrieden

und irgendwie testest du find() nie auf auf end()...

brauch ich nicht, ich teste es einmal am anfang,ob die Klasse an die andere Klasse weitergegeben wurde, und wenn das true ist, dann gehts weiter, sonst passiert gar nichts.

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