Skip to main content
Update contents
Source Link
JimmyHu
  • 5.5k
  • 2
  • 10
  • 40
        for(std::size_t index = 0; index < operators.size(); ++index)
        {
            bool operatorFound = true;
            if(operators[index] == "sqrt-")
            {
                numbers[index] = std::sqrt(-numbers[index]);
            }
            else if(operators[index] == "sqrt")
            {
                numbers[index] = std::sqrt(numbers[index]);
            }
            // many more of these...
            else
            {
                operator_foundoperatorFound = false;
            }
            if (operator_foundoperatorFound)
            {
                reduce_operators(index);
                return;
            }
        }
        for(std::size_t index = 0; index < operators.size(); ++index)
        {
            bool operatorFound = true;
            if(operators[index] == "sqrt-")
            {
                numbers[index] = std::sqrt(-numbers[index]);
            }
            else if(operators[index] == "sqrt")
            {
                numbers[index] = std::sqrt(numbers[index]);
            }
            // many more of these...
            else
            {
                operator_found = false;
            }
            if (operator_found)
            {
                reduce_operators(index);
                return;
            }
        }
        for(std::size_t index = 0; index < operators.size(); ++index)
        {
            bool operatorFound = true;
            if(operators[index] == "sqrt-")
            {
                numbers[index] = std::sqrt(-numbers[index]);
            }
            else if(operators[index] == "sqrt")
            {
                numbers[index] = std::sqrt(numbers[index]);
            }
            // many more of these...
            else
            {
                operatorFound = false;
            }
            if (operatorFound)
            {
                reduce_operators(index);
                return;
            }
        }
