Strings von Objekten vergleichen und ändern

Lennox

@Erhard-Henkes sagte in Strings von Objekten vergleichen und ändern:

for (int idx : participants)
{
    auto* c = game.world.creatures[idx];
    const std::string base = c->GetName();     // stabiler "Rohname"
    ++total[base];
}

@Finnegan Ich erwarte hier, also in der letzten Anweisung der for-each-Schleife, keine esoterischen oder kryptischen Konstrukte.

Finnegan

@Lennox sagte in Strings von Objekten vergleichen und ändern:

@Erhard-Henkes sagte in Strings von Objekten vergleichen und ändern:
for (int idx : participants)
{
    auto* c = game.world.creatures[idx];
    const std::string base = c->GetName();     // stabiler "Rohname"
    ++total[base];
}
@Finnegan Ich erwarte hier, also in der letzten Anweisung der for-each-Schleife, keine esoterischen oder kryptischen Konstrukte.

Das ist Ansichtssache. Ich finde das absolut normalen Code und keineswegs "esoterisch". Da haben ich schon ganz andere Konstrukte gesehen, bei denen ich dann auch zustimmen würde

Was mir hier eher etwas aufstößt sind die String-Kopien, die man z.B. mit String Views oder meinentwegen einer Referenz vermeiden könnte. Besonders wenn die Lebensdauer dieser Variablen ohnehin nur diese kompakten Schleifen sind (Ja, ich kenne die Gefahr von Dangling References)

Lennox

@Finnegan siehe hier: https://stackoverflow.com/questions/4445706/post-increment-and-pre-increment-concept

Viele Leute glauben fälschlicherweise, dass die Inkrementierung zuerst erfolgen muss, und kommen daher oft zu dem Schluss, dass bestimmte Ausdrücke eine klar definierte Wirkung haben müssen, obwohl sie tatsächlich ein undefiniertes Verhalten aufweisen.

Lennox

@Finnegan sagte in Strings von Objekten vergleichen und ändern:

und keineswegs "esoterisch"

Ich habe es jetzt nur kurz überflogen, aber es sieht so aus, als wäre es "besser" (bzw. übersichtlicher), die total map mit 1 anstatt 0 zu initialisieren, also 1 einzufügen, wenn leer, oder ansonsten zu erhöhen. Dieser spezielle "Inkrement-Schritt" könnte in einer eigenen Funktion stehen, damit es übersichtlich bleibt.

Finnegan

@Lennox sagte in Strings von Objekten vergleichen und ändern:

@Finnegan sagte in Strings von Objekten vergleichen und ändern:

und keineswegs "esoterisch"

Ich habe es jetzt nur kurz überflogen, aber es sieht so aus, als wäre es "besser" (bzw. übersichtlicher), die total map mit 1 anstatt 0 zu initialisieren, also 1 einzufügen, wenn leer, oder ansonsten zu erhöhen. Dieser spezielle "Inkrement-Schritt" könnte in einer eigenen Funktion stehen, damit es übersichtlich bleibt.

Dann muss man aber explizit initialisieren. Ein Grund, weshalb die Implementierug so kompakt ist, ist, dass ein neu eingefügter int-Wert automatisch mit 0 initialisiert wird (unordered_map::operator[] garantiert Value Initialization und diese ist für primitive Typen wie int eine Zero-Initialization). Daher kann man einfach ++total[base] schreiben, auch wenn total[base] noch nicht existiert. Der Wert ist danach garantiert 1.

Man muss auch an die Wartbarkeit des Code denken. Hier eine zusätzliche Funktion zum Initialisieren neuer Map-Elemente einzuführen, nur um i++ statt ++i schreiben zu können, finde ich ein wenig Overkill und alles andere als "übersichtlich". Da würde ich stattdessen lieber so was machen, wenns unbedingt i++ sein soll

int n = seen[base]++;
c->displayName = base + " (" + std::to_string(n + 1) + ")";

Lennox

seen[base]++;
c->displayName = base + " (" + std::to_string(seen[base]) + ")";

fände ich besser.

Lennox

@all: Irgendwie sind die ersten Postings in diesem Thema jetzt weg... Bei euch auch?

e: Sind wieder da.

Finnegan

@Lennox sagte in Strings von Objekten vergleichen und ändern:

seen[base]++;
c->displayName = base + " (" + std::to_string(seen[base]) + ")";

fände ich besser.

Okay. Spielen wir das mal durch. Wenn wir neue Elemente mit 1 initialisieren wollen, müssen wir erst einmal herausfinden, ob wir es überhaupt mit einem neuen Element zu tun haben. Mit std::unordered_map<T>::find ist der Code etwas umständlich, aber ab C++20 gibt es std::unordered_map<T>::contains:

int n = ++seen[base];
c->displayName = base + " (" + std::to_string(n) + ")";

wird dann z.B. zu:

if (!seen.contains(base))
    seen[base] = 1;
int n = seen[base]++;
c->displayName = base + " (" + std::to_string(n + 1) + ")";

Jetzt haben wir allerdings das Problem, dass wir 3 Query-Operationen auf der Map haben. Einmal contains und zweimal operator[]. Irgendwie ist das etwas ineffizient, also ist es vielleicht besser mit C++ insert arbeiten:

auto [iter, was_inserted] = seen.insert({ base, 1});
int n = iter->second++;
c->displayName = base + " (" + std::to_string(n) + ")";

