nicolewhite / algebra.js

Build, display, and solve algebraic equations.
http://algebra.js.org
MIT License
1.34k stars 111 forks source link

Fraction multiplied by constant #81

Open lucasgruwez opened 7 years ago

lucasgruwez commented 7 years ago

Hi @nicolewhite,

I noticed that when using algebra.js, a fraction multiplied by a constant is displayed like so:

var expr = algebra.parse("x * 2/5")
console.log(expr.toString())
> 2/5x

That could be misinterpreted as 2/(5x). I think it would be more logical to display it as 2x/5, which is clearer.

Thank you for creating such a great library by the way!

Lucas

Benjadahl commented 7 years ago

Hey Lucas, I have created a mockup solution for this, however I am not sure of its quality or if it breaks anything else in the project. As many of the unit tests are based on the term.prototype.toString() function, which is the one I have changed. I do not really have time to go through all of the tests myself.

The change is made in my commit 450dba3f4a62d99d8502d50c6a5b06fda3812691.

The code works by checking if it is the last coefficient, if it is, it will run the block of code, that inserted the variables after the coef before, however it will run this in between numerator and denominator.

Not sure how @nicolewhite feels about this? It does not represent how the internals of the library works as well as before, however it might be more userfriendly.

A quick solution for you @lucasgruwez might be to disable implicit multiplication in the toString function? Else I am able to provide you with a built version of my fix, if you want to use it.

lucasgruwez commented 7 years ago

Hi @benjadahl,

I have looked at how ASCII math would render 2/5x, and it renders it in a way that is hard to misinterpret, that looks like this:

 2
--- x
 5

And it renders 2x/5 as something more like:

   x
2 ---
   5

I think that we can easily agree that the first render is more elegant, therefore, I think it should be an option, rather than a feature.

I will have a look at your code and check how it performs.

Thank you for making that fix.

Lucas

nicolewhite commented 7 years ago

I think this does make sense but it is a large breaking change, so would need to be released under a new major version, unfortunately. I will think about it more.

lucasgruwez commented 7 years ago

@nicolewhite,

Thanks for the response, as I said before, you could add this as an option, so that existing code doesn't change the output

// Example of how this could be done

expr.toString(true) // 2x/5
expr.toString()        // 2/5x

Therefore, you might not need a major release, but a minor release could do.

Lucas