Bsrgvty

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

• 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).

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);