7
\$\begingroup\$

I'm learning about how to create a good and clean code. I just need to review something like that, and I really not sure how can I fix this code into a clean code.

bool isValidUserName(const std::string& username)
{
    bool valid = false;

    if (username.length() >= MIN_NAME_LENGTH)
    {
        if (username.length() <= MAX_NAME_LENGTH)
        {
            if (isalpha(username[0]))
            {
                bool foundNotValidChar = std::find_if(username.begin(), username.end(), isNotValid) != username.end();
                if (!foundNotValidChar)
                {
                    valid = true;
                }
            }
        }
    }

    return valid;
}
\$\endgroup\$
3
  • \$\begingroup\$ Is this your own code? Also, what do you mean by "arrow code"? I don't see any arrows. \$\endgroup\$ – G. Sliepen Apr 11 at 10:51
  • 2
    \$\begingroup\$ I think @TiZ_Crocodile means that if-statements in the code are nested like "christmas tree" which also looks like "arrow" \$\endgroup\$ – Dimitry K Apr 11 at 11:48
  • 4
    \$\begingroup\$ @G.Sliepen "arrow anti-pattern" is a common name for code where the {} start to look like an arrow head (a triangle). For example, Jeff Atwood blogged about the arrow anti-pattern a while ago. \$\endgroup\$ – Peilonrayz Apr 11 at 19:36
22
\$\begingroup\$

We can avoid the excessive indentation by returning early.

Instead of checking for validity:

if (username.length() >= MIN_NAME_LENGTH)
{
    ... additional nested checks...
}

we reverse the check and return as soon as possible:

if (username.length < MIN_NAME_LENGTH)
    return false; // done!

So overall we'd get something like:

if (username.length() < MIN_NAME_LENGTH || username.length() > MAX_NAME_LENGTH)
    return false;

if (username.empty() || !std::isalpha(static_cast<unsigned char>(username.front()))) // paranoia: avoid crashing with empty username
    return false;

if (std::find_if(username.begin(), username.end(), isNotValid) != username.end())
    return false;

return true;

Note that isalpha is in the std:: namespace in C++. Technically we should also cast to unsigned char when calling the function. (We could wrap that in a function of our own).

\$\endgroup\$
7
  • 4
    \$\begingroup\$ I would move the empty check to the beginning of the function. Returning early if the user passes in an empty string asserts that every other call will guarantee at least a non-empty string. \$\endgroup\$ – Casey Apr 12 at 5:21
  • 3
    \$\begingroup\$ Since this is a code review site, I'd like to point out that many coding standards would forbid removing the { and } around the body of the if statement. It's far too easy to come along later and add a line in between the if and return, which will look at a glance like both lines are part of the same condition, but will actually make the return unconditional, completely breaking the function. Worst is when this happens because you're trying to debug something else, so you add a print or assert line in there. \$\endgroup\$ – IMSoP Apr 12 at 9:35
  • 1
    \$\begingroup\$ I don't personally agree. Anecdotally, I've come across many coding standards that recommend using one-line if or for statements where possible (with perhaps some restrictions for if / else if / else, and empty following statements), and none that forbid it. \$\endgroup\$ – user673679 Apr 12 at 12:24
  • 2
    \$\begingroup\$ @user673679 Fair enough, I thought it was fairly widely accepted. For what it's worth, the top two results I found for "C++ coding style" on DuckDuckGo were Mozilla's, which says "Always brace controlled statements"; and Google's, which allows limited use of un-braced if statements "for historical reasons", and notes that "Some projects require curly braces always." \$\endgroup\$ – IMSoP Apr 12 at 15:06
  • \$\begingroup\$ I haven't done c++ in a really, really long time but by the time you are executing the second if, isn't username.empty() impossible because you've already checked that username.length() < MIN_NAME_LENGTH? \$\endgroup\$ – JeffC Apr 13 at 4:33
17
\$\begingroup\$

While in the general case of deconstructiong arrow-code, conversion to guard-clauses as demonstrated by user673679 is the way to go, in this case there is an alternative way to escape the damage caused by blind adherence to single entry, single exit;
Change it to a single return-expression:

#include <algorithm>
#include <cctype>
#include <string_view>

bool isValidUserName(std::string_view username) noexcept {
    return username.length() >= min_name_length
        && username.length() <= max_name_length
        && std::isalpha((unsigned char)username[0])
        && std::find_if(username.begin(), username.end(), isNotValid) == username.end();
}

Other points changed:

  1. As by design, none of the tests can throw an exception, I amended the contract with noexcept.

  2. As the argument is only inspected and never stored, and none of the code needs a nul-terminated string, accepting as std::string by value or constant reference is both sub-optimal. Receiving a std::string_view by value is more flexible and efficient.
    See When should I use string_view in an interface? for more detail.

  3. Generally prefer the C++-headers over the C legacy-headers. They put all their members in the std-namespace like the other standard headers, (though might also clutter the global namespace, instead of the other way around).

  4. The character-classification-functions and case-conversion-functions expect an unsigned char-value or EOF (-1) in an int. Because they also accept EOF, accepting an unsigned char or converting internally is unfortunately not in the cards for them. Avoid the trap of implementations-defined raw char signedness by adding a cast.

  5. Changed min_name_length and max_name_length to lower case as upper case signals preprocessor defines, which they really shouldn't be.

\$\endgroup\$

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.