Vereinfachung möglich und sinnvoll?

MegaV0lt

Ich habe in einem Project mehrfach Code, der einen String nach Zeichen durchsucht und ihn splittet, um ihn in zwei verschiedenen Farben anzuzeigen. Das zu suchende Zeichen (~) wird zwei mal gesucht (Mit und ohne Leerzeichen).

Das lässt sich doch bestimmt vereinfachen, in dem man nur nach ~ sucht und Leerzeichen falls vorhanden löscht.

    if (ColorExtraTextFg) {  // Extra Farbe ist definiert
        std::string tilde = Text;
        size_t found = tilde.find(" ~ ");
        size_t found2 = tilde.find('~');
        if (found != std::string::npos) {  // ' ~ ' gefunden
            std::string first = tilde.substr(0, found);
            std::string second = tilde.substr(found + 2, tilde.length());

            Pixmap->DrawText(cPoint(0, 0), first.c_str(), ColorFg, ColorBg, Font);
            int l = Font->Width(first.c_str());
            Pixmap->DrawText(cPoint(l, 0), second.c_str(), ColorExtraTextFg, ColorBg, Font);
        } else if (found2 != std::string::npos) {  // '~' gefunden
            std::string first = tilde.substr(0, found2);
            std::string second = tilde.substr(found2 + 1, tilde.length());

            Pixmap->DrawText(cPoint(0, 0), first.c_str(), ColorFg, ColorBg, Font);
            int l = Font->Width(first.c_str());
            l += Font->Width('X');
            Pixmap->DrawText(cPoint(l, 0), second.c_str(), ColorExtraTextFg, ColorBg, Font);
        } else  // Keine ~ gefunden. Text komplett anzeigen
            Pixmap->DrawText(cPoint(0, 0), Text.c_str(), ColorFg, ColorBg, Font);
    } else {  // Keine Extra Farbe definiert
        Pixmap->DrawText(cPoint(0, 0), Text.c_str(), ColorFg, ColorBg, Font);
    }

Man müsste bei found2 prüfen, ob beim String first ein Leerzeichen am ende ist und bei second am Anfang und diese jeweils entfernen. Gibt es da eine einfache Lösung?
In der tools.h habe ich das gefunden:

inline char *skipspace(const char *s)
{
  if ((uchar)*s > ' ') // most strings don't have any leading space, so handle this case as fast as possible
     return (char *)s;
  while (*s && (uchar)*s <= ' ') // avoiding isspace() here, because it is much slower
        s++;
  return (char *)s;
}

Kann ich das verwenden. Verstehe nicht ganz was da gemacht wird

SeppJ

Kannst du mal in Worten erklären, warum das überhaupt zwei verschiedene Fälle sind? Ich bin mir nicht sicher, ob ich mir das korrekt erschließe. Geht es darum, dass du an der Tilde die Farbe wechseln möchtest, aber bei dem Farbwechsel auch sicher gestellt werden soll, dass zwischen den sichtbaren Buchstaben ein gewisser Abstand ist?

Falls ja, würde ich von so einer Funktionalität eher abraten, sofern deine Motivation ist, dies aus Nutzerfreundlichkeit zu tun. Es ist nicht deine Aufgabe, dem Nutzer vorzuschreiben, wie er seinen Text formatieren soll. Das kann nur schief gehen. Eine Funktion muss vom Nutzer erwarten können, dass er weiß, was er tut. Wenn der Nutzer Trennzeichen haben möchte, soll er halt welche in die Funktion reingeben. Umgekehrt fände ich es als Nutzer sehr irritierend, wenn eine Funktion meine Eingaben neu interpretiert. Wenn ich "Farb~wechsel" schreibe, möchte ich auch "Farbwechsel" als Ausgabe, nicht "Farb wechsel", sonst hätte ich doch "Farb ~ wechsel" geschrieben.

Dann würde sich als schöner Zusatzeffekt auch deine Funktion wesentlich vereinfachen. Dann gibt es nix zu suchen und keine Verzweigungen, sondern einfach: Ausgabe in einer Farbe bis zum '~' (das halt kommen kann oder nicht), dann weiter in einer anderen Farbe. Fertig. Kurze, einfache Funktionen, mit klar definierten Aufgaben führen zu korrektem Code! Unerwartete, unnötige Komplexität zum Gegenteil.

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

if ((uchar)*s > ' ') // most strings don't have any leading space, so handle this case as fast as possible
return (char *)s;

early exit : Der String enthält keine führenden Leerzeichen, die weitere Verarbeitung kann sofort beendet werden

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

