1
\$\begingroup\$

Based on previous question, The code now is relied on std::regex for validate and split the input. For current improvement, I removed all duplicates functionality by reading the input and applies conversion in-place.

How can I improve it further.

#include <iostream>
#include <string>
#include <regex>
#include <cctype>
#include <limits>
#include <sstream>
#include <cassert>
#include <type_traits>
#include <stdexcept>

unsigned int string_to_int(const std::string& str)
{
    unsigned int value;
    std::stringstream ss(str);
    if (!(ss >> value) || !ss.eof())
    {
        throw std::invalid_argument("Oops\n");
    }
    return value;
}

unsigned int convert_to_numeric(const std::string& str)
{
    unsigned int value = 0;
    for (const auto& ch : str)
    {
        value = value * 26 + (ch - 'A' + 1);
    }
    return value;
}

std::string convert_to_alpha(unsigned int value)
{
    std::string result;
    do
        result =  static_cast<char>('A' + value % 26 - 1) + result;
    while (value /= 26);
    return result;
}

bool is_within_range(const std::string& row, unsigned int colVal)
{
    static const unsigned int Min = 1;
    static const unsigned int Max = 999999;

    const auto rowVal = string_to_int(row);
    return  rowVal >= Min
         && rowVal <= Max
         && colVal >= Min
         && colVal <= Max;
}

bool RC_to_Cell(const std::string& row, const std::string& col, std::ostream& os)
{
    bool is_valid = false;
    auto value = string_to_int(col);
    is_valid = is_within_range(row, value);
    if (is_valid)
    {
        const auto& result = convert_to_alpha(value);
        os << "\n\nResult: " << result << row << "\n\n";
    }
    return is_valid;
}

bool Cell_to_RC(const std::string& row, const std::string& col, std::ostream& os)
{
    bool is_valid = false;
    auto value = convert_to_numeric(col);
    is_valid = is_within_range(row, value);
    if (is_valid)
    {
        os << "\n\nResult: R" << row << "C" << value << "\n\n";
    }
    return is_valid;
}

bool convert(const std::string& in, std::ostream& os)
{
    static const std::regex Pattern{"^(?:R([^0]\\d*+)C([^0]\\d*+))$|^(?:([A-Z]+)([^0]\\d*+))$"};

    std::smatch matches;
    bool is_valid = false;

    if (!std::regex_match(in, matches, Pattern)) return is_valid;

    if (matches[1].matched && matches[2].matched)
        is_valid = RC_to_Cell(matches[1].str(), matches[2].str(), os);
    else
        is_valid = Cell_to_RC(matches[4].str(), matches[3].str(), os);

    return is_valid;
}

template <typename Input, typename Prompt, typename Handle>
Input read_input(Prompt&& prompt, Handle&& handle_input)
{
    std::istream& is = std::cin;
    std::ostream& os = std::cout;
    Input input{};
    bool is_not_valid = false;
    static const std::string Error{"Not valid input.\n"};

    do
    {
        os << std::forward<Prompt>(prompt) << std::flush;

        is_not_valid = std::forward<Handle>(handle_input)(input, is, os);

        if (is_not_valid)
        {
            os << Error;
        }

        is.clear();
        is.ignore(std::cin.rdbuf()->in_avail());
    } while (is_not_valid);

    return input;
}

template <typename Prompt>
unsigned int get_count(Prompt&& prompt)
{
    static auto int_input = [] (auto& in, std::istream& is, std::ostream&)
    {
        std::string s;
        std::getline(is, s);
        std::stringstream ss{s};
        return is.fail() || !(ss >> in) || !ss.eof() || (in <= 0 || in >= 100000);
    };

    return read_input<unsigned int>(std::forward<Prompt>(prompt), int_input);
}

template <typename Prompt>
void read_and_convert(Prompt&& prompt)
{
    static auto string_input = [] (auto& in, std::istream& is, std::ostream& os)
    {
        std::getline(is, in);
        return  is.fail() || !convert(in, os);
    };

    read_input<std::string>(std::forward<Prompt>(prompt), string_input);
}

int main()
{
    auto n = get_count("Enter a number of entries: ");

    std::cout << "\n\n\tWelcome\n\n";

    do
        read_and_convert("Enter your choice: ");
    while (--n);
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Extend your regular expressions to encompass is_within_range

My regular expression knowledge has some holes in it, but my reading of yours is that you're ignoring any leading zeros, then matching any number of digits for the relevant numeric fields. You could amend this, so that it actually checked to ensure that after any leading zeros, there were up to 6 digits. I think you need to look at replacing the + with {1,6}, but I would have to check it to be sure. This would effectively bound the numeric inputs to within the range expected by your is_within_range function. You could do a similar thing to bound the range of alpha column option. If you do both of these, then the is_within_range method becomes redundant.

Don't initialize variables to unused values

This may be subjective, however you do this in a couple of places:

bool is_valid = false;
auto value = string_to_int(col);
is_valid = is_within_range(row, value);

The initial value for is_valid isn't used, so it would be better replaced with:

auto value = string_to_int(col);
bool is_valid = is_within_range(row, value);  // or auto is_valid =

Unnecessary checks

In convert, you check if both group 1 & 2 have been matched. You only really need to check if one of the values has matched in order to perform your branching logic, so one of the matches could be removed. Alternatively, for consistency you could check to ensure that 3 & 4 have matched before using them in the else clause.

if (matches[1].matched)
    is_valid = RC_to_Cell(matches[1].str(), matches[2].str(), os);
else
    is_valid = Cell_to_RC(matches[4].str(), matches[3].str(), os);

OR

if (matches[1].matched && matches[2].matched)
    is_valid = RC_to_Cell(matches[1].str(), matches[2].str(), os);
else if (matches[3].matched && matches[4].matched)
    is_valid = Cell_to_RC(matches[4].str(), matches[3].str(), os);
\$\endgroup\$

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.