deleted 18 characters in body
Source Link
// define operators as enums
    enum class Operator
    {
        none,
        add,
        subtract,
        plus,
        minus,
        abs,
        sqrt,
...
// define operator strings with corresponding enums
    constexpr static std::pair<std::string, Operator> op_names[] = {
            {"abs", Operator::abs },
            {"sqrt", Operator::sqrt},
            {"sin", Operator::sin},
...
// in the parser, iterate over these:  
        for(std::size_t index = 0; index < input_string.size();)
        {
            bool found_operator = false;
            for (const auto& op : op_names)
            {
                auto op_name = op.first;
                if(input_string.substr(index, op_name.size()) == op_name)
                {
                    operators.emplace_back(op.second);
                    index += op_name.size();
                    found_operator = true;
                    continue;break;
                }
            }
            if (found_operator)
            {
                continue;
            }
...
// define operators as enums
    enum class Operator
    {
        none,
        add,
        subtract,
        plus,
        minus,
        abs,
        sqrt,
...
// define operator strings with corresponding enums
    constexpr static std::pair<std::string, Operator> op_names[] = {
            {"abs", Operator::abs },
            {"sqrt", Operator::sqrt},
            {"sin", Operator::sin},
...
// in the parser, iterate over these:  
        for(std::size_t index = 0; index < input_string.size();)
        {
            bool found_operator = false;
            for (const auto& op : op_names)
            {
                auto op_name = op.first;
                if(input_string.substr(index, op_name.size()) == op_name)
                {
                    operators.emplace_back(op.second);
                    index += op_name.size();
                    found_operator = true;
                    continue;
                }
            }
            if (found_operator)
            {
                continue;
            }
...
// define operators as enums
    enum class Operator
    {
        add,
        subtract,
        plus,
        minus,
        abs,
        sqrt,
...
// define operator strings with corresponding enums
    constexpr static std::pair<std::string, Operator> op_names[] = {
            {"abs", Operator::abs },
            {"sqrt", Operator::sqrt},
            {"sin", Operator::sin},
...
// in the parser, iterate over these:  
        for(std::size_t index = 0; index < input_string.size();)
        {
            bool found_operator = false;
            for (const auto& op : op_names)
            {
                auto op_name = op.first;
                if(input_string.substr(index, op_name.size()) == op_name)
                {
                    operators.emplace_back(op.second);
                    index += op_name.size();
                    found_operator = true;
                    break;
                }
            }
            if (found_operator)
            {
                continue;
            }
...
More explanation
Source Link
  • Also in the parser: the found tokens remain as strings, and have to be parsed again as the same strings later. Better use something like enums, that later allow you to use switch instead of these long if constructs in perform_computing (1).
  • These same if constructs also have the same redundant code (reduce_operators(index)), which can be factored out (see below)(2).
  • The tests repeat the input string in the error message - this is also error prone. You write your own testing logic anyway, write a function that takes the input string and the expected result, make the calculation in that test function, and use the input also for the error message (3).

(1) To answer the question in the comment about redundant code inshow what I mean with the if constructsenums:

// define operators as enums
    enum class Operator
    {
        none,
        add,
        subtract,
        plus,
        minus,
        abs,
        sqrt,
...
// define operator strings with corresponding enums
    constexpr static std::pair<std::string, Operator> op_names[] = {
            {"abs", Operator::abs },
            {"sqrt", Operator::sqrt},
            {"sin", Operator::sin},
...
// in the parser, iterate over these:  
        for(std::size_t index = 0; index < input_string.size();)
        {
            bool found_operator = false;
            for (const auto& op : op_names)
            {
                auto op_name = op.first;
                if(input_string.substr(index, op_name.size()) == op_name)
                {
                    operators.emplace_back(op.second);
                    index += op_name.size();
                    found_operator = true;
                    continue;
                }
            }
            if (found_operator)
            {
                continue;
            }
...

(2) (question from comment) What I mean is that the code:

(and ultimately with enums by a switch statement, or even a function table).

(3) So your tests would look like this:

    auto start = std::chrono::system_clock::now();

    testExpression("-12.3456 + 2 ^ 3 * 3.123", 12.6384, 1);
    testExpression("-0.12345 * 3 - 5", -5.37035, 1);
    testExpression("1024*2+2048*4", 10240, 1);
...
  • Also in the parser: the found tokens remain as strings, and have to be parsed again as the same strings later. Better use something like enums, that later allow you to use switch instead of these long if constructs in perform_computing.
  • These same if constructs also have the same redundant code (reduce_operators(index)), which can be factored out (see below).
  • The tests repeat the input string in the error message - this is also error prone. You write your own testing logic anyway, write a function that takes the input string and the expected result, make the calculation in that test function, and use the input also for the error message.

To answer the question in the comment about redundant code in the if constructs: What I mean is that the code:

(and ultimately with enums by a switch statement, or even a function table).

  • Also in the parser: the found tokens remain as strings, and have to be parsed again as the same strings later. Better use something like enums, that later allow you to use switch instead of these long if constructs in perform_computing (1).
  • These same if constructs also have the same redundant code (reduce_operators(index)), which can be factored out (2).
  • The tests repeat the input string in the error message - this is also error prone. You write your own testing logic anyway, write a function that takes the input string and the expected result, make the calculation in that test function, and use the input also for the error message (3).

(1) To show what I mean with the enums:

// define operators as enums
    enum class Operator
    {
        none,
        add,
        subtract,
        plus,
        minus,
        abs,
        sqrt,
...
// define operator strings with corresponding enums
    constexpr static std::pair<std::string, Operator> op_names[] = {
            {"abs", Operator::abs },
            {"sqrt", Operator::sqrt},
            {"sin", Operator::sin},
...
// in the parser, iterate over these:  
        for(std::size_t index = 0; index < input_string.size();)
        {
            bool found_operator = false;
            for (const auto& op : op_names)
            {
                auto op_name = op.first;
                if(input_string.substr(index, op_name.size()) == op_name)
                {
                    operators.emplace_back(op.second);
                    index += op_name.size();
                    found_operator = true;
                    continue;
                }
            }
            if (found_operator)
            {
                continue;
            }
...

(2) (question from comment) What I mean is that the code:

(and ultimately with enums by a switch statement, or even a function table).

(3) So your tests would look like this:

    auto start = std::chrono::system_clock::now();

    testExpression("-12.3456 + 2 ^ 3 * 3.123", 12.6384, 1);
    testExpression("-0.12345 * 3 - 5", -5.37035, 1);
    testExpression("1024*2+2048*4", 10240, 1);
...
Answer to comment
Source Link
Loading
Source Link
Loading