if Abfrage vereinfachung?

MegaV0lt

Hallo, ich habe folgende if-Abfrage:

    tColor ColorFg, ColorBg;
    tColor ColorExtraTextFg = Theme.Color(clrMenuItemExtraTextFont);
    if (Current) {
        ColorFg = Theme.Color(clrItemCurrentFont);
        ColorBg = Theme.Color(clrItemCurrentBg);
        ColorExtraTextFg = Theme.Color(clrMenuItemExtraTextCurrentFont);
    } else {
        if (Selectable) {
            ColorFg = Theme.Color(clrItemSelableFont);
            ColorBg = Theme.Color(clrItemSelableBg);
        } else {
            ColorFg = Theme.Color(clrItemFont);
            ColorBg = Theme.Color(clrItemBg);
        }
    }

Kann man das so umschreiben?:

    tColor ColorFg = Theme.Color(clrItemFont);
    tColor ColorBg = Theme.Color(clrItemBg);
    tColor ColorExtraTextFg = Theme.Color(clrMenuItemExtraTextFont);
    if (Current) {
        ColorFg = Theme.Color(clrItemCurrentFont);
        ColorBg = Theme.Color(clrItemCurrentBg);
        ColorExtraTextFg = Theme.Color(clrMenuItemExtraTextCurrentFont);
    } else if (Selectable) {
        ColorFg = Theme.Color(clrItemSelableFont);
        ColorBg = Theme.Color(clrItemSelableBg);
    }

Wäre kürzer, Die beiden tColor Variablen würden beim erstellen schon initialisiert. Und vielleicht auch schneller?
Ich frage mich nur, ob da das gleiche raus kommt, bzw. warum das oben so gemacht wurde?

*john 0

if (current) {
} else {
    if (selectable) {
    } else {
        // other
   }
}

Ist nicht gleichbedeutend mit dem was Du geschrieben hast. Denn es gibt in Original drei Fälle Current, Selectable und nennen wir es mal Other.

if (Current) {
} else if (Selectable) {
} else { // Other 
}

Sollte das abdecken.

MegaV0lt

Kann man das Other nicht weglassen. Die Variablen sind doch vorbelegt.

    tColor ColorFg = Theme.Color(clrItemFont);
    tColor ColorBg = Theme.Color(clrItemBg);

DocShoe

Ich würde das entweder in ein eigene Funktionen oder lokale Lambdas auslagern:

void func()
{
   auto fg_color = [&]
   {
      if( Current ) return Theme.Color(clrItemCurrentFont);
      else if( Selectable ) return Theme.Color(clrItemSelableFont);
      return Theme.Color(clrItemFont);
   };
   auto bk_color = [&]
   {
      if( Current ) return Theme.Color(clrItemCurrentBk);
      else if( Selectable ) return Theme.Color(clrItemSelableBg);
      return Theme.Color(clrItemBk);
   };
   auto extra_color = [&]
   {
      if( Current ) return Theme.Color(clrMenuItemExtraTextCurrentFont);
      else return Theme.Color(clrMenuItemExtraTextFont);
   };
   tColor const fg = fg_color();
   tColor const bk = bk_color();
   tColor const ex = extra_color();
}

Edit:
Antwort auf die Frage: Ja, den other Teil kannst du weglassen, wenn du deinen Variablen einen Default-Wert für den Normalfall zuweist.

hustbaer

@MegaV0lt sagte in if Abfrage vereinfachung?:

Und vielleicht auch schneller?

Das wird vermutlich alles auf das selbe rauskommen, weil es der Compiler schön so optimieren kann wie es effizient ist. Würde mich nicht wundern wenn da im Endeffekt ein Haufen cmov rauskommen.

Wenn mal allerdings davon ausgehen würde dass der Compiler es ohne Optimierungen kompiliert, dann wäre die Variante mit Vorinitialisieren vermutlich langsamer. Weil erstmal einen Wert reinschreiben um es dann mit einem anderen Wert zu überschreiben halt länger dauert als nur 1x den finalen Wert reinzuschreiben.

hustbaer

@DocShoe Also ich weiss nicht, ich finde das nicht wirklich übersichtlicher/besser. Und die Schreibweise mit if (condition) expression; alles in einer Zeile finde ich auch grässlich.

DocShoe

Mann könnte das durch ein Hilfsstruktur oder std::tuple<unsigned int, unsigned int, unsignet int> auf ein Lambda verkleinern.

#include <tuple>

void f()
{
  auto colors = [&]()
  {
      if( Current ) return std::make_tuple( clrItemCurrentFont, clrItemCurrentBk, clrMenuItemExtraTextCurrentFont );
      else if( Selectable ) return std::make_tuple( clrItemSelableFont, clrItemSelableBg, clrMenuItemExtraTextFont );
      return std::make_tuple( clrItemFont, clrItemBk, clrMenuItemExtraTextFont );
   };
   auto [fg,bk,extra] = colors();
   tColor const fg_color = Theme.Color( fg );
   tColor const bk_color = Theme.Color( bk );
   tColor const ex_color = Theme.Color( extra );
}

