std::multimap

unskilled

ich habs mal so geschrieben, weil ich finde, es ist so lesbarer...
Fehler sind mir keine aufgefallen - abgesehen davon, dass ich finde, dass die Funktion zu lang ist und die Variablennamen grausig sind - da sollte man sich evtl bessere Namen einfallen lassen...

void Renderer::destroyRenderTarget(const std::string& name)
{
	typedef RenderTargetPriorityMap::iterator Titerator;
	typedef std::pair <Titerator, Titerator> Titerator_pair;

	Titerator it = renderTargets.find(name); 
	if(it == renderTargets.end())
		return;

	Titerator_pair range = renderTargetsPriority.equal_range(it->second->getPriority());

	for (Titerator iter = range.first; iter != range.second; )
	{
		if(iter->second == it->second) 
			iter = renderTargetsPriority.erase(iter);    
		else 
			++iter; 
	} 

	delete it->second;
	renderTargets.erase(it);
}

bb

unskilled schrieb:

abgesehen davon, dass ich finde, dass die Funktion zu lang ist und die Variablennamen grausig sind

lol? DIESE Funktion ist zu lang?? wtf? Manchmal glaub ich echt, manche Leute leben in einer "Hello World" idealisierten Traumwelt...
Was an Namen wie renderTargets oder renderTargetsPriority "grausig" sein soll, weißt wohl nur du.
Ansonsten danke;)

unskilled

ich rede von Namen wie mmRangeIt...

Nur weil du http://www.cplusplus.com/reference/algorithm/remove_if/ nicht kennst, ist deine Fkt um 6 Zeilen länger, als sie das sein müsste...
Und zu dem noch sehr viel schwerer zu verstehen...

Und ja, in meiner Hallo-Welt-Traumwelt sind Fkt nicht so lang und Variablennamen haben keine sinnlosen Prefixe... Wenn du so etwas allerdings nicht hören möchtest, hättest du auch nicht fragen müssen

so in etwa würd es bei mir aussehen:

struct comparer
{
private:
	const RenderTarget* lhs;
public:
	comparer(const RenderTarget *_lhs) : lhs(_lhs) {}

	bool operator() (const RenderTarget *rhs) const
	{	return *lhs == *rhs; }
};

void Renderer::_destroyRenderTarget(const RenderTargetPriorityMap::iterator it)
{
    typedef RenderTargetPriorityMap::iterator Titerator;
    typedef std::pair <Titerator, Titerator> Titerator_pair;

    Titerator_pair range = renderTargetsPriority.equal_range(it->second->getPriority());

    remove_if( range.first, range.second, comparer(it->second) );

    delete it->second;
    renderTargets.erase(it);
}

void Renderer::destroyRenderTarget(const std::string& name)
{
    RenderTargetPriorityMap::iterator it = renderTargets.find(name);
    if(it != renderTargets.end())
        _destroyRenderTarget(it);
}

Wenn man diese beiden typedefs dann noch sinnvollerweise in den Header auslagert und ihnen bessere Namen verpasst, braucht kein Mensch mehr auch nur annähernd so viel Zeit, die Fkt zu verstehen, wie das bisher der Fall war...

bb

@Code: LOL. Na der Code ist natürlich jetzt kürzer und einfacher zu verstehen. Und der Methodenname mit dem Underscore ist natürlich richtig schön. Und einen comparer für einen einzigen Aufruf definieren ist auch sehr elegant. Und der Name Titerator_pair ist ebenfalls viel besser. *facepalm

Jetzt weiß ich immerhin, wieso du so heißt.

rolleis schrieb:

@Code: LOL. Na der Code ist natürlich jetzt kürzer und einfacher zu verstehen. Und der Methodenname mit dem Underscore ist natürlich richtig schön. Und einen comparer für einen einzigen Aufruf definieren ist auch sehr elegant. Und der Name Titerator_pair ist ebenfalls viel besser. *facepalm

Jetzt weiß ich immerhin, wieso du so heißt.

unskilled  adj.	 	unerfahren
unskilled  adj.	 	ungelernt
unskilled  adj.	 	ungeschickt
unskilled  adj.	 	nicht ausgebildet

