wie kann man das optimieren?

otze

IBuffer* operator-=(IBuffer* buffer){
            buffer->UnUse();
                if(buffer!=this&&buffer!=User){
                    if(User!=NULL){
                        *User-=buffer;
                    }
                    std::map<Effect::EffectDescription,std::list<IBuffer*> >::const_iterator buffer_map;
                    for(buffer_map=buffer->getTree()->begin();buffer_map!=buffer->getTree()->end();++buffer_map){
                        if(Tree.find(buffer_map->first)!=Tree.end()){
                            if(Tree.find(buffer_map->first)->second.size()==buffer_map->second.size()){
                                Tree.erase(buffer_map->first);
                            }
                            else
                            {
                                std::list<IBuffer*>::iterator this_list=Tree.find(buffer_map->first)->second.begin();
                                std::list<IBuffer*>::const_iterator buffer_list=buffer_map->second.begin();
                                bool found;
                                for(;buffer_list!=buffer_map->second.end();++buffer_list){
                                    found=false;
                                    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;
                                    }
                                }
                            }
                        }
                    }
                }
            }

ok, dieser operator soll folgendes machen:
ich habe zwei klassen buffer, die jeweils eine

std::map<Effect::EffectDescription,std::list<IBuffer*> >

enthalten.
ziel ist es, einen vorher in die struktur des buffers eingefügten anderen buffer zu entfernen.
eingefügt wurde der buffer, indem der inhalt der listen seiner map, geordnet in die listen der anderen map kopiert wurde, sodass gleiches bei gleichem ist.
aufjedenfall sieht mir die umkehrung des operators sehr klobig aus, und ist auch nicht sonderlich schnell.
ich hab schon versucht daran was zu optimieren, aber meine versuche waren nicht allzusehr von erfolg gekrönt,vorallem weil ich vom optimieren nicht so die Ahnung hab...

Das problem des operators ist, dass ich ihn noch rekursiv aufrufen muss,falls sowas passiert:

Buffer1+=Buffer2;
Buffer3+=Buffer1;
indem flal wäre dann nämlich in Buffer 3 der inhalt von buffer 2 und 1, dh da muss man den inhalt auch noch entfernenund genau das machen die zeilen:

if(User!=NULL){
    *User-=buffer;
}

weis jemand, wie man das ganze noch irgendwie optimieren kann?

Jesus Christus! Da blickt ja kein Mensch mehr durch.

otze

was meinste, wieso ich das ding net optimiert bekomm-.-

elise

das programm erinnert mich seltsam an lisp

Schreib alles nochmal neu.

Mit deinem Coding-Style wirst du früher oder später scheitern.

otze

normalerweise is der style ganz gut, nur bei der tiefen verschachtelung...

interpreter

otze schrieb:

IBuffer* operator-=(IBuffer* buffer){
            buffer->UnUse();
                if(buffer!=this&&buffer!=User){
                    if(User!=NULL){
                        *User-=buffer;
                    }
                    std::map<Effect::EffectDescription,std::list<IBuffer*> >::const_iterator buffer_map;
                    for(buffer_map=buffer->getTree()->begin();buffer_map!=buffer->getTree()->end();++buffer_map){
                        if(Tree.find(buffer_map->first)!=Tree.end()){
                            if(Tree.find(buffer_map->first)->second.size()==buffer_map->second.size()){
                                Tree.erase(buffer_map->first);
                            }
                            else
                            {
                                std::list<IBuffer*>::iterator this_list=Tree.find(buffer_map->first)->second.begin();
                                std::list<IBuffer*>::const_iterator buffer_list=buffer_map->second.begin();
                                bool found;
                                for(;buffer_list!=buffer_map->second.end();++buffer_list){
                                    found=false;
                                    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;
                                    }
                                }
                            }
                        }
                    }
                }
            }
ok, dieser operator soll folgendes machen:
ich habe zwei klassen buffer, die jeweils eine
std::map<Effect::EffectDescription,std::list<IBuffer*> >
enthalten.
ziel ist es, einen vorher in die struktur des buffers eingefügten anderen buffer zu entfernen.
eingefügt wurde der buffer, indem der inhalt der listen seiner map, geordnet in die listen der anderen map kopiert wurde, sodass gleiches bei gleichem ist.
aufjedenfall sieht mir die umkehrung des operators sehr klobig aus, und ist auch nicht sonderlich schnell.
ich hab schon versucht daran was zu optimieren, aber meine versuche waren nicht allzusehr von erfolg gekrönt,vorallem weil ich vom optimieren nicht so die Ahnung hab...

Das problem des operators ist, dass ich ihn noch rekursiv aufrufen muss,falls sowas passiert:

Buffer1+=Buffer2;
Buffer3+=Buffer1;
indem flal wäre dann nämlich in Buffer 3 der inhalt von buffer 2 und 1, dh da muss man den inhalt auch noch entfernenund genau das machen die zeilen:
if(User!=NULL){
    *User-=buffer;
}
weis jemand, wie man das ganze noch irgendwie optimieren kann?

Gibs zu, du wolltest doch nur etwas posen...

otze

nö-.- der code ist echt ein problem.
aber ich hätte mir eh denken können, dass sachen die man nichmehr anständig algorithmisieren kann nicht gut werden können

Jester

Hast Du schonmal drüber nachgedacht für diese

std::map<blablub<foo>, bla>-Geschichte ein typedef einzuführen? Das dürfte der Lesbarkeit einiges zurückgeben.
Und dann legst Du mal ne Referenz an, um dieses ständige buffer->getTree->blabla abzuschaffen.

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