Immerhin: Ebenfalls nur eine einzige Operation (insert). Jetzt gehen auch i++ und std::to_string(n). Aber ist das wirklich "übersichtlicher"? Ich weiß ja nicht, ich empfinde eher solche überflüssigen Zeilen als schlechten Stil. Auch die das iter->second statt Subscript-Operator gefällt mir nicht sonderlich. Die Namen der std::pair-Member sind so nichtssagend. Kann man so machen, aber ich bevorzuge es kurz und kompakt. Und du hast ja sogar noch eine extra Funktion vorgeschlagen. Wie würdest du das denn stattdessen "sauber" implementieren, hoffentlich ohne zu viel Code hinzuzufügen, der die Wartungskosten erhöht?

Lennox

@Finnegan sagte in Strings von Objekten vergleichen und ändern:

Wie würdest du das denn stattdessen "sauber" implementieren

Das habe ich doch schon vorgeschlagen:

@Lennox sagte in Strings von Objekten vergleichen und ändern:

seen[base]++;
c->displayName = base + " (" + std::to_string(seen[base]) + ")";

Der unordered_map-Zugriff hat O(1), der Zugriff auf ein n auch...

@Finnegan sagte in Strings von Objekten vergleichen und ändern:

Ich weiß ja nicht, ich empfinde eher solche überflüssigen Zeilen als schlechten Stil.

Du hast doch diese zusätzlichen Zeilen in deinem Beispiel hinzugefügt/verwendet...

seen[base] noch einmal "zu mappen/kapseln" und in eine zusätzliche Variable n zu stecken, bringt eigentlich keine Vorteile. Du hast eine Variable mehr, die Übersichtlichkeit erhöht sich nicht wesentlich - und da es nur einen lesenden Zugriff gibt, wäre es sogar geboten, hier strikt zu in-linen.

@Erhard-Henkes hätte einfach sagen können: Ok, das habe ich nicht gesehen, danke für den Hinweis. Aber nein... statdessen haut er mir eine allgemeine und allen bekannte Definition um die Ohren, was ich schon ein wenig anmaßend finde. Oder ist er hier der "oberste Guru", dessen Worte niemals kritisiert werden dürfen?

Finnegan

@Lennox sagte in Strings von Objekten vergleichen und ändern:

@Finnegan sagte in Strings von Objekten vergleichen und ändern:

Wie würdest du das denn stattdessen "sauber" implementieren

Das habe ich doch schon vorgeschlagen:

@Lennox sagte in Strings von Objekten vergleichen und ändern:

seen[base]++;
c->displayName = base + " (" + std::to_string(seen[base]) + ")";

Der unordered_map-Zugriff hat O(1), der Zugriff auf ein n auch...

@Finnegan sagte in Strings von Objekten vergleichen und ändern:

Ich weiß ja nicht, ich empfinde eher solche überflüssigen Zeilen als schlechten Stil.

Du hast doch diese zusätzlichen Zeilen in deinem Beispiel hinzugefügt/verwendet...

Ha! Jo, ich habe etwas zu schnell gelesen. Dein Code is schon korrekt (dachte zuerst er sei es nicht), aber zwei Zugriffe auf eine Datenstruktur finde ich persönlich eben nicht "besser". Die ganzen "O von irgendwas" sind nämlich mit Vorsicht zu genießen: Hier muss z.B. zwei Mal der Hash des String berechnet werden. Es wäre daher sowas hier effizienter:

auto& count = seen[base];
count++;
c->displayName = base + " (" + std::to_string(count) + ")";

seen[base] noch einmal "zu mappen/kapseln" und in eine zusätzliche Variable n zu stecken, bringt eigentlich keine Vorteile. Du hast eine Variable mehr, die Übersichtlichkeit erhöht sich nicht wesentlich - und da es nur einen lesenden Zugriff gibt, wäre es sogar geboten, hier strikt zu in-linen.

Das n stammt nicht von mir. Mein Code sah so aus (weiter oben):

std::format_to(std::back_inserter(creature.displayName), " ({})", ++seen[creature.displayName]);

Das ist nicht nur auch ++i, sondern vermutlich sogar noch "kryptischer" für dich
Aber das ist wie ich denke Ansichtssache. Es gibt deutlich wildere Konstrukte, bei denen ich dir da sicherlich auch recht geben würde, aber ich finde sowas hier noch völlig in Ordnung. Das sollte in meinen Augen jeder lesen können, der ernsthaft C++ machen will.

thuering

Oha, liebe Leute, hier hat sich ja was entwickelt!
Auch deine Lösung ist toll, Finnegan. Tatsächlich muss ich mir "Ranges" noch mal genauer anschauen. Performance ist uninteressant, es handelt sich eher um eine persönliche Machbarkeitsstudie als um ein ernsthaftes Projekt. Dennoch möchte ich natürlich lernen, wie man performanten Code schreibt. Momentan ist der Rest der Anwendung aber so extrem ineffizient geschrieben, dass die paar 2D-Pixelgrafiken auf einem 14700K und RTX 5060 Ti mit ~45 FPS laufen
Somit machen ein paar µSekunden durch irgendwelche Strings und Views den Kohl auch nicht mehr fett

@Finnegan sagte in Strings von Objekten vergleichen und ändern:

Nimm daher am besten die Lösung, die dir am besten gefällt und/oder die du am besten nachvollziehen kannst

Genau das mache ich.
Vielen Dank an alle!