Th69

Ich bin ein Verfechter der Trennung von Code und Daten und würde daher die ganzen Farbwerte in ein Array einer Struktur auslagern, so daß der Code in etwas so aussieht:

struct Colors { tColor ColorFg, ColorBg, ColorExtraTextFg; };

Colors colors =
{
  { Theme.Color(clrItemCurrentFont), Theme.Color(clrItemCurrentBg), Theme.Color(clrMenuItemExtraTextCurrentFont) }, // Current
  { ... }, // Selectable
  { ... } // Other
};

const Colors& GetColors(bool current, bool selectable, Colors[] colors)
{
  std::size_t index = current ? 0 : selectable ? 1 : 2; // kann auch in eine eigene Funktion ausgelagert werden
  return colors[index];
}

(zusätzlich könnte man auch ein enum für die Indizes anbieten)

Evtl. könnte man auch nur die reinen Farbwerte clrItemCurrentFont, clrItemCurrentBg, ... in das Array packen und dann eine Umrechnungsfunktion nach tColor (=> Theme.Color(...)) anbieten.

Edit: @DocShoe, da hatten wir ja eine ähnliche Idee...

*john 0

@DocShoe sagte in if Abfrage vereinfachung?:

  if( Current ) return std::make_tuple( clrItemCurrentFont, clrItemCurrentBk, clrMenuItemExtraTextCurrentFont );
  else if( Selectable ) return std::make_tuple( clrItemSelableFont, clrItemSelableBg, clrMenuItemExtraTextFont );
  return std::make_tuple( clrItemFont, clrItemBk, clrMenuItemExtraTextFont );

Genau wegen so einen Mist hasse ich Lambdas. Es ist optisch keinerlei Struktur erkennbar und man muss sich mühsam durch Fließtext quälen, um die Strukturen erkennen zu können.

DocShoe

@john-0

Ja dann bau' dir halt 'ne Struktur und 'ne eigene Funktion, in der du Current und Selectable als Parameter übergeben kannst. Hab' ich oben auch schon geschrieben... Das halte ich dann allerdings für doof, weil das alles nur lokal gebraucht wird.

Finnegan

@john-0 sagte in if Abfrage vereinfachung?:

@DocShoe sagte in if Abfrage vereinfachung?:
  if( Current ) return std::make_tuple( clrItemCurrentFont, clrItemCurrentBk, clrMenuItemExtraTextCurrentFont );
  else if( Selectable ) return std::make_tuple( clrItemSelableFont, clrItemSelableBg, clrMenuItemExtraTextFont );
  return std::make_tuple( clrItemFont, clrItemBk, clrMenuItemExtraTextFont );
Genau wegen so einen Mist hasse ich Lambdas. Es ist optisch keinerlei Struktur erkennbar und man muss sich mühsam durch Fließtext quälen, um die Strukturen erkennen zu können.

Der zitierte Code hat aber für sich nichts mit Lambdas zu tun, ausser dass er in einem solchen vorkommt. Letztendlich ist deine Kritik auch nur eine Frage der Code-Formatiertung. Ich sehe keinen Grund, weshalb man Lambdas nicht wie jeden anderen Codeblock formatieren sollte. Ich hätte das z.B. so geschrieben:

auto colors = [&]()
{
    if (Current) 
        return std::make_tuple(clrItemCurrentFont, clrItemCurrentBk, clrMenuItemExtraTextCurrentFont);
    else if (Selectable)
        return std::make_tuple(clrItemSelableFont, clrItemSelableBg, clrMenuItemExtraTextFont);
    return std::make_tuple(clrItemFont, clrItemBk, clrMenuItemExtraTextFont);
};

Oder besser noch ohne das else, da ich ohnehin ein Fan von "early return" bin (ich empfinde else sogar fast schon als schlechten Stil wenn der eine Branch mit einem return endet):

auto colors = [&]()
{
    if (Current) 
        return std::make_tuple(clrItemCurrentFont, clrItemCurrentBk, clrMenuItemExtraTextCurrentFont);
    if (Selectable)
        return std::make_tuple(clrItemSelableFont, clrItemSelableBg, clrMenuItemExtraTextFont);
    return std::make_tuple(clrItemFont, clrItemBk, clrMenuItemExtraTextFont);
};

Ist die Struktur so besser?

MegaV0lt

Vielen Dank für die Vorschläge. Mir ist das zu kompliziert und ich lasse meinen Vorschlag so im code.

Das other trifft am wahrscheinlichsten zu gefolgt von selectabe und current