Quadratic equation solver in JavaScript
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
4
down vote
favorite
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.
javascript parsing
 |Â
show 3 more comments
up vote
4
down vote
favorite
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.
javascript parsing
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.
â Blindman67
Apr 17 at 12:46
Probably instead of using split you can use regular expression to get the values of a b and c
â Karan Khanna
Apr 17 at 12:53
a1, b1 and c1 are not required. you can directly use 2a -b ..
â Karan Khanna
Apr 17 at 12:54
@Blindman67 please share your failing input. I have added more working examples.
â Shubhendu Vaid
Apr 17 at 12:54
1
instead of return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); use return Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1));
â Karan Khanna
Apr 17 at 12:55
 |Â
show 3 more comments
up vote
4
down vote
favorite
up vote
4
down vote
favorite
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.
javascript parsing
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.
javascript parsing
edited Apr 17 at 23:52
Jamalâ¦
30.1k11114225
30.1k11114225
asked Apr 17 at 12:36
Shubhendu Vaid
1497
1497
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.
â Blindman67
Apr 17 at 12:46
Probably instead of using split you can use regular expression to get the values of a b and c
â Karan Khanna
Apr 17 at 12:53
a1, b1 and c1 are not required. you can directly use 2a -b ..
â Karan Khanna
Apr 17 at 12:54
@Blindman67 please share your failing input. I have added more working examples.
â Shubhendu Vaid
Apr 17 at 12:54
1
instead of return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); use return Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1));
â Karan Khanna
Apr 17 at 12:55
 |Â
show 3 more comments
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.
â Blindman67
Apr 17 at 12:46
Probably instead of using split you can use regular expression to get the values of a b and c
â Karan Khanna
Apr 17 at 12:53
a1, b1 and c1 are not required. you can directly use 2a -b ..
â Karan Khanna
Apr 17 at 12:54
@Blindman67 please share your failing input. I have added more working examples.
â Shubhendu Vaid
Apr 17 at 12:54
1
instead of return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); use return Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1));
â Karan Khanna
Apr 17 at 12:55
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.
â Blindman67
Apr 17 at 12:46
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.
â Blindman67
Apr 17 at 12:46
Probably instead of using split you can use regular expression to get the values of a b and c
â Karan Khanna
Apr 17 at 12:53
Probably instead of using split you can use regular expression to get the values of a b and c
â Karan Khanna
Apr 17 at 12:53
a1, b1 and c1 are not required. you can directly use 2a -b ..
â Karan Khanna
Apr 17 at 12:54
a1, b1 and c1 are not required. you can directly use 2a -b ..
â Karan Khanna
Apr 17 at 12:54
@Blindman67 please share your failing input. I have added more working examples.
â Shubhendu Vaid
Apr 17 at 12:54
@Blindman67 please share your failing input. I have added more working examples.
â Shubhendu Vaid
Apr 17 at 12:54
1
1
instead of return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); use return Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1));
â Karan Khanna
Apr 17 at 12:55
instead of return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); use return Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1));
â Karan Khanna
Apr 17 at 12:55
 |Â
show 3 more comments
3 Answers
3
active
oldest
votes
up vote
2
down vote
accepted
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.
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
up vote
4
down vote
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
maybeparseEquation
getXList
mabesolveQuadratic
list
maybecoefficients
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.
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:17
add a comment |Â
up vote
2
down vote
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!
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
2
down vote
accepted
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.
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
up vote
2
down vote
accepted
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.
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
up vote
2
down vote
accepted
up vote
2
down vote
accepted
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.
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.
edited Apr 17 at 21:00
answered Apr 17 at 19:45
Isac
494210
494210
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
up vote
4
down vote
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
maybeparseEquation
getXList
mabesolveQuadratic
list
maybecoefficients
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.
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:17
add a comment |Â
up vote
4
down vote
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
maybeparseEquation
getXList
mabesolveQuadratic
list
maybecoefficients
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.
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:17
add a comment |Â
up vote
4
down vote
up vote
4
down vote
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
maybeparseEquation
getXList
mabesolveQuadratic
list
maybecoefficients
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.
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
maybeparseEquation
getXList
mabesolveQuadratic
list
maybecoefficients
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.
edited Apr 17 at 13:33
answered Apr 17 at 13:26
Blindman67
5,3611320
5,3611320
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:17
add a comment |Â
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:17
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:17
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:17
add a comment |Â
up vote
2
down vote
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!
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
up vote
2
down vote
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!
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
up vote
2
down vote
up vote
2
down vote
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!
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!
edited Apr 18 at 20:49
answered Apr 17 at 22:34
Snowbody
7,7271343
7,7271343
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
Please check the new program here : codereview.stackexchange.com/questions/192457/â¦
â Shubhendu Vaid
Apr 19 at 12:19
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f192285%2fquadratic-equation-solver-in-javascript%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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.
â Blindman67
Apr 17 at 12:46
Probably instead of using split you can use regular expression to get the values of a b and c
â Karan Khanna
Apr 17 at 12:53
a1, b1 and c1 are not required. you can directly use 2a -b ..
â Karan Khanna
Apr 17 at 12:54
@Blindman67 please share your failing input. I have added more working examples.
â Shubhendu Vaid
Apr 17 at 12:54
1
instead of return x = Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1)); use return Math.round((sign === '+') ? ((b1 + c1) / a1) : ((b1 - c1) / a1));
â Karan Khanna
Apr 17 at 12:55