Checking if a username is valid

Question

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

Answer 1

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

Answer 2

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.

Answer 3

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.