3. Code refactoring C/C++

Code refactoring C/C++

  • O

    What do you think of this Method? Can I do anything to make it more readable or less repetitive? The Set methods contain checks that return an error code or OK.
    Any suggestion will help
    (the data types must stay as they are)

    ErrorCode NetworkLib::Client::EstablishConnection(const char* host, uint16_t port, char* clientId, 
    const char* customKey, const char* customValue, uint16_t timeout, uint32_t expiryInterval,
    const char* authMethod, const char* authData, uint16_t maxReceive, uint32_t maxPacketSize, 
    uint16_t maxAlias, bool responseFlag, bool problemFlag, uint32_t delayInterval, 
    bool formatFlag, uint32_t msgExpiry, const char* dataType, const char* replyTopic, const char* correlationInfo) 
{
    this->host = strdup(host);
    this->port = port;
    ErrorCode result = OK;
    ConnectionHandler handler = ConnectionHandler(timeout, clientId);

    result = handler.header.sessionExpiry.Set(expiryInterval);
    if (result) return result;

    result = handler.header.receiveLimit.Set(maxReceive);
    if (result) return result;

    result = handler.header.packetSizeLimit.Set(maxPacketSize);
    if (result) return result;

    result = handler.header.aliasLimit.Set(maxAlias);
    if (result) return result;

    result = handler.header.enableResponse.Set((uint8_t)responseFlag);
    if (result) return result;

    result = handler.header.enableProblem.Set((uint8_t)problemFlag);
    if (result) return result;

    if (customKey && customValue) {
        result = ConvertToCustomProperty(&handler.header, customKey, customValue);
        if (result) return result;
    }

    if (authMethod) {
        result = handler.header.authMethod.Set(authMethod);
        if (result) return result;
    }

    if (authData) {
        result = handler.header.authPayload.Set((uint8_t*)authData, strlen(authData));
        if (result) return result;
    }

    result = handler.body.delayInterval.Set(delayInterval);
    if (result) return result;

    result = handler.body.formatIndicator.Set(formatFlag);
    if (result) return result;

    result = handler.body.expiryTime.Set(msgExpiry);
    if (result) return result;

    if (dataType) {
        result = handler.body.dataType.Set(dataType);
        if (result) return result;
    }

    if (replyTopic) {
        result = handler.body.replyTopic.Set(replyTopic);
        if (result) return result;
    }

    if (correlationInfo) {
        result = handler.body.correlationInfo.Set((uint8_t*)correlationInfo, strlen(correlationInfo));
        if (result) return result;
    }

    result = handler.Encode(buffer, bufferLength);
    if (result) return result;

    resolver->ResolveHostName(host, this, logger);

    return OK;
}
    0
    C
     1 Antwort
  • C

    @orangejuice23 sagte in Code refactoring C/C++:

    the data types must stay as they are

    That deprives you of pretty much every opportunity to refactor what is ugly with this function. The number of function parameters should be greatly reduced. The obvious approach would be to pack parameters that semantically belong together into structs and then pass those.

    I'd have suggested to also pack all those set-functions into a vector of std::functions and iterate over them, but then they have differing signatures, don't know how to go about that in a readable, comprehensible way.

    Very minor:

    this->host = strdup(host);

    Just my personal taste, but I think it's bad style having to mention this explicitly only to make the function parameter disinguishable from the member. I'd choose different names in the first place, using a prefix or sth.

    0

  • Herzlichen Willkommen @orangejuice23 !
    Wenn ich z.b. in einem englischsprachigen Forum unterwegs bin, mache ich mir die Mühe, meine Beiträge in Englisch zu verfassen.
    Ich bin der Meinung, dass es schon einen Sinn macht, Foren zu einem Thema in verschiedenen Sprachen vorzufinden. Auch wenn einige meinen, dass Englisch die Sprache der Programmierer::innen ist. Das würde aber dieses Forum wiederum in Frage stellen.
    Ich nutze z.b. in mir fremdsprachlichen Foren gerne einen "Übersetzer".
    Mit besten Grüßen

    PS. Der Vorschlag von @cpr scheint mir auch schlüssig, das mit den Strukturen.
    Attributen einer Klasse stelle ich persönlich ein "m_" voran. "This" benutze ich aber auch, und zwar bei Aufrufen von "eigenen" Methoden.

    0
    F
     1 Antwort
  • F

    @Helmut-Jakoby sagte in Code refactoring C/C++:

    PS. Der Vorschlag von @cpr scheint mir auch schlüssig, das mit den Strukturen.
    Attributen einer Klasse stelle ich persönlich ein "m_" voran. "This" benutze ich aber auch, und zwar bei Aufrufen von "eigenen" Methoden.

    Ich persönlich bevorzuge direkte Namen und empfinde solche Präfixe eher als Krücken. Entweder sind die Funktionen so kurz, so dass man leicht Member von Parametern oder lokalen Variablen unterscheiden kann, oder der Editor hebt mir ohnehin die verschiedenen Variablen-Kategorien entsprechend hervor.

    Was Namenskonflikte angeht, so gebe ich Parametern und Membern durchaus gerne die selben Namen, wenn sie die exakt selbe Sache repräsentieren. Das lässt sich nämlich meistens elegant mit einer Member-Initialisierungsliste lösen:

    Connection(const char* host, ...)
: host{ strdup(host) }, ...
{
    ...
}

    In diesem Fall hier natürlich nicht, da es sich nicht um einen Konstruktor handelt, aber bei mir wären die Verbindungsdaten wahrscheinlich auch eine eigene Klasse, so dass das letztendlich ebenfalls ein Konstruktoraufruf wäre:

    EstablishConnection(const char* host, ...)
: connection_data{ host, ... }, ...
{
    ...
}

    Aber letztendlich sind Namen Geschmackssache, ich hab's halt gern direkt und simpel ohne zu viel "Dekorationen".

    @orangejuice23 @cpr Regarding the number of parameters i was thinking it would be neat if C++ had keyword arguments just like Python. I then came up with the idea to emulate this using C++20 Designated Initializers, which achieves a similar effect:

    #include <iostream>
#include <cstdint>
#include <string>
#include <print>

using namespace std;

struct ConnectionParams
{
    string host;
    uint16_t port = 80;
    string clientId;
    string customKey = "default_key";
    string customValue = "default_value";
    uint16_t timeout = 300;
};

struct Connection
{
    Connection(ConnectionParams params)
    : params{ params }
    {
    }

    ConnectionParams params;
};


auto main() -> int
{
    Connection c{{ .host = "my-host", .clientId = "client1" }};

    std::print(
        R"(
            host:{0}
            port:{1}
            clientId:{2}
            customKey:{3}
            customValue:{4}
            timeout:{5}
        )",
        c.params.host,
        c.params.port,
        c.params.clientId,
        c.params.customKey,
        c.params.customValue,
        c.params.timeout
    ); 
}

    Output:

               host:my-host
           port:80
           clientId:client1
           customKey:default_key
           customValue:default_value
           timeout:300

    https://godbolt.org/z/vWjWcr86G

    This approach allows to specify default values for parameters and only pass those which are relevant in the current context. I think that would be a nice solution, as you don't have to clutter your code by passing all arguments whenever a connection is established. You have to pass the arguments in the same order they were defined in the struct though (Designated Initializers require this), but you can leave out the ones where you are fine with the default values.

    1
Anmelden zum Antworten