Parse strings and respond

Question

I'm doing some exercises from another good website aimed at a similar problem - exercism.io, and my 1st C++ problem was to pass a certain test suite.

Here is the README for relevant information:

Bob is a lackadaisical teenager. In conversation, his responses are very limited.

Bob answers 'Sure.' if you ask him a question.

He answers 'Whoa, chill out!' if you yell at him.

He says 'Fine. Be that way!' if you address him without actually saying anything.

He answers 'Whatever.' to anything else.

Here is the test suite:

BOOST_AUTO_TEST_CASE(stating_something)
{
 BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("Tom-ay-to, tom-aaaah-to."));
}
BOOST_AUTO_TEST_CASE(shouting)
{
 BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("WATCH OUT!"));
}
BOOST_AUTO_TEST_CASE(asking_a_question)
{
 BOOST_REQUIRE_EQUAL("Sure.", bob::hey("Does this cryogenic chamber make me look fat?"));
}
BOOST_AUTO_TEST_CASE(talking_forcefully)
{
 BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("Let's go make out behind the gym!"));
}
BOOST_AUTO_TEST_CASE(using_acronyms_in_regular_speech)
{
 BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("It's OK if you don't want to go to the DMV."));
}
BOOST_AUTO_TEST_CASE(forceful_questions)
{
 BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("WHAT THE HELL WERE YOU THINKING?"));
}
BOOST_AUTO_TEST_CASE(shouting_numbers)
{
 BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("1, 2, 3 GO!"));
}
BOOST_AUTO_TEST_CASE(only_numbers)
{
 BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("1, 2, 3"));
}
BOOST_AUTO_TEST_CASE(question_with_only_numbers)
{
 BOOST_REQUIRE_EQUAL("Sure.", bob::hey("4?"));
}
BOOST_AUTO_TEST_CASE(shouting_with_special_characters)
{
 BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("ZOMG THE %^*@#$(*^ ZOMBIES ARE COMING!!11!!1!"));
}
BOOST_AUTO_TEST_CASE(shouting_with_no_exclamation_mark)
{
 BOOST_REQUIRE_EQUAL("Whoa, chill out!", bob::hey("I HATE YOU"));
}
BOOST_AUTO_TEST_CASE(statement_containing_question_mark)
{
 BOOST_REQUIRE_EQUAL("Whatever.", bob::hey("Ending with a ? means a question."));
}
BOOST_AUTO_TEST_CASE(prattling_on)
{
 BOOST_REQUIRE_EQUAL("Sure.", bob::hey("Wait! Hang on. Are you going to be OK?"));
}
BOOST_AUTO_TEST_CASE(question_with_trailing_whitespace)
{
 BOOST_REQUIRE_EQUAL("Sure.", bob::hey("Are you ok? "));
}
BOOST_AUTO_TEST_CASE(silence)
{
 BOOST_REQUIRE_EQUAL("Fine. Be that way!", bob::hey(""));
}
BOOST_AUTO_TEST_CASE(prolonged_silence)
{
 BOOST_REQUIRE_EQUAL("Fine. Be that way!", bob::hey(" "));
}

Now here is my first-iteration code:

/*
 * #ifndef BOB_H
 * #define BOB_H
 *
 * #include <string>
 *
 * namespace bob {
 * std::string hey(std::string);
 * }
 * #endif
*/
#include <algorithm>
#include <cctype>
#include "bob.h"
std::string bob::hey(std::string s){
 //remove any whitespaces
 std::string::size_type pos = s.find(' ');
 while(pos != std::string::npos){
 s.erase(s.find(' '), 1);
 pos = s.find(' ' , pos);
 }
 //Oh , he never said anything? Fine. Be that way!
 if(s.empty()) return "Fine. Be that way!";
 bool allUpper = std::all_of(s.begin() , s.end() , [](char ch){ if(std::isupper(ch) || !std::isalpha(ch)) return true; else return false; });
 bool atleastAlpha = std::any_of(s.begin() , s.end() , [](char ch){ if(std::isalpha(ch)) return true; else return false; });
 //check for ques mark and say sure
 if(s.back() == '?' && (!allUpper || !atleastAlpha)) return "Sure.";
 //check for capital alphabets and yell Whoa, chill out!
 if(allUpper && atleastAlpha && !s.empty()) return "Whoa, chill out!";
 return "Whatever.";
}

In what ways can I improve my code? And is there anything I might be missing that is not according to the standards?

Answer 1

Return by const reference

Since you have a set of canned responses. You can create all your strings once and return by const reference. This will avoid a copy.

std::string const& bob::hey(std::string s){
 ^^^^^^
static std::string response_Sure = ""Sure.";
... etc.

USe standard algorithms when you can.

There is a standard algorithm for removing space. Easier to use that.

std::string::size_type pos = s.find(' ');
while(pos != std::string::npos){
 s.erase(s.find(' '), 1);
 pos = s.find(' ' , pos);
}

Its called the erase/remove idiom.

std::erase(std::remoce(std::begin(s), std::end(s), ' '), std::end(s));

Have one statement per line and use {} on sub statements.

//Oh , he never said anything? Fine. Be that way!
if(s.empty()) return "Fine. Be that way!";

I would write like this:

if (s.empty()) {
 return response_Fine;
}

The reason for using {} is that some function calls are actually macros (yes some people use them still). If they are bad enough to write macros they probably don't know how to do them well and thus they can be compound statements and would break an if/for/while sub block unless properly braced with {}. So it is just good habit to always use the braces.

Answer 2

Don't write long lines.

The reason Loki Astari didn't comment on

bool allUpper = std::all_of(s.begin() , s.end() , [](char ch){ if(std::isupper(ch) || !std::isalpha(ch)) return true; else return false; });

is that it is too long. Written properly as

bool allUpper = std::all_of(s.begin() , s.end() ,
 [](char ch){
 if(std::isupper(ch) || !std::isalpha(ch))
 return true; 
 else 
 return false;
 });

it'd prompt for another advice:

 if (condition)
 return true;
 else
 return false;

is a standard anti-pattern: it is equivalent to short and clean

 return condition;