[if - else if - else] oder [switch - case - default] ?

XSpille

@Lichtlein: Blödes Beispiel

Dein in der Regel nicht spürbarer Unterschied ist nur an den Datentyp geknüpft.
Dann kannst du auch gleich ein Beispiel produzieren, das ein spürbar
unterschiedliches Verhalten hat:

#include <iostream>

class Foo{
public:
        Foo() : i(2){}

        operator int(){
                return i--;
        }
private:
        int i;
};

void if_test(Foo& foo){
        if(foo == 0){
                std::cout << "0" << std::endl;
        } else if(foo == 1){
                std::cout << "1" << std::endl;
        } else {
                std::cout << "2" << std::endl;
        }
}

void switch_test(Foo& foo){
        switch(foo){
        case 0:
                std::cout << "0" << std::endl;
                break;
        case 1:
                std::cout << "1" << std::endl;
                break;
        default:
                std::cout << "2" << std::endl;

        }
}

int main(){
        Foo foo;
        if_test(foo);
//      switch_test(foo);       
}

if_test:

1

switch_test:

2

Gruß,
XSpille

EDIT: Ich weiß, dass dieses Beispiel auch sinnlos ist

Hier mal ein Beispiel aus der Echtzeitprogrammierung.

Gegeben ist ein Hardware Status Register. Aus dem Register bekommt man eine
Zahl zurück die folgend Kodiert ist.

0 = Kein Ereignis
1 = Empfangs Buffer Belegt ( hat hoehere prio )
2 = Sende Buffer Frei
3 = Fehler
x = Hardware Error

Ändert sich am Status was so wird in Programm eine Funktion aufgerufen die macht folgendes. (Ich bilde jetzt nur mal die if/else um das Fehlverhalten zu zeigen. Mit switch könnt Ihr es euch selber ausdenken :-))

volatile struct sDevice
{
  uint32_t   status;
  uint32_t   command;
};

/* 
Funktion wird aufgerufen weil der sende buffer frei wird.
status hat den inhalt 2.
*/

bool funktion( sDevice *pDev)
{
   if(pDev->status == 0)
     return false;
   else 
   if(pDev->status == 1)
     return writeBuffer(pDev);
   else

/*
hier wird das Programm vom Betriebssystem unterbrochen.
bevor man wieder rechenzeit bekommt. In der Zeit meldet das
Device eine neue Message im Buffer.
im status steht jetzt 1.

Und was passiert jetzt im Program, na ?
*/

   if(pDev->status == 2)
     return readBuffer(pDev);
   else
   if(pDev->status == 3)
     return resetDevice(pDev);
   else
   {
     panic();
     stopSystem();
   }
}

Hoffe das ist ersichtlich das hier ein Fehler passiert.

Lichtlein

Ist das nicht einfach bloss schlecht kodiert? Der Status sollte kopiert werden, nicht ein Pointer übergeben... wäre ja nicht mal Thread safe.

XSpille

Es ist logisch, dass zusätzliche Operationen das Ergebnis des Programms
verändern können. Ob es jetzt ein Funktionsaufruf ist, der etwas zurückliefert
oder eine Konvertierungsfunktion.

Dort findet ein zusätzliches Laden eines Wertes statt.
Da kannst du genauso einen Funktionsaufruf reinpacken, ist für mich
inhaltlich das Gleiche.

@schlecht

Ist bei Device Treiber gängige Paxis und Thread safe braucht es auch nicht sein. Der Treiber an sich ist immer Single Thread (In der ISR) . Kann aber durch das Betriebssystem unterbrochen werden.

@XSpille

Darum ging es mir ja, das sich was ändern kann, das eben ein if/else nicht gleichzusetzen ist mit einen switch.

Noch mal zu den Beispiel.
Es ist ja nicht so das ich mir das aus den Finger gesogen habe. Häufig entsteht solcher Code über lange Zeit weil der Chip sich weiter Entwickelt und neue Funktionen bekommt. Und dann lass jemanden am Treiber Programmieren der aus der Anwendungs-Entwicklung kommt. Da kommen Sachen raus.

Also, benutzt switch/case wann immer möglich. Ist leichter zu warten.

Lichtlein

volkard

Lichtlein schrieb:

Also, benutzt switch/case wann immer möglich. Ist leichter zu warten.

April, April!

hustbaer

