ZFX + Developia

EyDu hat geschrieben:Hallo.

Du hast einfach nur ein `const` vergessen:

Code: Alles auswählen

VALUE& operator[] (const KEY& key)
{
return (*_items)[key];
}

EyDu hat geschrieben: Etwas Kritik an deinem Code habe ich aber noch: Warum benutzt du für `_items` einen Pointer? Der ist hier eigentlich eine unnötige Indirektion. Aufbauend darauf hast du dir im Destruktor auch gleich ein hübsches Speicherleck gebaut ;-) Du gibst zwar den Inhalt der Map frei, die Map selbst aber nicht.
Sebastian

Krishty hat geschrieben:Richtig.

Außerdem weißt du schon, dass operator [] ein neues Element anlegt, falls der Schlüssel ins Leere führt? Wenn du ihn benutzt hast du also keine Möglichkeit, festzustellen, wann etwas eingefügt wurde (es sei denn, du fängst mit Umwegen an wie, vorher und nachher die Größe abzufragen und beide Werte zu vergleichen). Denk nochmal darüber nach, ob find() in einem nach außen hin const gewrappten operator [] nicht die bessere Wahl wäre.

(Persönlich verabscheue ich die Entscheidung, dass da völlig kontraintuitiv ein Mutator im Inspektorspelz implementiert wurde.)

odenter hat geschrieben:

Krishty hat geschrieben:Richtig.

Außerdem weißt du schon, dass operator [] ein neues Element anlegt, falls der Schlüssel ins Leere führt? Wenn du ihn benutzt hast du also keine Möglichkeit, festzustellen, wann etwas eingefügt wurde (es sei denn, du fängst mit Umwegen an wie, vorher und nachher die Größe abzufragen und beide Werte zu vergleichen). Denk nochmal darüber nach, ob find() in einem nach außen hin const gewrappten operator [] nicht die bessere Wahl wäre.

(Persönlich verabscheue ich die Entscheidung, dass da völlig kontraintuitiv ein Mutator im Inspektorspelz implementiert wurde.)

Das Verhalten ist so gewünscht.

BeRsErKeR hat geschrieben:

odenter hat geschrieben:

Krishty hat geschrieben:Richtig.

Außerdem weißt du schon, dass operator [] ein neues Element anlegt, falls der Schlüssel ins Leere führt? Wenn du ihn benutzt hast du also keine Möglichkeit, festzustellen, wann etwas eingefügt wurde (es sei denn, du fängst mit Umwegen an wie, vorher und nachher die Größe abzufragen und beide Werte zu vergleichen). Denk nochmal darüber nach, ob find() in einem nach außen hin const gewrappten operator [] nicht die bessere Wahl wäre.

(Persönlich verabscheue ich die Entscheidung, dass da völlig kontraintuitiv ein Mutator im Inspektorspelz implementiert wurde.)

Das Verhalten ist so gewünscht.

Dann feuerst du also nur Events, wenn Elemente per Insert eingefügt werden und nicht wenn sie per []-operator eingefügt werden? Der []-operator von std::map ruft ja intern auch ein std::insert auf. Für mich ist das sehr unintuitiv und inkonsequent. Aber du wirst schon wissen was du da tust. ;)

Krishty hat geschrieben:Sobald das Ding kurz vor der Freigabe steht, schick mir bitte eine PM mit dem Namen, damit ich es bloß nie versehentlich benutze. Echt – du könntest mit der Operatorüberladung und const *perfekt* verhindern, dass jemals irgendwer Murks mit der Klasse baut. Und fährst stattdessen die trust me on this one-Schiene. Wegen sowas ist Software heute immernoch so scheiße wie sie es schon immer war.

waigie hat geschrieben:Eigentlich sollte sich das ganze einfacher Lösen lassen wenn du von std::map ableitest und daraufhin nur die insert und delete Methode abänderst, das du dein Event feuerst und dann den parent Code ausführst.

TGGC hat geschrieben:Ich verstehs nicht. Wo ist der Sinn dieses Wrappers? Du solltest dir nur zur Sicherheit noch einen Wrapper fuer deinen Wrapper schreiben...

TGGC hat geschrieben:Wie gesagt, ich verstehe es nicht. Es erscheint mir voellig sinnlos. Du willst sowas machen wir "map[blubb]=bla;DoEvent();" Warum schreibst du dann nicht eine Funktion die das macht? Warum musst du std::map wrappen? std::map ist ja bereits ein Wrapper.

