Designfragen zu normalen Funktionen



  • Hallo Leute,

    seit ich mit C++ arbeite, beschäftigen mich immer wieder eine Reihe Fragen zum Programmierdesign und wie man es besser machen könnte. Ich stelle daher gleich mehrere Fragen und hoffe, dass mir der ein oder andere einige Tipps oder Erfahrungswerte geben kann.

    Frage 1: Wo und wie deklariert ihr eure lokalen Variablen?
    Ich habe es mir angewöhnt jede Variable/Struktur die ich verwende am Anfang der Funktion zu deklarieren und mit 0/nullptr zu initialisieren. Dabei deklariere ich auch immer nur eine Variable pro Zeile. Nicht selten sehe ich so etwas:

    void meineFunktionA()
    {
        int a, b, c;    
        float x,y;
        int* d, e, f;   // --> finde ich ganz schlimm, da nur 'd' ein Zeiger ist
    
        ... code ...
    }
    

    (Deklaration am Anfang der Funktion, aber keine Initialisierung.)

    void meineFunktionB()
    {
        ... code ...
        int a = machWas();
        ... code ...
        int b = machWasAnderes();
        ... code ...
        int c = nochWas(a, b);
        ... code ...
    }
    

    (Deklaration und Initialisierung von Variablen an den Stellen, an dem sie das erste mal benötigt werden.)

    Frage 2: Schachtelt ihr Code häufig oder selten? ...und wie tief schachtelt ihr?
    Als ich noch mit C gearbeitet habe, schrieb ich viel Code der geschachtelt war (insbesondere if/else):

    void meineFunktionC()
    {
        ... code ...
        if (a == true)
        {
            ... code ...
            if (b == true)
            {
                ... code ...
                if ((c == true) && (d == true))
                {
                    ... code ...
                }
            }
        } 
        else
        {
            ... code ...
        }
        ... code ...
    }
    

    Bei mehreren Schleifen und größeren Funktionsinhalt musste ich öffters mal schauen, was eine schließende Klammer gerade verlassen hatte. Irgendwie bin ich nun zu sowas übergegangen:

    void meineFunktionD()
    {
        ... code ...
        if (a == false)
        {
            return / throw
        }
        ... code ...
        if (b == false)
        {
            return / throw
        }
        ... code ...
        if ((c == false) || (d == false))
        {
            return / throw
        }    
        ... code ...
    }
    

    Frage 3: Nutzt ihr verkürzte Schreibformen oder bekommen alle eure Schleifen/Bedingungen einen Body?

    // mit Body
    void meineFunktionE()
    {
        if (a < b)
        {
            ... code ...
        }
    
         ... code ...
    
        if (c == nullptr)
        {
            ... code ...
    		... code ...
    		... code ...
        }
    
        ... code ...
    
        for (...;...;...)
        {
            ... code ...
        }
    
         ... code ...
    }
    
    // verkürzte Schreibweise
    void meineFunktionF()
    {
        if (a < b)
            ... code ...
    
        ... code ...
    
        if (c == nullptr)
    		... code ...  // nur diese Zeile wird von 'if' abgedeckt. 
    		... code ...  // diese Zeile wird immer ausgeführt. ...wie groß ist die Wahrscheinlichkeit, dass es vom Programmierer nicht so gewollt ist? 
    		... code ...
    
        ... code ...
    
        for (...;...;...)
            ... code ...
    
        ... code ...
    }
    

    Frage 4: Wie handhabt ihr Fehler und wie häufig prüft ihr auf Fehler?

    Hier ist mal eine Funktion aus meinem aktuellen Code:

    std::string getExtensionType (X509 *x509, int extIndex)
        {
            X509_EXTENSION *ext = nullptr;
            ASN1_OBJECT *object = nullptr;
            const char *extType = nullptr;
            int nid = 0;
    
            if ((x509 == nullptr) || (extIndex <= 0))
            {
                return "";
            }
    
            ext = X509_get_ext(x509, extIndex - 1);
            if (ext == nullptr)
            {
                return "";
            }
    
            object = X509_EXTENSION_get_object(ext);
            if (object == nullptr)
            {
                return "";
            }
    
            nid = OBJ_obj2nid(object);
            if (nid == NID_undef)
            {
                return "";
            }
    
            extType = OBJ_nid2ln(nid);
            if (extType == nullptr)
            {
                return "";
            }
    
            return extType;
        }
    

    Wie man sieht ist der Code relativ lang und besteht fast nur aus Fehlerüberprüfungen. Da diese Funktion OpenSSL nutzt, kommen zur Fehlerbehandlung Fehlercodes zum Einsatz. Da viele Pointer hin und hergeschoben werden, empfinde ich eine Prüfung als wichtig, damit das Programm nicht irgendwann mal nach einer nullptr-Dereferenzierung abschmiert.

    Manchmal finde ich aber auch solche Konstellationen im Internet:

    std::string getExtensionType (X509 *x509, int extIndex)
        {
            if ((!x509) || (extIndex <= 0))
            {
                return "";
            }
    
            // ..naja.... hoffen wir mal das beste -.-
            return OBJ_nid2ln(OBJ_obj2nid(X509_EXTENSION_get_object(X509_get_ext(x509, extIndex - 1))));
        }
    

    Verschachtelte Funktionsaufrufe ohne Überprüfungen (auch wenn diese meist nur 1-2 mal geschachtelt sind).

    Frage 5: Kann ich die Fehlerüberprüfung besser gestalten?
    Also solche Ausdrücke:

    std::string getExtensionType (X509 *x509, int extIndex)
        {
            X509_EXTENSION *ext = nullptr;
    
            ext = X509_get_ext(x509, extIndex - 1);
            if (ext == nullptr)
            {
                return "";
            }
        }
    

    Frage 6: Sollten getter-Methoden exception werfen oder einfach nur einen leeren Rückgabewert (0, nullptr, "") bei einem Fehler zurückgeben?

    Frage 7: Sollte ich an meiner Funktion 'getExtensionType' aus Frage 4 etwas verbessern (oder habe ich grobe Fehler drin)?

    Ich könnte ja z.B. auch sowas machen:

    std::string getExtensionType (X509 *x509, int extIndex)
        {
            X509_EXTENSION *ext = nullptr;
            ASN1_OBJECT *object = nullptr;
            const char *extType = nullptr;
            int nid = 0;
    
            try
            {
                if ((x509 == nullptr) || (extIndex <= 0))
                {
                    throw "...fehlermeldung...";
                }
    
                ext = X509_get_ext(x509, extIndex - 1);
                if (ext == nullptr)
                {
                    throw "...fehlermeldung...";
                }
    
                object = X509_EXTENSION_get_object(ext);
                if (object == nullptr)
                {
                    throw "...fehlermeldung...";
                }
    
                nid = OBJ_obj2nid(object);
                if (nid == NID_undef)
                {
                    throw "...fehlermeldung...";
                }
    
                extType = OBJ_nid2ln(nid);
                if (extType == nullptr)
                {
                    throw "...fehlermeldung...";
                }
            }
            catch (char const *error)
            {
                MeinLogger(error);
                ... ggf. Speicher freigeben ...
                return "";  // ...oder doch lieber rethrow?
            }
            return extType;
        }
    

    viele Grüße,
    SBond



  • Frage 1: Wo und wie deklariert ihr eure lokalen Variablen?

    Da, wo sie zum ersten Mal benutzt werden. Den Scope so klein wie möglich halten.

    Frage 2: Schachtelt ihr Code häufig oder selten? ...und wie tief schachtelt ihr?

    Kommt darauf an. Weniger tief ist besser, je tiefer, desto wahrscheinlicher sollte man irgendwas rausfaktorisieren. Such mal ein bisschen nach Cyclomatic Complexity. Aber ich mag fixe Grenzwerte nicht.

    Frage 3: Nutzt ihr verkürzte Schreibformen oder bekommen alle eure Schleifen/Bedingungen einen Body?

    Alle außer "einfache" returns.
    Ich formatiere aber gerne mit clang-format (benutze ich andauernd) - dann fällt sofort auf, wenn irgendwo plötzlich die Einrückung wieder verschwindet.



  • Zu Frage 1:
    Variante 2, also
    "(Deklaration und Initialisierung von Variablen an den Stellen, an dem sie das erste mal benötigt werden.)"

    Zu Frage 2:
    Variante 2, also ich mache gern "early exit". Speziell bei Sachen wo erst 100 Bedingungen geprüft werden die erfüllt sein müssen bevor man dann endlich was machen kann ... ist das mMn. das einzig vernünftige.

    Zu Frage 3:
    Variante 2, also keine unnötigen {}.
    Ausnahme: Wenn z.B. die Bedingung bei nem if über mehrere Zeilen geht, dann mach ich immer nen Block. Also

    // Die WTF??? Variante:
    if (someCondition
    	&& someOtherCondition
    	&& someThirdCondition)
    	x++;
    
    // Und ich schreib so:
    if (someCondition
    	&& someOtherCondition
    	&& someThirdCondition)
    {
    	x++;
    }
    
    // Aber alles andere so:
    if (dingsi)
    	x++;
    

    Zu Frage 4:

    Wie man sieht ist der Code relativ lang und besteht fast nur aus Fehlerüberprüfungen. Da diese Funktion OpenSSL nutzt, kommen zur Fehlerbehandlung Fehlercodes zum Einsatz. Da viele Pointer hin und hergeschoben werden, empfinde ich eine Prüfung als wichtig, damit das Programm nicht irgendwann mal nach einer nullptr-Dereferenzierung abschmiert.

    Ja, wenn man mit C-Libraries arbeitet hat man oft eigentlich nur zwei Möglichkeiten: drauf scheissen oder ca. so programmieren wie in deinem Beispiel. Also ein if in jeder 2. ~ 3. Zeile.

    Bzw. man kann natürlich immer eins machen: jede einzelne Funktion der C-Libary erstmal in eine eigene Funktion einwickeln, in der man Exceptions wirft wenn Dinge schief gehen. Und aus denen baut man dann seine "komplexeren" Funktionen auf.

    Zu Frage 5:
    Was meinst du mit "besser"? Du kannst es etwas kürzer schreiben indem du die unnötige Initialiserung von "ext" weglässt (wird ja danach gleich zugewiesen),
    und die unnötigen {} weg lässt.

    Zu Frage 6:
    Kommt drauf an. Ist der Wert "optional", dann "leeren" Wert zurückgeben (wobei wieder abhängig vom Typ und erlaubten Bereich des Wertes sein kann dass man sowas wie boost::optional braucht).
    Und mit "optional" meine ich einfach dass es kein Fehler ist wenn der Wert leer ist, und dass es gut denkbar ist dass der Aufrufer mit der Info "Wert ist leer" sinnvoll weiter arbeiten kann.

    Zu Frage 7:
    Definiere deine Variablen erst dort wo du sie brauchst und lass die unnötigen {} weg 😃
    String-Literals schmeissen ist ganz pfui, lass das.
    Und ich würde die Funktion die loggt und/oder ne Exception wirft und die die die eigentliche Arbeit macht trennen.
    z.B.

    bool TryGetBlub(params, std::string& out_blub, std::string& out_errorMessage)
    {
        // ...
    }
    
    std::string GetBlub(params)
    {
    	std::string blub;
        std::string errorMessage;
        if (!TryGetBlub(params, blub, errorMessage))
            throw std::runtime_error("Could not get blub: " + errorMessage);
        return blub;
    }
    

    ----

    Und ein 👍 von mir dafür dass du dir über solche Dinge Gedanken machst.



  • vielen Dank für die Tipps 😃

    hustbaer schrieb:

    Zu Frage 1:
    Variante 2, also
    "(Deklaration und Initialisierung von Variablen an den Stellen, an dem sie das erste mal benötigt werden.)"

    Aber kann das nicht ggf. Fehler provozieren? Ich denke an sowas:

    void meineFunktion()
    {
        ... code ...
        int a = 5;
        ... code ...
        if (irgendwas)
        {
            ... code ...
            int b = 6;
            ... code ...
        }
        ... code ...
        int c = a + b;  // --> Fehler, da b nur in der if-Schleife gültig war.
    }
    

    ...naja wahrscheinlich trifft sowas selten zu. 🙂

    hustbaer schrieb:

    Bzw. man kann natürlich immer eins machen: jede einzelne Funktion der C-Libary erstmal in eine eigene Funktion einwickeln, in der man Exceptions wirft wenn Dinge schief gehen. Und aus denen baut man dann seine "komplexeren" Funktionen auf.

    Jap. Ist gerade in Arbeit ;).

    hustbaer schrieb:

    Zu Frage 6:
    Kommt drauf an. Ist der Wert "optional", dann "leeren" Wert zurückgeben (wobei wieder abhängig vom Typ und erlaubten Bereich des Wertes sein kann dass man sowas wie boost::optional braucht).
    Und mit "optional" meine ich einfach dass es kein Fehler ist wenn der Wert leer ist, und dass es gut denkbar ist dass der Aufrufer mit der Info "Wert ist leer" sinnvoll weiter arbeiten kann.

    Ich glaube so eine ähnliche Frage hatte ich vor kurzem auch schon mal angestoßen. Ja bei mir sind die meisten getter eher optional und dürfen scheitern. Bei kritischen Fehlern, die den Programmfluss stören können, werden allerdings exceptions geworfen. Die ganze Sache mit der Fehlerbehandlung ist da auch nicht immer so einfach. Habe hier eine gute Gegenüberstellung gefunden: (http://www.codeproject.com/Articles/38449/C-Exceptions-Pros-and-Cons).

    Boost ist an sich eine gute Ergänzung, aber da ich diese momentan nicht benötige, wollte ich es jetzt auch nicht zwingend einbinden.

    hustbaer schrieb:

    String-Literals schmeissen ist ganz pfui, lass das.

    Meinst du damit den return von 'const char *'? Ist das Problematisch? Sollte ich lieber immer einen std::string zurückgeben?

    hustbaer schrieb:

    bool TryGetBlub(params, std::string& out_blub, std::string& out_errorMessage)
    {
        // ...
    }
     
    std::string GetBlub(params)
    {
        std::string blub;
        std::string errorMessage;
        if (!TryGetBlub(params, blub, errorMessage))
            throw std::runtime_error("Could not get blub: " + errorMessage);
        return blub;
    }
    

    Ha, das ist super. Die Idee ist mir noch gar nicht gekommen es so zu machen 😃

    ------------------------------------------

    Naja so hat wohl jeder sein Programmierstil. 😉
    Es ist aber interessant zu sehen, was es da so alles gibt (google: 'filetype:cpp').

    vielen Dank nochmals 😃


Log in to reply