@Lichtlein:
Vorweg: ja, du hast Recht, dein Beispiel "funktioniert" (in dem Sinn dass sich da mit if-else-if ein Problem ergibt welches sich mit switch() nicht ergibt. I stand corrected.

----

Ich hab schon einigen Treiber-Code gesehen, aber noch keinen derart schrottigen.

Es ist ja nicht so das ich mir das aus den Finger gesogen habe. Häufig entsteht solcher Code über lange Zeit weil der Chip sich weiter Entwickelt und neue Funktionen bekommt. Und dann lass jemanden am Treiber Programmieren der aus der Anwendungs-Entwicklung kommt. Da kommen Sachen raus.

Man kann die Zugriffe auch in ReadXxxRegister() und WriteYyyRegister() Funktionen packen. Dann versteht auch Mr. Anwendungsprogrammiererwastler was da abgeht. Mal ganz davon abgesehen dass Mr. Anwendungsprogrammiererwastler in der Treiberentwicklung nix verloren hat. Zumindest nicht ohne Einschulung.

Was als Aussage für mich übrig bleibt: schlechte Programmierer schreiben schlechte Programme. Das lässt sich aber mMn. nicht dadurch fixen/verbessern dass man ihnen verbietet if-else-if zu verwenden (bzw. empfiehlt switch stattdessen zu verwenden). Auch wenn manche anderen Programmierer glauben dass es ne super Taktik ist um Probleme zu vermeiden.

abc.w

Lichtlein schrieb:

Also, benutzt switch/case wann immer möglich. Ist leichter zu warten.

Habe heute in einem Treiber diesen Code gesehen und an diesen Thread hier gedacht:

switch (value)
{
    default:
        ...
        blabla(value);
        ...
        break;
}

... ja, ein switch ohne case ist sogar noch einfacher zu warten

Ah, ein Treiberentwickler der Zukünftige Entwicklungen berücksichtigt !! Sehr Löblich.

Lichtlein

abc.w

Genau, toller Code!
Es gibt da noch mehr Sachen, z.B. switch mit nichts:

switch (value)
{
    default:
        ;
}

oder ein switch-case ohne default-Zweig:

switch (value)
{
    case A:
        ...
        break;
    case B:
        ...
        break;
}

oder ein switch-case mit einem mekrwürdigen break:

switch (value)
{
    case A:
        ...
        ret = blabla();
        ...
        if (ret)
            break;
    case B:
    case C:
        break;
}

in case B und C steht kein Code, einfach break. Toll...

Nexus

abc.w schrieb:

oder ein switch-case ohne default-Zweig:

Wenn bereits vorhin alle möglichen Fälle (in denen etwas passiert) abgedeckt sind, ist default unnötig. Du schreibst ja auch nicht zu jedem if ein else .

abc.w schrieb:

in case B und C steht kein Code, einfach break. Toll...

Wo liegt das Problem? So macht man wenigstens explizit, dass es noch die Fälle B und C gibt und dass in diesen Fällen nichts getan wird. Noch ausdrücklicher ginge es nur noch mit einem Kommentar.

abc.w

Nexus schrieb:

abc.w schrieb:

oder ein switch-case ohne default-Zweig:

Wenn bereits vorhin alle möglichen Fälle (in denen etwas passiert) abgedeckt sind, ist default unnötig. Du schreibst ja auch nicht zu jedem if ein else .

Alle möglichen Fälle abdecken - so was gibt es nicht. Ich schreibe gerne immer ein default , auch wenn es "nicht notwendig" ist, und mache noch z.B. so was:

switch (value)
{
    case A:
        ...
        break;
    case B:
        ...
        break;
    default:
        atomic_inc(&unexpected_case_cnt);
        break;
}

... und manchmal schreibe ich auch ein else zu jedem if , manchmal mit einem Kommentar drin, z.B.:

if (something)
{
    a = blabla();
}
else
{
    /* Do not touch a in this case here, because bla bla bla bla */
    a = a;
}

Nexus schrieb:

abc.w schrieb:

in case B und C steht kein Code, einfach break. Toll...

Wo liegt das Problem? So macht man wenigstens explizit, dass es noch die Fälle B und C gibt und dass in diesen Fällen nichts getan wird. Noch ausdrücklicher ginge es nur noch mit einem Kommentar.

Das Problem ist, dass für ret == 0 wird auch B und C ausgeführt. In diesem Fall steht da einfach ein break und es passiert nichts, bis jemand dort seinen Code einfügt...

Michael E.

abc.w schrieb:

Nexus schrieb:

abc.w schrieb:

oder ein switch-case ohne default-Zweig:

Wenn bereits vorhin alle möglichen Fälle (in denen etwas passiert) abgedeckt sind, ist default unnötig. Du schreibst ja auch nicht zu jedem if ein else .

Alle möglichen Fälle abdecken - so was gibt es nicht.

Warum nicht? Vielleicht habe ich vorher bereits sichergestellt, dass die Variable einen von mir erwarteten Wert hat. Vielleicht will ich nur eine zusätzliche Operation für ein paar Spezialfälle machen und für alles andere soll gar nichts getan werden.

... und manchmal schreibe ich auch ein else zu jedem if , manchmal mit einem Kommentar drin, z.B.:
if (something)
{
    a = blabla();
}
else
{
    /* Do not touch a in this case here, because bla bla bla bla */
    a = a;
}

Finde ich nicht schön, vor allem die Selbstzuweisung. Ich würd das "else" wahrscheinlich mit in den Kommentar tun.

minastaros

Mal unabhängig von dem, was der Compiler macht:

Wenn es wirklich nur 2 Werte sind und sonst default-Fälle, und es einigermaßen vorhersehbar ist, dass die 2 Fälle nicht später mal um einige mehr erweitert werden, würde ich zu einem if .. else if .. else tendieren.

Bei default im switch -Block gehe ich nach der Faustregel, dass die behandelten Fälle (meistens, wie schon gesagt, enums) durch die case abgedeckt werden und das default den Fehlerfall (also wider Erwarten und Programmierkunst ein out-of-range) behandelt.

Meistens lässt sich das so umsetzen, und so hat man projektweit den Wiedererkennungseffekt default -> Fehlerfallbehandlung.
Nur in wenigen Fällen kam es bisher vor, dass ich das default eben wirklich für "alle anderen Fälle" genutzt habe, weil ein if/else wegen der vielen Einzelfälle zu umständlich gewesen wäre.

Für das Problem mit dem versehentlichen

if( a = 1 )

hab ich mir

if( 1 == a )

angewöhnt, dann meckert der Compiler, wenn man = schreibt...

volkard

minastaros schrieb:

Für das Problem mit dem versehentlichen
if( a = 1 )
hab ich mir
if( 1 == a )
angewöhnt, dann meckert der Compiler, wenn man = schreibt...

... anstatt die Compilerwarnungen anzuschalten, was der viel bessere Weg gewesen wäre.

minastaros

volkard schrieb:

... anstatt die Compilerwarnungen anzuschalten, was der viel bessere Weg gewesen wäre.

Die sind an Ist mir nur nicht aufgefallen, dass er da warnt, weil das bei mir ja schon ein Error ist. Gut zu wissen.

Nexus

minastaros schrieb:

Für das Problem mit dem versehentlichen
if( a = 1 )
hab ich mir
if( 1 == a )
angewöhnt, dann meckert der Compiler, wenn man = schreibt...

Hihi, das hört man immer wieder
Was ich davon halte:

Es ist weniger übersichtlich und logisch, wenn die zu prüfende Variable am Schluss steht. Gerade wenn der Ausdruck komplizierter ist als ein Literal.
Man kann das nicht konsistent durchziehen. Was machst du bei a == b ?
Man wiegt sich in falscher Sicherheit. Zum Beispiel kann if (Function() = b) ohne Probleme kompilieren, wenn ein nicht-skalarer RValue zurückgegeben wird.
Dieser typische Anfängerfehler kommt so selten vor, dass es sich nicht lohnt, den Code hässlich zu machen.
Wie von volkard erwähnt helfen einem gewisse Compiler sogar mit einer Warnung.

volkard

minastaros schrieb:

volkard schrieb:

... anstatt die Compilerwarnungen anzuschalten, was der viel bessere Weg gewesen wäre.

Die sind an Ist mir nur nicht aufgefallen, dass er da warnt, weil das bei mir ja schon ein Error ist. Gut zu wissen.

Da gibt es wohl nur drei Möglichkeiten:
a) Warnungen nicht ignorieren (empfohlen)
b) auch -Werror einschalten