template <typename KEY, typename VALUE>
class MapBase<KEY, VALUE>
{
public:
  boost::signal<void (KEY, VALUE)> ItemInserting;
  boost::signal<void (KEY, VALUE)> ItemInserted;
  boost::signal<void (KEY, VALUE)> ItemAccessing;
  boost::signal<void (KEY, VALUE)> ItemAccessed;
};
MapBase<int, string> m;
m.ItemInserting.connect(&itemInserting_eventHandler);
m.ItemInserted.connect(&itemInserted_eventHandler);
m.ItemAccessing.connect(&itemAccessing_eventHandler);
m.ItemAccessed.connect(&itemAccessed_eventHandler);

m.Insert(1, "karl");
m.Insert(2, "dall");
etc.

// und dann vielleicht irgendwann mal um Daten direkt abzufragen
string name = m[2];
// oder halt einen vorhandenen Wert abändern.
m[2] = "meyer";

BeRsErKeR hat geschrieben:Ich kann da TGGC nur zustimmen. Eine Funktion tut es in dem Fall auch. Man muss nicht immer alles kapseln und wrappen.

odenter hat geschrieben:Man kann soviele machen, man kann aber auch sovieles lassen. Man kann auch eine Engine in C schreiben, muss nicht immer C++ sein, meistens tut es sogar eine fertige. ;)

odenter hat geschrieben:Mir wird es hier langsam zu blöde, besonders die neunmal klugen Kommentare von Leuten die keine Ahnung haben was ich mache, machen will oder warum ich etwas genauso mache wie ich es mache.

CodingCat hat geschrieben:

odenter hat geschrieben:Man kann soviele machen, man kann aber auch sovieles lassen. Man kann auch eine Engine in C schreiben, muss nicht immer C++ sein, meistens tut es sogar eine fertige. ;)

odenter hat geschrieben:Mir wird es hier langsam zu blöde, besonders die neunmal klugen Kommentare von Leuten die keine Ahnung haben was ich mache, machen will oder warum ich etwas genauso mache wie ich es mache.

Ein Wrapper ist dann sinnvoll, wenn du damit unkorrekte Nutzung wesentlich besser ausschließen kannst als ohne. Wenn dein Wrapper schon Löcher hat, dann tut es eine simple Funktion wesentlich besser, weil von vorneherein klar ist, dass NUR bei Benutzung der Funktion korrektes Verhalten gewährleistet ist.
In deinem Fall TUT der Wrapper nur SO, als würde er (in seiner Funktion als Wrapper) korrektes Verhalten GARANTIEREN, du weigerst dich aber, gut gemeinte Vorschläge zur Absicherung des Wrappers ernstzunehmen (z.B. einfach eine vernünfige Implementierung von operator [], siehe Krishty), mit der Begründung, der Wrapper würde eh nur auf bestimmte Weise genutzt. Damit machst du deinen Wrapper nicht nur selbst absolut überflüssig, sondern auch noch gefährlich, und irgendwann WIRD er auch deine Software zerreißen.

odenter hat geschrieben:a.) Exception sofern Key nicht vorhanden ist (bool an Constructor, Vorschlag zur Lösung steht hier im Thread)
b.) Wenn Key/Item nicht da ist anlegen (Gegenteil von a.)
c.) Events die bei Aufruf verschiedener Methoden geworfen werden (die werden immer geworfen muss ja niemand abbonieren)
d.) Änderung der Benamung der Methoden (public Methoden schreibe ich groß nicht klein)

odenter hat geschrieben:Auch Du unterstellst das das anlegen von KEY/VALUE Paaren unkorrektes Verhalten ist, was schlicht nicht zutrifft. Weiter oben hatte ich bereits geschrieben das dieses Verhalten, neben den anderen hier jetzt aufgezählen, explizit erwünscht ist.

odenter hat geschrieben:Ja genau nur wenn insert von mir aufgerufen wird. Da auf absehbare Zeit niemand anderes an dem Code rumfummelt und ich den Operator nur zum abrufen verwenden will, für Werte die vorher defenitiv einefügt wurden, kann da nicht viel schief gehen.

odenter hat geschrieben:Ein Wrapper ist immer dann Sinnvoll wenn ich Funktionalität erweitern/ändern will oder Verhalten verändern will.

odenter hat geschrieben:Statt zu fragen was ich will, wird aufgrund von eigenen Erfahrungen angenommen welches Verhalten ich haben will und dann mit Kommentaren gepaart wie ich etwas doch viel besser machen sollte. ;)