15
\$\begingroup\$

I'm a C++ dev, and I've recently started working my way through Clean Code*. Whenever I encounter an example I think I could improve, I try to re-implement it in C++.

On pp. 28-29 there is an example with a message building class, which looks like this when implemented in C++ (includes and namespaces omitted):
.h

class GuessStatisticsMessage
{
public:
    GuessStatisticsMessage() = default;
    ~GuessStatisticsMessage() = default;

    std::string make(char candidate, int count);

private:
    void createPluralDependentMessageParts(int count);
    void createThereAreManyLetters(int count);
    void createThereIsOneLetter();
    void createThereAreNoLetters();

    std::string number;
    std::string verb;
    std::string pluralModifier;
};

.cpp

std::string GuessStatisticsMessage::make(char candidate, int count)
{
    createPluralDependentMessageParts(count);
    return fmt::format("There {} {} {}{}", verb, number, candidate, pluralModifier);
}

void GuessStatisticsMessage::createPluralDependentMessageParts(int count)
{
    if (count == 0) {
        createThereAreNoLetters();
    } else if (count == 1) {
        createThereIsOneLetter();
    } else {
        createThereAreManyLetters(count);
    }
}

void GuessStatisticsMessage::createThereAreManyLetters(int count)
{
    verb = "are";
    number = std::to_string(count);
    pluralModifier = "s";
}

void GuessStatisticsMessage::createThereIsOneLetter()
{
    verb = "is";
    number = "one";
    pluralModifier = "";
}

void GuessStatisticsMessage::createThereAreNoLetters()
{
    verb = "are";
    number = "no";
    pluralModifier = "s";
}

In an attempt to make this code more functional I ended up with the following:
.h

class GuessStatisticsMessageAlt
{
public:
    GuessStatisticsMessageAlt() = default;
    ~GuessStatisticsMessageAlt() = default;

    std::string make(char candidate, int count);

private:
    struct MessageComponents
    {
        std::string verb;
        std::string number;
        std::string pluralModifier;
    };

    MessageComponents createPluralDependentMessageParts(int count);
    MessageComponents createThereAreManyLetters(int count);
    MessageComponents createThereIsOneLetter();
    MessageComponents createThereAreNoLetters();
};

.cpp

std::string GuessStatisticsMessageAlt::make(char candidate, int count)
{
    auto messageComponents = createPluralDependentMessageParts(count);
    return fmt::format("There {} {} {}{}", messageComponents.verb, messageComponents.number, 
        candidate, messageComponents.pluralModifier);
}

GuessStatisticsMessageAlt::MessageComponents GuessStatisticsMessageAlt::createPluralDependentMessageParts(int count)
{
    if (count == 0) {
        return createThereAreNoLetters();
    } else if (count == 1) {
        return createThereIsOneLetter();
    } else {
        return createThereAreManyLetters(count);
    }
}

GuessStatisticsMessageAlt::MessageComponents GuessStatisticsMessageAlt::createThereAreManyLetters(int count)
{
    return { "are", std::to_string(count), "s" };
}

GuessStatisticsMessageAlt::MessageComponents GuessStatisticsMessageAlt::createThereIsOneLetter()
{
    return { "is", "one", "" };
}

GuessStatisticsMessageAlt::MessageComponents GuessStatisticsMessageAlt::createThereAreNoLetters()
{
    return { "are", "no", "s" };
}

Since at this point there was no real need for a class, I continued, reaching the final result:
.h

std::string makeGuessStatisticsMessage(char candidate, int count);

.cpp

namespace {

struct MessageComponents
{
    std::string verb;
    std::string number;
    std::string pluralModifier;
};

MessageComponents createThereAreManyLetters(int count)
{
    return { "are", std::to_string(count), "s" };
}

MessageComponents createThereIsOneLetter()
{
    return { "is", "one", "" };
}

MessageComponents createThereAreNoLetters()
{
    return { "are", "no", "s" };
}

MessageComponents createPluralDependentMessageParts(int count)
{
    if (count == 0) {
        return createThereAreNoLetters();
    }
    else if (count == 1) {
        return createThereIsOneLetter();
    }
    else {
        return createThereAreManyLetters(count);
    }
}

}

std::string makeGuessStatisticsMessage(char candidate, int count)
{
    auto messageComponents = createPluralDependentMessageParts(count);
    return fmt::format("There {} {} {}{}", messageComponents.verb, messageComponents.number,
        candidate, messageComponents.pluralModifier);
}

The point is, I grapple a bit with when to apply different programming paradigms and find it difficult to decide if my re-implementation is actually an improvement in this case. In particular, hiding so much stuff in the anonymous namespace is something that bothers me somewhat.

