1

I am writing code of calculator considering the priority of operation. What I am trying to do is to replace the priority that matches the regular expression I wrote and calculate it, and loop through it until the length of string, which is the input initially, becomes 1.

The below is my code, but running it results in an infinite loop.

const Calculator = function() {
    this.evaluate = string => {
    let regex = /\((\d+)\s[*/+-]\s(\d+)\)|(\d+)\s[*/]\s(\d+)|(\d+)\s[+-]\s(\d+)/g

        while(string.length > 1) {
            string.replace(regex, cal);
        }
        return string;
    }
};

function cal(argument) {
    let temp = argument.split(' ');
    let regexP = /\((\d+)|(\d+)\)/g

    if(temp[0].match(regexP)) {
        temp[0] = temp[0].slice(1);
        temp[2] = temp[2].slice(0, 1);
    }
    switch(temp[1]) {
        case '+': return +temp[0] + +temp[2];
        case '-': return +temp[0] - +temp[2];
        case '*': return +temp[0] * +temp[2];
        case '/': return +temp[0] / +temp[2];
        default: return undefined;
    }
}

let calculate = new Calculator()
console.log(calculate.evaluate("2 / 2 + 3 * 4 - 6"));

For some reason, the code is looping over and over and isn't returning a value.

What am I doing wrong, and how can I fix it?

Improve this question
0

2 Answers 2

Reset to default
2

You need to

(1) assign the result of calling .replace to the string (Javascript strings are immutable):

string.replace(regex, cal);

(2) The resulting string could have all +-*/ operations finished but still have a length larger than 1, eg, if the result was 3 * 4 resulting in 12. Use while(/[-+*/]/.test(string)) { instead:

const Calculator = function() {
    this.evaluate = string => {
    let regex = /\((\d+)\s[*/+-]\s(\d+)\)|(\d+)\s[*/]\s(\d+)|(\d+)\s[+-]\s(\d+)/g

        while(/[-+*/]/.test(string)) {
            string = string.replace(regex, cal);
        }
        return string;
    }
};

function cal(argument) {
    let temp = argument.split(' ');
    let regexP = /\((\d+)|(\d+)\)/g

    if(temp[0].match(regexP)) {
        temp[0] = temp[0].slice(1);
        temp[2] = temp[2].slice(0, 1);
    }
    switch(temp[1]) {
        case '+': return +temp[0] + +temp[2];
        case '-': return +temp[0] - +temp[2];
        case '*': return +temp[0] * +temp[2];
        case '/': return +temp[0] / +temp[2];
        default: return undefined;
    }
}

let calculate = new Calculator()
console.log(calculate.evaluate("2 / 2 + 3 * 4 - 6"));

You can also improve the code in cal by matching and destructuring the left digits, operator, and right digits in one go, with

`const [, left, operator, right] = matchedSubstr.match(/(\d+) ([-+*/]) (\d+)/);


const Calculator = function() {
    this.evaluate = string => {
    let regex = /\((\d+)\s[*/+-]\s(\d+)\)|(\d+)\s[*/]\s(\d+)|(\d+)\s[+-]\s(\d+)/g

        while(/[-+*/]/.test(string)) {
            string = string.replace(regex, cal);
        }
        return string;
    }
};

function cal(matchedSubstr) {
    const [, left, operator, right] = matchedSubstr.match(/(\d+) ([-+*/]) (\d+)/);
    switch(operator) {
        case '+': return +left + +right;
        case '-': return +left - right;
        case '*': return +left * right;
        case '/': return +left / right;
    }
}

let calculate = new Calculator()
console.log(calculate.evaluate("2 / 2 + 3 * 4 - 6"));

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

Wow. I have learned a lot from you. Thanks for your correction and giving me another perspective !
Can I ask one more question? The question would be abstract but I want to ask you this. Does the code look inefficient? Since I submit the code in Codewars that I am using to do code challenge, It keeps spitting out the message Execution timed out
No, it's not particularly inefficient. It's not great, but it's not bad. There could be efficiency improvements (by tokenizing the string, or by using eval instead), but if the submission is timing out, there are some input possibilities you're not accounting for. (if you post the inputs that don't work in the question, we can see what they are and try to fix them)
What is odd is the sample tests are passed but the time out, and there is no more test after that. since it should be passed before another samples will be passed. Thanks for your opinion. I have been studying JS for 3 months by myself, I am glad that I could help something from people like you! I will try to figure it out by myself again !
The sample tests are not the real tests, right? Sounds like the sample tests are passing, but the real tests are running into a logic error and infinite loop
|
1

CertainPerformance has already pointed out the mistakes that led to the undesired behaviour.

In order to get the priorities right, you should first resolve all parentheses, then the multiplications/divisions and then the additions/subtractions.

You could use three regular expressions for this (one for each), and use recursion to deal with any expression within parentheses. Note that such a sub-expression does not need to be just one operation. It could well have more operations and even parentheses. So parentheses can best be resolved from the inside out.

Here is how you could adapt your code for doing that:

const Calculator = function() {
    const cal = (a, op, b) => {
        switch(op) {
            case '+': return +a + +b;
            case '-': return a - b;
            case '*': return a * b;
            case '/': return a / b;
            case ')': return +this.evaluate(a); // Use recursion to resolve parentheses
        }
    };

    this.evaluate = string => {
        let regexes = [
            /\(([^()]+)(\))/g, // Priority 1: parentheses
            /(-?\d+(?:\.\d+)?)\s*([*\/])\s*(-?\d+(?:\.\d+)?)/, // Priority 2: multiplication & division
            /(-?\d+(?:\.\d+)?)\s*([+-])\s*(-?\d+(?:\.\d+)?)/, // Priority 3: addition & subtraction
        ];
        for (let regex of regexes) {
            let found;
            do {
                found = false;
                string = string.replace(regex, (_, ...args) => {
                    found = true;
                    return cal(...args).toFixed(12);
                });
            } while (found);
        }
        return string;
    };
};

let calculate = new Calculator()
console.log(+calculate.evaluate("2 / 2 + 3 * 4 - 6"));

The regular expressions allow for decimal points (as division may lead to fractional numbers), and negative numbers (as subtraction may yield that). So (\d+) became (-?\d+(?:\.\d+)?)

To avoid scientific notation getting inserted into the string, I use toFixed(12), assuming that this will be enough as precision.

For more efficiency look into the Shunting-yard algorithm. For supporting more operators, functions and precendence-settings in your string parsing, you could take inspiration from this answer

Improve this answer

8 Comments

Thanks for this big help and giving me a great perspective! now I am trying to compare this to mine.
If you need more help, it may be useful to point/link to the specific codewars kata you are looking at, as there are several variations, each with their specific requirements/assumptions.
codewars.com/kata/calculator/train/javascript. There are 3 fails out of more than 100 tests. Only one question is that in the replace method, what does '_' this underscore do?
I updated the answer to also pass those 3 tests. The issue was that for very large numbers, the default coercion into string would insert scientific notation. So I have made a few alterations, mainly using toFixed to avoid that scientific notation creeping in.
No no, I really appreciate your help. I have been having a trouble for this for long. so I am so thankful for your help
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.