Quadratic equation solver in JavaScript

The name of the pictureThe name of the pictureThe name of the pictureClash 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.







share|improve this question





















  • 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
















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.







share|improve this question





















  • 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












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.







share|improve this question













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.









share|improve this question












share|improve this question




share|improve this question








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
















  • 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










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.






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:19

















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 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.






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:17

















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!






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:19










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















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






























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.






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:19














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.






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:19












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.






share|improve this answer















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.







share|improve this answer















share|improve this answer



share|improve this answer








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
















  • 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












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 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.






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:17














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 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.






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:17












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 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.






share|improve this answer















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.







share|improve this answer















share|improve this answer



share|improve this answer








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
















  • 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










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!






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:19














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!






share|improve this answer























  • Please check the new program here : codereview.stackexchange.com/questions/192457/…
    – Shubhendu Vaid
    Apr 19 at 12:19












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!






share|improve this answer















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!







share|improve this answer















share|improve this answer



share|improve this answer








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
















  • 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












 

draft saved


draft discarded


























 


draft saved


draft discarded














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













































































Popular posts from this blog

Chat program with C++ and SFML

Function to Return a JSON Like Objects Using VBA Collections and Arrays

Will my employers contract hold up in court?