So the primary question is - is this really an improvement? And the secondary - how can I learn to judge this better?

I hope it isn't too opinion-based, as this is a real struggle for me.


* Clean Code: A Handbook of Agile Software Craftsmanship, Robert C. Martin

\$\endgroup\$
10
\$\begingroup\$
  • Getting rid of the class is definitely a step in the right direction.

  • Assuming count can never be negative, we should use an unsigned integer type for it.

  • I don't think the MessageComponents class is a good idea. It makes the design very inflexible. If we ever want to change the message, we'll have to change every single function there.

  • We're programming a very specific output message, and the code doesn't seem to be intended for re-use, so I don't think we need the named functions either.


I'd suggest something more like:

std::string makeStatisticsMessage(char letter, unsigned int count)
{
    auto const verb = (count == 1) ? "is" : "are";
    auto const number = (count == 0) ? "no" : (count == 1) ? "one" : std::to_string(count);
    auto const plural = (count == 1) ? "" : "s";
    return fmt::format("There {} {} '{}'{}.", verb, number, letter, plural);
}

Although we now check count multiple times, I'd argue that it's much easier to follow the logic for each component separately.

It also means that we can abstract or change the behavior for each component later (e.g. if we decide we want to print all the numbers as words instead of digits).

\$\endgroup\$
  • 1
    \$\begingroup\$ One reason to use a class rather than a simple function could be a redesign to allow multiple languages for the output. In this case I would define a interface with a virtual function makeStatisticsMessage() and several inherited classes for the specific languages which should be supported. \$\endgroup\$ – πάντα ῥεῖ Jul 21 at 12:15
  • 4
    \$\begingroup\$ Also to enhance readability of your conditional logic, I'd recommend to avoid nested ternary operations in favor of if / else statements. The result would be the same and it's much better to follow for anyone (beginner or expert). \$\endgroup\$ – πάντα ῥεῖ Jul 21 at 12:22
  • \$\begingroup\$ I've accepted this answer since the other one builds on it. Both of them were helpful, though. \$\endgroup\$ – SG_90 Jul 23 at 18:19
8
\$\begingroup\$

While I am supporting @user673679's approach a lot in terms that putting everything into a single function, I still have concerns:

  1. Using an interface might be better if you want to refactor the functionality to support multiple languages:

    struct IGuessStatisticsMessage {
         virtual std::string makeStatisticsMessage(char letter, unsigned int count) = 0;
    virtual ~IGuessStatisticsMessage() {}
    };
    

    This would allow to provide implementations for different languages easier:

    struct GuessStatisticsMessage_EN() : public IGuessStatisticsMessage {
        std::string makeStatisticsMessage(char letter, unsigned int count) {
            auto const verb = (count == 1) ? "is" : "are";
            auto const number = (count == 0) ? "no" : (count == 1) ? "one" : 
            std::to_string(count);
            auto const plural = (count == 1) ? "" : "s";
            return fmt::format("There {} {} '{}'{}.", verb, number, letter, plural);
        }       
    };
    
    struct GuessStatisticsMessage_DE() : public IGuessStatisticsMessage {
        std::string makeStatisticsMessage(char letter, unsigned int count) {
            auto const number = (count == 0) ? "kein" : (count == 1) ? "ein" : 
            std::to_string(count);
            auto const plural = (count == 1) ? "" : "s";
            return fmt::format("Es gibt {} '{}'{}.", number, letter, plural);
        }       
    };
    
  2. Also I am not a friend of nested ternary expressions, they generally tend to be hard to read. I'd rather use some code like this:

    std::string makeStatisticsMessage(char letter, unsigned int count) {
        auto const verb = (count == 1) ? "is" : "are";
        std::string number;
        if(count == 0) { 
            number = "no";
        else if (count == 1) { 
            number = "one";
        }
        else { 
            number = std::to_string(count);
        }
        auto const plural = (count == 1) ? "" : "s";
        return fmt::format("There {} {} '{}'{}.", verb, number, letter, plural);
    }       
    
\$\endgroup\$
  • \$\begingroup\$ Thanks for the contribution. Would it make sense to go a step further by getting rid of the verb/number/plural vars altogether and returning a complete string depending on the count (having multiple return statements), or would you insist on keeping the vars? \$\endgroup\$ – SG_90 Jul 23 at 18:16
  • \$\begingroup\$ @SG_90 The (local) variables are a minor point. Keeping a replacable interface is a major. \$\endgroup\$ – πάντα ῥεῖ Jul 23 at 18:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.