while (*s && (uchar)*s <= ' ') // avoiding isspace() here, because it is much slower
s++;

Iteration : Der char pointer wird so lange weitergesetzt, bis das Ende oder das erste Zeichen erreicht wurde, das kein Leerzeichen darstellt - dabei wird die Funktion isspace() vermieden und stattdessen händisch überprüft

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

return (char *)s;

regular exit : Der übriggebliebene String wird zurückgegeben (Pointer darauf)

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

inline char *skipspace(const char *s)

Anzumerken ist hier vielleicht noch ... dass die Funktion mit der Signatur (const char const *s) nicht funktionieren würde, weil dann der char-Pointer s auch nicht modifiziert werden dürfte ...

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

Das lässt sich doch bestimmt vereinfachen,

Wie es @SeppJ bereits gesagt hatte ... Deine Funktion tut insgesamt zu viel ... In einem ersten Schritt könntest Du diese in kleinere aufteilen, um dann zu sehen, was möglicherweise "wegkann".

Ich habe Deine Funktion nicht im Detail nachvollzogen.

MegaV0lt

@SeppJ
Zwei Fälle, da ein Plugin es anders darstellt. Möglich sind:

"Eine Serie~Folge 1"
"Eine Serie ~ Folge 1"

Beide Strings sollen in
"Eine Serie" und "Folge 1" gesplittet werden. Im OSD wird der zweite String in einer anderen Farbe angezeigt.

Die Farrbe ist einstellbar und auch abschaltbar (if (ColorExtraTextFg))

firefly

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

@SeppJ
Zwei Fälle, da ein Plugin es anders darstellt. Möglich sind:

"Eine Serie~Folge 1"
"Eine Serie ~ Folge 1"

Beide Strings sollen in
"Eine Serie" und "Folge 1" gesplittet werden. Im OSD wird der zweite String in einer anderen Farbe angezeigt.

Die Farrbe ist einstellbar und auch abschaltbar (if (ColorExtraTextFg))

Wäre das nicht aus lösbar mit

Splitte string mit trennzeichen '~'
Trimme die strings von leerzeichen zu beginn/ende
Dann wäre das für beide fälle verwendbar ohne separaten code zu benötigen

meuterei

Oder mit regex, das ginge auch, um beide zu berücksichtigen...

MegaV0lt

Ich habe das im Netz gefunden

// trim from left
inline std::string &ltrim(std::string &s, const char* t = " \t\n\r\f\v") {
    s.erase(0, s.find_first_not_of(t));
    return s;
}

// trim from right
inline std::string& rtrim(std::string& s, const char* t = " \t\n\r\f\v") {
    s.erase(s.find_last_not_of(t) + 1);
    return s;
}

Und der Code sieht jetzt so aus:

    if (ColorExtraTextFg) {
        std::string tilde = Text;
        size_t found = tilde.find('~');  // Search for ~
        if (found != std::string::npos) {
            std::string first = tilde.substr(0, found);
            std::string second = tilde.substr(found + 1, tilde.length());
            rtrim(first);   // Trim possible space on right side
            ltrim(second);  // Trim possible space at begin

            Pixmap->DrawText(cPoint(0, 0), first.c_str(), ColorFg, ColorBg, Font);
            int l = Font->Width(first.c_str());
            l += Font->Width('X');
            Pixmap->DrawText(cPoint(l, 0), second.c_str(), ColorExtraTextFg, ColorBg, Font);
        } else  // ~ not found
            Pixmap->DrawText(cPoint(0, 0), Text.c_str(), ColorFg, ColorBg, Font);
    } else {  // No extracolor defined
        Pixmap->DrawText(cPoint(0, 0), Text.c_str(), ColorFg, ColorBg, Font);
    }

Schon viel kleiner...
Nur von cpplint kommt ne Warnung:

Is this a non-const reference? If so, make const or use a pointer: std::string &s cpplint(warning:runtime/references)

Das kann ich nicht lösen

SeppJ

Schlechte Warnung, wo dir jemand seinen fragwürdigen, restriktiven Stil aufzwingen will. Ja, es ist mit Absicht eine non-const Referenz. Das kann schlechter Stil sein, wenn man es versteckt an unerwarteten Orten, aber hier ist das intuitiv klar und erwartet, dass die Funktion so benutzt werden sollte.

MegaV0lt

Warum wird eigentlich noch ein

return s;

benötigt? Und stimmt der Aufruf mit

ltrim(second);

Wird da nur die Referenz übergeben...

SeppJ

Nötig ist es nicht, aber es ermöglicht auch die Nutzung in verketteten Aufrufen, a la

