Wartbarkeit std::any_of



  • Hi Leute,

    ich bin gerade dabei einige Schleifen zu entfernen und mache mir Gedanken was die Wartbarkeit diverser Funktionen im algorithm-Header angeht.

    Vorher:

    // Haengen alle Depots an die neue Position an
    for ( const auto &depots: pc.DepotStatesByDepotName )
    {
        if ( !newposition_iter->second->appendDepot( depots.first ) )
        {
            // Das Depot konnte nicht hinzugefuegt werden, daher brechen wir ab und entfernen die Position
            PositionsByKey.erase( newposition_iter ); // entfernen die bereits erzeugte Position
            return PositionsByKey.end();
        }
    }
    

    Nachher:

    if ( std::any_of( pc.DepotStatesByDepotName.begin(), pc.DepotStatesByDepotName.end(),
                        [&newposition_iter] ( const std::pair<std::string, std::pair<DepotType,bool>> &depot )
                        {
                            // Sobald das anhaengen eines Depots schief geht, bricht std::any_of ab und liefert true zurueck
                            return !newposition_iter->second->appendDepot( depot.first );
                        } ) )
    {
        // entfernen die bereits erzeugte Position wieder, weil ein Depot nicht hinzugefuegt werden konnte
        PositionsByKey.erase( newposition_iter );
        return PositionsByKey.end();
    }
    

    Von der Wartbarkeit finde ich eigentlich die erste Variante irgendwie intuitiver.
    Mein CppCheck kritisiert die Schleife aber als "bitte durch std::any_of ersetzen". Nun schwanke ich zwischen beiden Varianten wobei mir in dem Fall die Wartbarkeit wichtiger wäre.

    Sexyer finde ich die zweite...

    Was denkt ihr?



  • Ich würde auf jeden Fall Lambdas mit sprechendem Namen verwenden:

    auto try_append = [&newposition_iter] ( const auto& depot )
    {
        // Sobald das anhaengen eines Depots schief geht, bricht std::any_of ab und liefert true zurueck
        return !newposition_iter->second->appendDepot( depot.first );
    } );
    
    if ( std::any_of( pc.DepotStatesByDepotName.begin(), pc.DepotStatesByDepotName.end(),
         try_append )
    {
        // entfernen die bereits erzeugte Position wieder, weil ein Depot nicht hinzugefuegt werden konnte
        PositionsByKey.erase( newposition_iter );
        return PositionsByKey.end();
    }
    


  • Ich seh erstmal keinen Grund, das grundsätzlich umzubauen, nur weil CppCheck das meint. Ich schreib auch viel öfter Schleifen, als entsprechende Algorithmen zu verwenden. Ich kenn das Video "no raw loops", aber ich würds nicht grundsätzlich unterschreiben. Meist ist eine Schleife schneller zu schreiben und zu lesen und hat auch sonst keine Nachteile.



  • Gefühlt (von mehr kann man nicht reden) finde ich Variante 1 besser.

    @manni66 Den Lambda als extra Funktion zu bauen finde ich auch nicht unbedingt übersichtlicher. Da gefällt mir die Notation von @It0101 besser.
    Der Grund ist einfach: Der Code für die "Entscheidungsfindung" ist kompakter am Code, "was passieren soll"...
    Es passt in meine Denkstrukturen besser hinein. Ist aber genauso Geschmacksache.

    Aber es ist wohl eher eine Frage der Gewohnheit.
    Just my 2 cents...



  • Ich glaube, dass das Problem ist, dass eine Aktion durchgeführt wird, die man bei any_of nicht auf den ersten Blick erwartet. Du willst ja gar nicht feststellen, ob irgendwo ein Wert vorhanden ist, sondern eigentlich scheint mir die Hauptaufgabe das Einfügen zu sein, das unter einem bestimmten Fall abgebrochen werden muss. Any_of erweckt außerdem den Eindruck, dass die Reihenfolge egal ist. Du willst eher den ersten Fall finden, wo etwas nicht mehr geht. Wenn, dann ist es eher ein find_if_not. Wobei auch das das Finden in den Fokus rückt.



  • @wob
    Bestätigt irgendwie @Martin-Richter. Bei der ersten Variante erkennt man sofort was Phase ist... Bei der zweiten muss man kurz überlegen, welche der vielen wahnsinnigen Container-Schleifen-Ersatze dort gerade im Einsatz ist.



  • @wob sagte in Wartbarkeit std::any_of:

    dass eine Aktion durchgeführt wird, die man bei any_of nicht auf den ersten Blick erwartet

    Das stimmt. Aber vielleicht stimmt das auch nur, weil man any_of (oder hier vielleicht auch all_of) so selten benutzt und lieber for-Schleifen schreibt.



  • Hm.

    Was mich an diesem Beispiel eigentlich mehr stört, ist das newposition_iter->second->appendDepot. Was ist newposition_iter->second? Ein Wert aus einer Map? Kann man das Ding benennen in eine Variable, z.B. auto &depotCollection = newposition_iter->second;?

    Und wieso wird am Ende das ganze Ding wieder entfernt? Kann man nicht erst versuchen, alle Depots zu appenden und dann am Ende bei Erfolg in PositionsByKey einfügen? Dann wäre auch ein any_of kein Problem: wenn any insert fails, dann Abbruch! (nicht: dann irgendwas erasen, in das man gerade eingefügt hat)



  • @It0101 sagte in Wartbarkeit std::any_of:

    Von der Wartbarkeit finde ich eigentlich die erste Variante irgendwie intuitiver.

    Der Codebloat ist keineswegs objektiv besser sondern deutlich schlechter. Es handelte sich ohnehin um eine Schleife, die automatisch über den Container iteriert. Weshalb dann mehr Code für einen eigentlichen trivialen Sachverhalt einführen? Weil irgend welche Sprachprüfmittel den Unsinn propagiert?



  • @Mechanics sagte in Wartbarkeit std::any_of:

    Ich kenn das Video "no raw loops", aber ich würds nicht grundsätzlich unterschreiben. Meist ist eine Schleife schneller zu schreiben und zu lesen und hat auch sonst keine Nachteile.

    Ich finde den ausdrucksvollen Namen eines Algorithms scheller und einfacher zu lesen als eine Schleife. Wo es eventuell sinnvoll ist: Klaus Iglberger - "Almost no raw loops" (C++ Beginner's Lightning Talk).



  • Habs mir jetzt auch angeschaut... Auch ein Argument, ja.
    Ansonsten ist es denke ich Geschmackssache/Gewohnheitssache. Die meisten Algorithmen sagen ja im Endeffekt auch nichts und man muss sich genauso reindenken, nur ist es noch schwerer zu lesen (zumindest, wenn man das nicht WIRKLICH gewohnt ist). any_of sagt für mich halt auch nichts, und dann muss man sich optisch erstmal durch die ganzen Klammern wühlen, bis man versteht, was da passiert. Versteh ich bei den Schleife schneller, und ohne das any_of geht für mich persönlich vom Versändnis nichts verloren.



  • @Mechanics sagte in Wartbarkeit std::any_of:

    Habs mir jetzt auch angeschaut... Auch ein Argument, ja.

    Du hast aber wahrscheinlich Paulo Portelas Talk "C++ Seasoning" gemeint?



  • @wob sagte in Wartbarkeit std::any_of:

    Und wieso wird am Ende das ganze Ding wieder entfernt? Kann man nicht erst versuchen, alle Depots zu appenden und dann am Ende bei Erfolg in PositionsByKey einfügen? Dann wäre auch ein any_of kein Problem: wenn any insert fails, dann Abbruch! (nicht: dann irgendwas erasen, in das man gerade eingefügt hat)

    Den Punkt habe ich inzwischen schon geändert. Der war wirklich etwas verdreht 😉 Da waren wohl einige Neuronen kurzzeitig falsch verbunden.

    Aber die grundlegende Frage bleibt quasi offen, bzw. es gibt verschiedene Ansichten, die eigentlich alle plausibel sind.

    Ich bleib erstmal bei der ersten Variante, wegen der intuitiven Lesbarkeit.



  • @Swordfish sagte in Wartbarkeit std::any_of:

    Ich finde den ausdrucksvollen Namen eines Algorithms scheller und einfacher zu lesen als eine Schleife.

    Das klingt zuerst ganz toll. Aber Algorithmen sollten einen bestimmten Grad an Abstraktion bieten, und bei trivialen Schleifen ist das nicht mehr der Fall. Zudem wird man irgend wann doch eine Schleife im Code haben müssen, nämlich dann wenn man den Algorithmus implementiert.

    Dazu kommt ein weiterer Aspekt. Die ganzen tollen neuen C++ Sprachfeatures sind nur ein Versprechen auf eine tolle Zukunft, aber sie bieten deutlich weniger als die alten etablierten Sprachfeatures, weil man sich bei der Einführung dieser nicht darauf geachtet hat, dass sie mit alten Erweiterungen kompatibel sind.

    Jetzt vier mal das gleiche und die Frage ist, was ist in der Praxis die beste Lösung?

    #include <mkl_cblas.h>
      
    #include <iostream>
    #include <numeric>
    #include <vector>
    
    int main() {
        const size_t s = 100000;
        std::vector<double> v1(s), v2(s);
        double r = 0.0;
    
        // (1)
        for (size_t i = 0; i < s; ++i) {
            r += v1[i] * v2[i];
        }
    
        // (2)
        r = inner_product(v1.begin(), v1.end(), v2.begin(), 0.0);
    
        // (3)
        #pragma omp parallel for shared(v1, v2) reduction (+:r)
        for (size_t i = 0; i < s; ++i) {
            r += v1[i] * v2[i];
        }
    
        // (4)
        r = cblas_ddot(s, v1.data(), 0, v2.data(), 0);
    }
    

    Die Schleife 3 ist eine Variation der ersten Schleife, und die Lösung vier ist die klassische Algorithmenlösung, die man so seit einer halben Ewigkeit im HPC nutzt. Was kann 2 besser als 4?


Anmelden zum Antworten