Is there a better way?

[dune]

I have a some if statements nested and was wondering if there's a better way to handle this. What I have works perfectly fine, but maybe there's another way, one that's considered a best practice.

This wants the user to input an book's ISBN in the format of n-n-n-x.

void Book::setIsbn() {
    string s;
    cout << "Book ISBN: ";
    getline(cin, s);

    if(isdigit(s[0]))
        if (isdigit(s[1]))
            if (isdigit(s[2]))
                if (isalpha(s[3])) {
                    s[3] = toupper(s[3]);
                    isbn.emplace_back(s);
                } else
                    setIsbn();
            else
                setIsbn();
        else
            setIsbn();
    else
        setIsbn();
}

Any and all suggestions are welcome.

[Ganado]

First, recursion is generally confusing, error-prone if not terminated correctly, and should generally be avoided for purposes of "repeat until correct". [I am not against using recursion in places where it may help readability, such as tree structures, if depth scales logarithmically]

Would probably be better to use && to combine your if-statements.

// Example program
#include <iostream>
#include <string>
#include <vector>
#include <cctype>
using namespace std;

bool isValidIsbn(const std::string& isbn)
{
    // ensure assumptions are correct
    if (isbn.length() < 4)
    {
        return false;   
    }
    
    return (isdigit(isbn[0]) && isdigit(isbn[1]) && isdigit(isbn[2]) && isalpha(isbn[3]));
}

void userInputIsbn(vector<string>& isbns)
{   
    while (true)
    {
        string isbn;
        cout << "Book ISBN: ";
        getline(cin, isbn);
        
        if (isValidIsbn(isbn))
        {
            isbn[3] = toupper(isbn[3]);
            isbns.emplace_back(isbn);
            break;
        }
        else
        {
            std::cout << "Invalid input, try again.\n";
        }
    }
}

int main()
{
    vector<string> isbns;
    userInputIsbn(isbns);
    
    for (const auto& isbn: isbns)
    {
        std::cout << isbn << '\n';   
    }
}

I avoided using a class for simplicity of example; the important part is understanding the loop and how you check for validity of user input.

I would actually make my "isValidIsbn" a local lambda function if it is not needed inside other functions.

void userInputIsbn(vector<string>& isbns)
{   
    auto isValidIsbn = [](const std::string& isbn)
    {
        // ensure assumptions are correct
        if (isbn.length() < 4)
        {
            return false;   
        }
        
        return (isdigit(isbn[0]) && isdigit(isbn[1]) && isdigit(isbn[2]) && isalpha(isbn[3]));
    };

    while (true)
    {
        string isbn;
        cout << "Book ISBN: ";
        getline(cin, isbn);
        
        if (isValidIsbn(isbn))
        {
            isbn[3] = toupper(isbn[3]);
            isbns.emplace_back(isbn);
            break;
        }
        else
        {
            std::cout << "Invalid input, try again.\n";
        }
    }
}

You can also use the template-subset of the standard library, henceforth referred to as the STL,

#include <algorithm>
// ...

bool isValidIsbn(const std::string& isbn)
{
    // ensure assumptions are correct
    if (isbn.length() < 4)
    {
        return false;   
    }
    
    return (std::all_of(&isbn[0], &isbn[3], ::isdigit) && isalpha(isbn[3]));
}

[dune]

@Ganado - Nicely explained. I knew recursion was frowned upon, but I haven't used the stuff you showed me enough to see them. Your use of std::all_of is one thing I haven't seen before. Anywhere. Or how you returned isbn:

return (isdigit(isbn[0]) && isdigit(isbn[1]) && isdigit(isbn[2]) && isalpha(isbn[3]));

Thank you.

As an aside, in your last example, shouldn't

return (std::all_of(&isbn[0], &isbn[3], ::isdigit) && isalpha(isbn[3]));

actually be

return (std::all_of(&isbn[0], &isbn[2], ::isdigit) && isalpha(isbn[3]));

I'll test it and see? Thanks again. Good stuff, this.

[Ganado]

No, I can see why you would think that, but how all functions within <algorithm> work is by using the concept of an iterator.

https://www.cplusplus.com/reference/algorithm/all_of/

template<class InputIterator, class UnaryPredicate>
  bool all_of (InputIterator first, InputIterator last, UnaryPredicate pred)
{
  while (first!=last) {
    if (!pred(*first)) return false;
    ++first;
  }
  return true;
}

The "first" iterator is the first element of the sequence, while the "last" iterator is one-past-the-end of the sequence, to delimit the sequence.

Essentially, the algorithm runs while it's not the end element. The end element itself is not dereferenced.

Perhaps the following expresses clearer intent:
std::all_of(&isbn[0], &isbn[0] + 3, ::isdigit) although the same conceptual issue exists there.
(Think of the "3" as being the number of elements checked) and not index 3.

[dune]

Thanks again. After looking at how that works, I see what you're saying.

return (std::all_of(&isbn[0], &isbn[3], ::isdigit) && isalpha(isbn[3]));

It's telling me that we're starting with isbn[0] and iterating out 3 with isbn[3], which will make it end on isbn[2]. Right?

[jonnin]

yes. 0, 1, 2 is 3 things.

[seeplus]

#include <iostream>
#include <string>
#include <algorithm>
#include <cctype>

auto userInputIsbn()
{
	const auto isValidIsbn {[](const auto& isbn)
	{
		return isbn.size() == 4 && std::isalpha(static_cast<unsigned char>(isbn[3])) && std::all_of(isbn.data(), isbn.data() + 3, [](unsigned ch) { return std::isdigit(ch); });
	}};

	std::string isbn;

	while (std::cout << "Book ISBN: " && getline(std::cin, isbn) && !isValidIsbn(isbn))
			std::cout << "Invalid input, try again.\n";

	isbn[3] = static_cast<char>(std::toupper(static_cast<unsigned char>(isbn[3])));
	return isbn;
}

int main()
{
	const auto isbn {userInputIsbn()};

	std::cout << isbn << '\n';
}