4
\$\begingroup\$

The task is to implement a solveEquation function, which solves the Quadratic equation. Each equality has exact 2 integer solutions. Return those numbers as an ordered array.

Example:

const solutions = solveEquation('2 * x^2 - 10 * x + 12');
console.log(solutions); // [2, 3]

Solution:

function solveEquation(equation) {
    // Your solution here
    let a, b, c;
    // Using ES6 destructuring
    [a, b, c] = getValues(equation);
    return getXList(a, b, c);
}

// Determine a,b,c and return in an array
function getValues(equation) {
    // ax^2 + bx + c
    let a = equation.split(' * x^2')[0];
    let aSign = equation.split(a + ' * x^2')[0];
    // Assigns +- a
    a = parseInt(aSign + a);
    let bSign = equation.split(a + ' * x^2 ')[1].charAt(0);
    let b = equation.split(' * x^2 ' + bSign + ' ')[1].split(' * x ')[0];
    // Assigns +- b
    b = parseInt(bSign + b);
    // Assigns +- b
    let cSign = equation.split(' * x ')[1].charAt(0);
    let c = equation.split(' * x ' + cSign + ' ')[1];
    // Assigns +- c
    c = parseInt(cSign + c);
    return [a, b, c];
}
// Returns ordered Array
function getXList(a, b, c) {
    let list = [];
    list.push(getX('+', a, b, c));
    list.push(getX('-', a, b, c));
    list.sort((a, b) => a - b);
    return list;
}
// Returns X
function getX(sign, a, b, c) {
    // represent x = (- b +- √b^2-4ac)/2a as x = (b1 +- c1)/a1 
    let x, b1, a1, c1;
    b1 = parseInt(- + b);
    a1 = 2 * a;
    c1 = Math.sqrt((b * b) - 4 * a * c);
    return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1));
}

console.log(solveEquation('2 * x^2 - 10 * x + 12'));// [2, 3]
console.log(solveEquation('-20 * x^2 - 108797540 * x - 130011773690520'));// [-3667291, -1772586]
console.log(solveEquation('294 * x^2 - 141195558 * x - 1600964090384736'));// [-2105744, 2586001]
console.log(solveEquation('-267 * x^2 + 296412186 * x + 4722715166392080'));// [-3687112, 4797270]
console.log(solveEquation('-381 * x^2 + 2871374115 * x - 5385906614046864'));// [3516911, 4019504]
console.log(solveEquation('-154 * x^2 + 645219652 * x - 602658645850800'));// [1405588, 2784150]

The main reason I post this program is to get critical feedback and perhaps improve my coding style.

\$\endgroup\$
  • \$\begingroup\$ Could you specify the input constraints as your code crashes a lot. and I can not get it to solve anything apart from your exact example. \$\endgroup\$ – Blindman67 Apr 17 '18 at 12:46
  • \$\begingroup\$ Probably instead of using split you can use regular expression to get the values of a b and c \$\endgroup\$ – Karan Khanna Apr 17 '18 at 12:53
  • \$\begingroup\$ a1, b1 and c1 are not required. you can directly use 2a -b .. \$\endgroup\$ – Karan Khanna Apr 17 '18 at 12:54
  • \$\begingroup\$ @Blindman67 please share your failing input. I have added more working examples. \$\endgroup\$ – Shubhendu Vaid Apr 17 '18 at 12:54
  • 1
    \$\begingroup\$ instead of return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); use return Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); \$\endgroup\$ – Karan Khanna Apr 17 '18 at 12:55
4
\$\begingroup\$

Buggy as.

Unusable I made a single change and then could not get it to work (turned out I added an extra space)

Bugs

Any spaces in the wrong spot, and deviations from the simplified equation, any fractions and more will cause the code to throw a variety of errors.

Expectation

When you create a function people that use it have idiomatic expectations. You must attempt to conform at least to common variations on these expectations, and or ensure that your code does not throw errors if the input is not as expected.

One would expect a equation solver to be robust and handle all the standard input variations, like be

  • invariant to white spaces,
  • invariant to equation layout (1 * x ^ 2 + 1 * x ^ 2) === ( 2 * x ^ 2)
  • invariant to form of power (x * x) === (x ^ 2)
  • invariant to implied constants (x + 0) === (x), (1*x) === (x) and (2*x^2+0*x) === (2*x^2)
  • invariant to equation form (x + 1) ^ 2 === ((x + 1) * (x + 1)) === (x ^ 2 + 2 * x + 1)
  • I invariant to implied multiplier (2x === 2 * x) Note the number (or variable) if before x and has no operator, the multiplication * is always implied.

Other problems.

Not all quadratics have solutions.

  • A quadratic can have zero solutions as it is above of below the x axis. Eg x ^ 2 + 1
  • A quadratic can have one solution if it just touches the x axis eg x ^ 2
  • A quadratic can have two solution eg x ^ 2 - 1 * x

Quadratics can also include fractions 1/2 or 0.5. You should also be able to add the equations strings together, two or more 2nd order polynomials added together (joined with a "+" or "-") should equate into a valid quadratic.

Naming

The function naming is rather bad. To name a few examples

  • getValues maybe parseEquation
  • getXList mabe solveQuadratic
  • list maybe coefficients

Constants

You should use const for constants. Some examples...

const [a, b, c] = getValues(equation);

const list = [];

const cSign = equation.split(' * x ')[1].charAt(0);

And most likely more problems but as the code is so hard to use I have not looked any deeper into your algorithm.

Conclusion.

I would say "Try again." as it is not at all what I would expect from a function that solves quadratics.

\$\endgroup\$
2
\$\begingroup\$

Just adding a couple of things to what @Blindman67 already said, and focusing a bit on the code itself.