unskilled

rolleis schrieb:

@Code: LOL. Na der Code ist natürlich jetzt kürzer

darum gehts doch gar nicht - es geht darum, wie gut man die fkt. xyz verstehen kann...

rolleis schrieb:

und einfacher zu verstehen.

auf jeden fall besser zu überblicken und es sollte eigtl für jeden einfacher zu lesen sein ( remove_if vs. 6 Zeilen for -Schleife...) - man muss noch nicht mal wissen, wie remove_if arbeitet sondern erkennt sofort am Namen, was es macht - aber naja... Du kannst ja mal deinen Quelltext aus dem ersten Post und meinen gerade eben vergleichen und dann sagen, welchen du besser findest, wenn du jetzt gucken möchtest, was die fkt xyz genau macht...

rolleis schrieb:

Und der Methodenname mit dem Underscore ist natürlich richtig schön.

Kannst es auch in eine Fkt hauen, mag ich allerdings nicht so - und meine privaten Fkt haben eigtl alle nen Underscore am Anfang, damit Intellisense nicht ständig private Fkt vorschlägt - ist halt Geschmackssache... Allerdings bin ich der Meinung, dass jede Fkt eine Aufgabe haben sollte: Eine überprüft, ob das Element existiert und die andere sorgt dann für das löschen der Elemente...

rolleis schrieb:

Und einen comparer für einen einzigen Aufruf definieren ist auch sehr elegant.

Japp, ist es... Oder siehst du irgendwelche Nachteile?

rolleis schrieb:

Und der Name Titerator_pair ist ebenfalls viel besser. *facepalm

myself schrieb:

Wenn man diese beiden typedefs dann noch sinnvollerweise in den Header auslagert und ihnen bessere Namen verpasst

rolleis schrieb:

Jetzt weiß ich immerhin, wieso du so heißt.

myself schrieb:

Wenn du so etwas allerdings nicht hören möchtest, hättest du auch nicht fragen müssen

und wenns dir nicht passt, dann ignorier es halt und mach es weiterhin so, wie du es jetzt machst...
naja - viel erfolg noch beim Hilfe suchen, wenn du denkst, es so und so besser zu wissen...

edit: da oben is übrigens ein unnötiges const dabei...

unskilled schrieb:

auf jeden fall besser zu überblicken

Ich finde genau das Gegenteil ist der Fall. Für so eine trivale Sache extra eine 2. Funktion anlegen und eine Klasse ist... "Overkill" und ein klarer Fall von Overdesigned. Ich finde deinen Code viel umständlicher. Um zu verstehen was da passiert muss ich die eine Funktion anschauen, dann die andere Funktion, dann die Comparer Klasse anschauen und dann auch noch raffen wie remove_if funktioniert. Sonst rafft man nämlich NICHT, was die Anweisung macht.

unskilled schrieb:

Kannst es auch in eine Fkt hauen, mag ich allerdings nicht so - und meine privaten Fkt haben eigtl alle nen Underscore am Anfang, damit Intellisense nicht ständig private Fkt vorschlägt - ist halt Geschmackssache...

In so ziemlich jeden Buch zu C++ das ich gelesen habe stand, dass man beginnende Underscores vermeiden sollte.

unskilled schrieb:

Japp, ist es... Oder siehst du irgendwelche Nachteile?

Japp. Overkill. Extra ne Klasse definieren um sie an einer einzigen Stelle zu verwenden ist einfach Quatsch.

unskilled schrieb:

Wenn man diese beiden typedefs dann noch sinnvollerweise in den Header auslagert und ihnen bessere Namen verpasst

Einen typdef für einen Typen, den ich genau an EINER Stelle in einer Funktion verwende... schon wieder Overkill. Ich glaub du bist so ein Sprachliebhaber, der sich stundenlang an irgendwelchen Konstrukten aufgeilt, aber noch nie eine wirklich größere Software programmiert hat.

unskilled schrieb:

naja - viel erfolg noch beim Hilfe suchen, wenn du denkst, es so und so besser zu wissen...