cout << rtrim(my_string);

Wobei wir da tatsächlich so langsam in Gebiete kommen, wo die non-const Referenz zu gefährlichen Irrtümern führen kann. Beispielsweise wäre

result = ltrim(my_string)+ rtrim(my_string)

nicht so günstig.

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

Nur von cpplint kommt ne Warnung:
Is this a non-const reference? If so, make const or use a pointer: std::string &s cpplint(warning:runtime/references)
Das kann ich nicht lösen

Vermutlich könnte der zweite Parameter auch so deklariert werden:

const char * const t

Aber das ist, wie es SeppJ erklärt hat, gewissermaßen unnötig erzwungen, würde ich sagen.

*john 0

@SeppJ sagte in Vereinfachung möglich und sinnvoll?:

Nötig ist es nicht, aber es ermöglicht auch die Nutzung in verketteten Aufrufen, a la
cout << rtrim(my_string);
Wobei wir da tatsächlich so langsam in Gebiete kommen, wo die non-const Referenz zu gefährlichen Irrtümern führen kann. Beispielsweise wäre
result = ltrim(my_string)+ rtrim(my_string)
nicht so günstig.

Persönlich finde ich das besser entweder

std::string trim (const std::string& input);

oder

void trim (std::string& input_output);

zu wählen.

Ich vermisse mittlerweile in C++ ein Fortran Feature – die intent Deklaration von Parametern in Funktionen und Subroutinen. Funktionen können übrigens nur „in“ Parameter haben, Subroutinen „in“, „out“ und „in out“.

SeppJ

Ja, ich kann durchaus die Meldung des Linters da verstehen. Komplett neue Programmierer können sich da durchaus auf die Schnauze legen, und ein bösartiger Programmierer könnte absichtlich auch erfahrenen Programmierern ein Bein stellen. Aber ich finde es stark übertrieben. Die Hürde ist jetzt echt nicht hoch, das zu lernen. Und wenn du bösartige Feinde hast, werden die dich auf andere Weise kriegen. Ein bisschen Niveau muss man voraussetzen dürfen, sonst kommt man zu nix. Dieser Linter würde die gleiche Diagnose für einen großen Teil der Standardbibliothek liefern müssen.

DocShoe

Die boost-Stringbibliotheken bennenen ihre trim() Funktion explizit, da gibt es Sachen wie trim( std::string& s ) und trim_copy( std::string const& s ), in allen Kombinationen (left, right mit und ohne copy im Namen). Sieht manchmal etwas sperrig aus, aber dafür sieht man bei

std::string const cleaned = trim_copy( input );

oder

std::string const cleaned2 = trim_left_copy( input );

aber auch sofort, was da passiert.

Finnegan

Dieses ganze trim, find und substr sieht mir so aus, als ginge es im Prinzip nur darum, den "richtigen" Substring in einem vorhandenen String zu finden. Da kann man eventuell auch überlegen, mit std::string_view zu arbeiten. So ließen sich einige Kopier- und Speicherallokations-Operationen einsparen... aber gut möglich, dass das bei diesem Code nur wenig ins Gewicht fällt und daher nicht so wichtig ist.

Das sieht mir aber durchaus wie eine Rendering-Funktion aus, da lassen sich solche Optimierungen meistens schon rechtfertigen.

meuterei

@DocShoe sagte in Vereinfachung möglich und sinnvoll?:

mit und ohne copy im Namen

Das gefällt mir. +1

Ich bin mittel-fortgeschrittener Anfänger, und für mich ist es eine Erleichterung, die Funktionsweise nicht über konstante bzw. nicht konstante Funktionsparameterdeklarationen "erahnen" zu müssen ... sondern sie direkt am Namen der Funktion ablesen zu können.

MegaV0lt

Ich habe das jetz so abgeändert, da ich das trim nur an den besagten Stellen brauche:

// Trim from left
inline void ltrim(std::string &s, const char* t = " \t\n\r\f\v") {
    s.erase(0, s.find_first_not_of(t));
}

// Trim from right
inline void rtrim(std::string& s, const char* t = " \t\n\r\f\v") {
    s.erase(s.find_last_not_of(t) + 1);
}

Es wäre auch ein Name im Stil von

inplace_rtrim()

denkbat

meuterei

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

find_first_not_of

Heißt das nicht not_in anstatt not_of? In SQL heißt's so... Oder not_one_of?

@MegaV0lt sagte in Vereinfachung möglich und sinnvoll?:

Es wäre auch ein Name im Stil von
inplace_rtrim()