solveEquation

The declaration and assignment:

let a, b, c;
// Using ES6 destructuring
[a, b, c] = getValues(equation);

Don't need to be separated in such a simple statement, regardless whether you use destructuring or not. You can directly do:

let [a, b, c] = getValues(equation);

I'm not going to go deep into variables and functions names since @Blindman67 already covered them. But i still can't help but mention them again. They are far more important than they look, and you haven't picked them properly. Take your time picking them, it's definitely worth it.

getXList

In this one you repeat the same logic twice:

list.push(getX('+', a, b, c));
list.push(getX('-', a, b, c));

When we look closely to getX we can see that the code is barely the same, except for the very final returned root. So this leans towards repeated logic and creates all sort of problems. In this case it also makes your code less efficient because you repeat some part of calculations. Better would be to restructure your getX to return an array with both roots.

getX

Restructuring this one to return both roots could look like this:

function getRoots(a, b, c) {
    // represent x = (- b +- √b^2-4ac)/2a as x = (b1 +- c1)/a1 
    const numeratorRoot = Math.sqrt((b * b) - 4 * a * c);
    const denominator = 2 * a;
    const root1 = (-b + numeratorRoot) / denominator;
    const root2 = (+b + numeratorRoot) / denominator;
    return [root1, root2];
}

In this refactored version both roots are returned as an array. I included +b solely for clarity. You also had an x variable that wasn't being used at all. Always be aware of those, as they clutter the code and leave readers wondering what they are for.

With this new roots calculation getXList could be way simpler:

function getXList(a, b, c) {
    const roots = getRoots(a,b,c);
    roots.sort((a, b) => a - b);
    return roots;
}

Which given what it does, a better name would be getSortedRoots. Given that all it does is sort the roots, its even questionable if it should exist, and probably better would be to do the sorting directly in getRoots.

Considering we are talking about two values, you could easily get away with:

return root1 < root2 ? [root1, root2] : [root2, root1];

Or:

return [Math.min(root1, root2), Math.max(root1, root2)];

getValues

The getValues function actually parses the coeficients for each degree term. I won't dive too deep in to this one, but i'll start saying that you do a lot of splits. And many of them are almost identical, being not only inefficient as well as confusing.

Consider this one for example:

let a = equation.split(' * x^2')[0];
let aSign = equation.split(a + ' * x^2')[0];
a = parseInt(aSign + a);

Let's say we have 2 * x^2 - 10 * x + 12. Splitting on * x^2 yields:

['2' , ' - 10 * x + 12']

Now if we had -2 * x^2 - 10 * x + 12 the same operation would now give:

['-2' , ' - 10 * x + 12']

Both first blocks have the exact value you are looking for and aSign will always be an empty string because there is nothing to the left. Thus it won't affect the result at all. So you can drop the aSign altogether, and adjust the parseInt appropriately.

Personally i feel you must rethink your logic a bit on this one. Restating @Blindman67, you should take spaces into consideration to make the structure less rigid. On a simpler level you can strip down the spaces before parsing:

equation = equation.replace(/\s/g, "");

And adjust the rest accordingly.

\$\endgroup\$
2
\$\begingroup\$

I feel like I'm piling on here, but I hope you're able to take all of these solutions as they're intended: trying to help you become a better programmer.

I'm going to focus on the comments. Almost none of your comments are useful as they stand. The purpose of comments is to explain the goal of the code, not the nitty gritty implementation details. Let's look at a few problems:

// Your solution here
let a, b, c;

What you're trying to communicate here is that the variables a, b, c will store the end result of some process. A few problems. Comments are for the reader of the code, not the user. So using the word "your" is inappropriate. Secondly, the solution of the problem, according to the problem statement, is to find the two roots. So I don't see why you're calling these three variables the solution. They're coefficients of the terms of the quadratic, right? Finally, the word "here" is confusing because it suggests that this is the place in the code that computes some values. It's not. You probably can't pick better names, but the code should explain what they're used for: coefficients parsed from user input.

// Using ES6 destructuring
[a, b, c] = ...

I infer you added this comment because you're afraid that someone reading the code won't know about the new syntax that ES6 added. However, the code as it stands is completely obvious, even if someone doesn't know about ES6. Maybe the reader can't write code like this, but they'll be able to interpret what's going on. This code doesn't need a comment. A side note -- your code does not even check to make sure it's running under an ES6 engine.

// Determine a,b,c and return in an array
function getValues(equation) {

This comment is superfluous. Anyone looking at the code can see that some computation is done and then [a,b,c] are returned. Rather, the comment should explain the purpose of the function: why the code is doing what it's doing.

// ax^2 + bx + c

Even after understanding your code, I have no idea why you added this comment here. It's the formula for a generic quadratic equation, but it's in a format your code doesn't support parsing. Comments need to explain what the code is supposed to be doing, in a more verbose manner than variable and function names.

// Assigns +- a
a = ...

Comments should not just restate what the code is doing. It's obvious that the next line is assigning something to a. Also, the +- notation is very hard to understand.

// Assigns +- b
b = parseInt(bSign + b);
// Assigns +- b
let cSign = equation.split(' * x ')[1].charAt(0);

Same problem here, but this time the comment is both before and after the line in question. Why? Also there are many other lines that are not clear on first glance, why didn't you comment them?

// Returns ordered Array

This comment is incorrect. It doesn't return a sorted array; it returns the pair of roots it computed, with the small root before the larger root.

// Returns X

The purpose of comments is to explain the code. Don't add comments just because you're "supposed to". That just adds noise to the code. How is this comment supposed to help the reader of the code? They are looking to find out what the purpose of the function is!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.