Der Punkt ist ganz einfach: Es kotzt mich in diesem Forum generell manchmal an, dass jeder immer gleich daher kommt und meint das Design des OP bemängeln zu müssen. Ich habe klipp und klar nur gefragt ob die Logik stimmt und wollte eben KEINE Designvorschläge. Aber du musst natürlich gleich wieder daherkommt und rumnörgeln. Und eine Funktion von 20 Zeilen als zu lang bezeichnen ist wirklich lächerlich. Wie gesagt, ungefragt immer gleich den Stil des anderen kritisieren wirkt einfach klugscheisserisch und damit

unskilled

wenn für dich alles nen overkill ist, was du nur einmal brauchst, dann ists ja gut...
mir ist es ziemlich egal, obs am ende 20 LOC mehr sind oder nicht, wenn ich dafür ne Fkt einfach überblicken kann...
Aber mit deiner unendlichen STL-Erfahrung wirst du das schon besser wissen als ich, wann man fertige Funktionen nimmt und wann nicht - wenn dich die Klasse stört, könntest du auch ne Fkt nehmen - je nach Compiler auch ne lamba-Fkt, aber das wär wahrscheinlich auch wieder overkill - weil du overkill ja offenbar mit "kenn ich nicht" gleichzusetzen scheinst...

nur dazu noch was:
"In so ziemlich jeden Buch zu C++ das ich gelesen habe stand, dass man beginnende Underscores vermeiden sollte."

Ja, den sollte man vermeiden, wenn man vor hat, nen Großbuchstaben danach zu schreiben oder noch nen Underscore - ansonsten ist es garantiert, dass dort keine Makros drauf liegen sondern maximal fkt- oder variablennamen - die aber einfach überdeckt werden und es somit nicht stört... Ist auch im Standard nachzulesen, falls du mir wieder nicht glaubst... Aber das wäre auch overkill, für so ne Frage iwo nachzuschlagen...

naja, hf noch - wird mir hier zu dumm - zu mal ich auch noch unmissverständlich kenntlich gemacht habe, was meine meinung ist und was nicht...

otze

unskilled schrieb:

rolleis schrieb:

@Code: LOL. Na der Code ist natürlich jetzt kürzer

darum gehts doch gar nicht - es geht darum, wie gut man die fkt. xyz verstehen kann...

das Problem ist, dass dein "comparer" nichts aussagt. Ich weis immer noch nicht, wann Elemente gelöscht werden sollen, ohne dass ich comparer kenne. Der Name ist einfach Schrott.
Viel wichtiger ist allerdings, das remove_if bei maps überhaupt nicht funktioniert, weil der key der pairs konstant ist.

Oder wie es der gcc sagt:

-------------- Build: Debug in test ---------------

Compiling: main.cpp
In file included from /usr/include/c++/4.4/bits/stl_algobase.h:66,
                 from /usr/include/c++/4.4/bits/char_traits.h:41,
                 from /usr/include/c++/4.4/ios:41,
                 from /usr/include/c++/4.4/ostream:40,
                 from /usr/include/c++/4.4/iostream:40,
                 from /home/otze/Projekte/test/main.cpp:1:
/usr/include/c++/4.4/bits/stl_pair.h: In member function ‘std::pair<const int, int>& std::pair<const int, int>::operator=(const std::pair<const int, int>&)’:
/usr/include/c++/4.4/bits/stl_pair.h:68:   instantiated from ‘_FIter std::remove_if(_FIter, _FIter, _Predicate) [with _FIter = std::_Rb_tree_iterator<std::pair<const int, int> >, _Predicate = bool (*)(std::pair<const int, int>)]’
/home/otze/Projekte/test/main.cpp:25:   instantiated from here
/usr/include/c++/4.4/bits/stl_pair.h:68: error: non-static const member ‘const int std::pair<const int, int>::first’, can't use default assignment operator
In file included from /usr/include/c++/4.4/algorithm:62,
                 from /home/otze/Projekte/test/main.cpp:4:
