Ytdyklly

I'm extracting variables from user entered mathematical expressions, so that I can enter them into the expression evaluator exprtk. For now, the condition is given that variable names will be composed of consecutive capital letters. Any mathematical constants or function names will be lowercase. Some hypothetical inputs and expected outputs:

Input: "(A + B) - 2"

Expected Output: { A, B }

Input: "pow(AC,E) * ( E * F )"

Expected Output: { AC, E, F }

The code I have right now seems to work, but I would love a critique. I add a space to the end of the string to make my algorithm catch cases at the end of the string, which seems a bit hacky(?).

template<class T>

std::vector<std::string> MapEvaluator<T>::extractVariables(std::string expression) {



    expression = expression + " ";

    std::vector<std::string> variables = {};

    std::string substring;

    size_t consecutiveCharsCaps = 0;

    bool previousCharCaps = false;



    for (size_t i = 0; i < expression.length(); i++)

    {

        if (isCapital(expression[i]))

        {

            consecutiveCharsCaps++;

            previousCharCaps = true;

        }

        else {

            if(previousCharCaps) {

                substring = expression.substr(i - consecutiveCharsCaps, consecutiveCharsCaps);

                variables.push_back(substring);

                consecutiveCharsCaps = 0;

                previousCharCaps = false;

            }

        }

    }



    unique(variables);

    return variables;

}



template <class T>

void MapEvaluator<T>::unique(std::vector<std::string> &vec)

{

    auto end = vec.end();

    for (auto it = vec.begin(); it != end; ++it) {

        end = std::remove(it + 1, end, *it);

    }

    vec.erase(end, vec.end());

}



template<class T>

bool MapEvaluator<T>::isCapital(char c) {

    return (c >='A' && c <= 'Z');

}

Some observations:

• It seems that std::adjacent_find essentially already does what you want, which is extracting all continuous sub-strings consisting of uppercase letters.




• Removing duplicates from a vector might be fine, but you can also avoid this completely by inserting the found elements into a std::set. I suspect that the number of unique variables is always small, so this is a cleaner approach.




• There is no reason for isCapital to be a member function. Instead, it should be a free function. Remember that interfaces should be complete but minimal. But in fact, there's no reason for the function in the first place: the standard already has std::isupper that we should rather use.

So with these in mind, we can re-write your function to e.g.,:

std::vector<std::string> get_variables(const std::string& str)

{

    std::set<std::string> vars;



    for (auto first = str.cbegin(); first != str.cend(); )

    {

        auto var_end = std::adjacent_find(first, str.cend(),

            (char a, char b) { return std::isupper(a) != std::isupper(b); });



        if (var_end != str.cend())

        {

            ++var_end;

        }



        if (is_capital(*first))

        {

            vars.insert(std::string(first, var_end));

        }



        first = var_end;

    }



    return std::vector<std::string>(vars.cbegin(), vars.cend());

}