8
\$\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;
}

thank you very much!, i replied lately because i had to do something and i forgot about it already

\$\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, 2021 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, 2021 at 11:48
  • 5
    \$\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, 2021 at 19:36

3 Answers 3

26
\$\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, 2021 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, 2021 at 9:35
  • 2
    \$\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, 2021 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, 2021 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, 2021 at 4:33
21
\$\begingroup\$

While in the general case of deconstructing 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 s) noexcept {
    return s.length() >= min_name_length
        && s.length() <= max_name_length
        && std::isalpha((unsigned char)s[0])
        && std::find_if(s.begin(), s.end(), isNotValid) == s.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 are 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 on the cards for them. Avoid the trap of implementation-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. The preprocessor plays by its own rules.

  6. Changed the only arguments name to s, because there is nothing for it convey not already in the function-name, and it is the only local in a nice short scope.

\$\endgroup\$
1
\$\begingroup\$

One more tip not mentioned by the others: Take your parameter as std::string_view instead of const std::string&. This will be more efficient if called with a lexical string literal or other C-style string.

Instead of seeing if you can find something that's not legal in the string, with its awkward need to test against end, and throwing away the position where it was found anyway; instead use std::all_of, which clearly states the intent: "do all the values pass the test?". Well, if you insist on having isNotValid as the predicate for testing each letter, then use none_of instead.

The code is not shown, but the lack of braces makes me suspect that isNotValid is a simple function. It actually generates better code to make such predicates functors (function objects) instead! Though if you had defined it as a lambda expression it would still be a bare name as shown; I really don't know what you did there.

\$\endgroup\$
1
  • \$\begingroup\$ std::string_view is point 2 of changes made in my answer. Using std::none_of() is an idea though. \$\endgroup\$ Jun 11, 2021 at 18:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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