hustbaer

minastaros schrieb:

hab ich mir
if( 1 == a )
angewöhnt, dann meckert der Compiler, wenn man = schreibt...

Wenn ich sowas lese verfalle ich in Blutrausch.
Yoda-Programmieren das ist, Reihenfolge in dieser normaler Mensch denkt keiner.
Also warum verdammt sollte man es dann in der Reihenfolge hinschreiben?
*rage*

minastaros

Besten Dank. Auch wenn wir bereits OT diskutieren, noch kurz die Antwort:

@Nexus: Ich seh es eher als Flüchtigkeitsfehler, kann aus Unachtsamkeit immer mal wieder passieren (wenn auch extrem selten).
Alle Fälle erschlägt man mit dem "Trick" nicht, man minimiert zumindest die Fälle mit konstanten Werten.

a == b

: Genau da würde die Compilerwarnung ja auch jetzt schon greifen. Ist mir halt noch nicht vorgekommen.

@Volkard: Ich sehe Warnungen immer als Fehler an: Keine Freigabe mit Warnungen. Aber zum Experimentieren kann man sie manchmal tolerieren, d.h. kein Werror.
Die Sache ist einfach so: Das hab ich mir das mal irgendwann angewöhnt, und seit dem ist es so drin. Als eine Lösung für die Bedenken von klöklö weiter vorn im Thread würde es zumindest reichen.

Insgesamt und spätestens durch Argument "Hässlichkeit" sehe ich die Sache mal wieder als überdenkenswert an.

@hustbaer:

Blutrausch ... kein normaler Mensch

Hey, locker bleiben, das steht in durchaus guten Büchern als präventive Maßnahme. Es funktionert und ist besser als gar nichts, aber wie gesagt, ich sehe es absolut ein, dass man durch Warnungen (und dann die "richtige" Reihenfolge der Operanden) wesentlich eleganter zum Ziel kommt, und das heißt: sicherer Code.
*tröst*