/usr/include/c++/4.4/bits/stl_algo.h: In function ‘_FIter std::remove_if(_FIter, _FIter, _Predicate) [with _FIter = std::_Rb_tree_iterator<std::pair<const int, int> >, _Predicate = bool (*)(std::pair<const int, int>)]’:
/usr/include/c++/4.4/bits/stl_algo.h:1161: note: synthesized method ‘std::pair<const int, int>& std::pair<const int, int>::operator=(const std::pair<const int, int>&)’ first required here 
Process terminated with status 1 (0 minutes, 0 seconds)
1 errors, 0 warnings

Auch ist die Aufteilung der Funktion nicht sinnvoll, solange wir den Rest des Interfaces nicht kennen. Wenn das Ding überhaupt kein Iteratorinterface hat, ist die Aufteilung höchstens verwirrend, zumal sie keine Vereinfachung darstellt sondern nur Code rum schiebt.

camper

Falls man überhaupt über Vereinfachung nachdenkt, sollten auch bekannte Nebenbedingungen mit einbezogen werden. Wir setzen ja voraus, dass jedes RT jeweils genau einmal in jeder map auftaucht, dann kann diese Eigenschaft auch beim Suchen genutzt werden - sind diese Voraussetzungen verletzt, liegt sowieso ein Fehler vor.

void Renderer::destroyRenderTarget(const std::string& name) {

   RenderTargetMap::iterator it = renderTargets.find(name);

   if(it == renderTargets.end())
       return;

   RenderTarget* rt = it->second;

   //--- Erase entry in multimap<int, RenderTarget*> ---
   // Get all entries which have the priority of the RT to be deleted (there can be multiple
   // RTs with the same priority), but each RT is unique
   RenderTargetPriorityMap::iterator mmIt = renderTargetsPriority.lower_bound(it->second->getPriority());
   for ( ; mmIt->second != rt; ++mmIt )
       ;
   renderTargetsPriority.erase(mmIt);

   renderTargets.erase( it );    // Erase entry in map<string, RenderTarget*>

   delete rt;
}

Solange Zeiger darauf existieren, sollten Speicherbereiche nicht gelöscht werden - derartige deletes gehört immer ans Ende der Funktion. In diesem konkreten Fall ist das zwar wahrscheinlich unproblematisch, im Allgemeinen ist das aber nicht immer leicht zu überschauen.

LordJaxom

otze schrieb:

Viel wichtiger ist allerdings, das remove_if bei maps überhaupt nicht funktioniert, weil der key der pairs konstant ist.

Darüberhinaus ist remove_if falsch angewandt worden (soviel zum Thema "man muss nicht wissen, was die Funktion tut" :D)

drakon

Unskilled schrieb:

Und das ganze auch noch direkt hintereinander... Ab jetzt werde ich meine Membervariablen auch immer so verunstalten *scnr*
Naja - mal im Ernst: Ich finds hässlich und alles andere als nützlich - aber jedem das Seine...

*SCNR*

Und jetzt plötzlich:

Unskilled schrieb:

Kannst es auch in eine Fkt hauen, mag ich allerdings nicht so - und meine privaten Fkt haben eigtl alle nen Underscore am Anfang, damit Intellisense nicht ständig private Fkt vorschlägt

Und ich dachte du meinst das ironisch, dass du das ab dort auch so machst.. :p
(ich weiss, dass es da um m_ ging, aber in etwa läuft es auf das selbe hinaus.. ;))

Quelle:
http://www.c-plusplus.net/forum/viewtopic-var-p-is-1619162.html

Hehe, welch Ironie. Da kritisiert er meinen Code, schreibt groß um, verkompliziert den Code total und dann ist der Code auch noch falsch. Wirklich ne riesen Verbesserung. :p
Glaub das mach ich jetzt auch, für jede Aktion mindestens 2 Funktionen und mindestens eine Hilfsklasse. Vor allem hilfreich, wenn man nach LOC bezahlt wird.
</sarkasmus>

Naja, nix für ungut. Ich bleib jetzt bei meiner EINEN Funktion.

@camper: Deine Lösung finde ich klasse. Schön eine (!) kurze Funktion, noch kürzer als mein Code. Danke