Fix order dependence on cancel like terms #237
Conversation
|
... |
|
hey sorry for letting this slide! I've been prioritizing a lot less time on mathsteps lately next week I'm gonna have a lot of free time and I'll take a look :) |
There was a problem hiding this comment.
Hi!! Thanks for your patience :)
And thanks for writing such clean code - I haven't really looked at this project much in the past year and was still able to understand everything and have opinions on it, which was really nice.
I put a lot of comments about little style things, which I wish our linter covered so sorry about that. I'd also like to see more comments - mathsteps code can be tricky to understand, so comments and examples help a ton.
Big thing I need from you: TESTS! For any kind of expression that would now get different steps, please add tests for it. If you need help figuring out where/how to do this just lemme know.
Feel free to ask questions or debate any of my comments - they're opinions I hold but they aren't set in stone.
Thanks for contributing to mathsteps! 😊
| @@ -19,7 +19,8 @@ function addConstantFractions(node) { | |||
| if (!Node.Type.isOperator(node) || node.op !== '+') { | |||
| return Node.Status.noChange(node); | |||
| } | |||
| if (!node.args.every(n => Node.Type.isIntegerFraction(n, true))) { | |||
| if (!node.args.every(n => Node.Type.isIntegerFraction(n, true) || | |||
| hasIntegerMultiplicationDenominator(n))) { | |||
There was a problem hiding this comment.
for readability, can you put Node.Type.isIntegerFraction(n, true) || hasIntegerMultiplicationDenominator(n) on the second line together? at first I read it as !node.args.every(n => Node.Type.isIntegerFraction(n, true)) || hasIntegerMultiplicationDenominator(n) (which I know doesn't make sense :p)
and similar for other things below - when you write something longer than a line, can you do the linebreak at the beginning of a parenthesis instead of in the middle of it? I'm pretty sure there's a style guide out there somewhere that better explains what I mean, but I can't find it. Let me know if how I described this is confusing
| @@ -47,6 +48,16 @@ function addConstantFractions(node) { | |||
| newNode = Node.Status.resetChangeGroups(status.newNode); | |||
| } | |||
|
|
|||
| const newDenominators = newNode.args.map(fraction => { | |||
There was a problem hiding this comment.
Why did you choose to put this between steps 1B and 2A? I'm also wondering if steps 1A and 1B might break if the denominators aren't integers yet but instead still multiplication.
My intuition is to move it before we even start any of the adding fraction logic - but you've thought more about this problem than me so I'm curious what you think!
There was a problem hiding this comment.
also wherever it goes, adding a comment before it explaining what the step is for would be very helpful :)
| const newDenominators = newNode.args.map(fraction => { | ||
| return fraction.args[1]; | ||
| }); | ||
| if (!newDenominators.every(denominator => hasEqualNumberOfArgs(denominator, newDenominators[0]))){ |
There was a problem hiding this comment.
I'm curious why you do this check (would be useful as a comment for other people who are also curious!)
There was a problem hiding this comment.
also why do you call it newDenominators? does it get mutated?
| @@ -136,15 +147,15 @@ function makeCommonDenominator(node) { | |||
| if (missingFactor !== 1) { | |||
| const missingFactorNode = Node.Creator.constant(missingFactor); | |||
| const newNumerator = Node.Creator.parenthesis( | |||
| Node.Creator.operator('*', [child.args[0], missingFactorNode])); | |||
| Node.Creator.operator('*', [child.args[0], missingFactorNode])); | |||
There was a problem hiding this comment.
please keep it 2 space indentation :)
(same for all the other 4 space indentation you did below)
| @@ -169,4 +180,41 @@ function evaluateNumerators(node) { | |||
| ChangeTypes.MULTIPLY_NUMERATORS, node, newNode); | |||
| } | |||
|
|
|||
| function hasIntegerMultiplicationDenominator(node) { | |||
There was a problem hiding this comment.
these two functions are so readable 👌
| @@ -125,6 +127,43 @@ function cancelLikeTerms(node) { | |||
| numerator.content.args : numerator.args; | |||
| const denominatorArgs = Node.Type.isParenthesis(denominator) ? | |||
| denominator.content.args : denominator.args; | |||
|
|
|||
| const likeTerms = getIndicesOfFirstTwoLikeTerms(numeratorArgs, denominatorArgs); | |||
| if (likeTerms){ | |||
There was a problem hiding this comment.
this seems like a distinct case from the code below - as in, either this code block will execute or the one below it will
to make that more clear, what do you think about adding a comment explaining this case, and then another comment for the case below?
maybe this one can be 4A the one below 4B, since they're both a subset of 4
| if (likeTerms){ | ||
| const cancelStatus = cancelTerms(numeratorArgs[likeTerms.numeratorIndex], | ||
| denominatorArgs[likeTerms.denominatorIndex]); | ||
| if (cancelStatus.hasChanged) { |
There was a problem hiding this comment.
oh woops the distinct case is from here on
but wait, in what case would it not cancel if there were indeed like terms?
| if (cancelStatus.numerator) { | ||
| numeratorArgs[likeTerms.numeratorIndex] = cancelStatus.numerator; | ||
| } | ||
| // if the cancelling out got rid of the numerator node, we remove it |
There was a problem hiding this comment.
ah man, I'm pretty sad this logic is mostly copy pasted 3 times in this file now :( but I remember last time I copied it, I couldn't figure out how to cleanly separate out this logic
I'll take a look again in a bit to think about it again, though feel free to take a stab at it first
| @@ -43,6 +43,12 @@ function divideByGCD(fraction) { | |||
| return Node.Status.noChange(fraction); | |||
| } | |||
|
|
|||
| if (numeratorValue === denominatorValue) { | |||
There was a problem hiding this comment.
👌 nice
I'm assuming that before, for this case it still worked out but the steps looked really weird? just wanna make sure this code block is necessary
| @@ -68,15 +74,18 @@ function findGCD(node, gcd, numeratorValue, denominatorValue) { | |||
| const gcdNode = Node.Creator.constant(gcd); | |||
| gcdNode.changeGroup = 1; | |||
|
|
|||
| const intermediateNumerator = Node.Creator.parenthesis(Node.Creator.operator( | |||
| '*', [Node.Creator.constant(numeratorValue/gcd), gcdNode])); | |||
| let intermediateNumerator = Node.Creator.constant(numeratorValue); | |||
There was a problem hiding this comment.
add a comment/example here (similar to the ones in other places in this file)
There was a problem hiding this comment.
I also think it's more clear if you do let intermediateNumerator;
and
else {
intermediateNumerator = Node.Creator.constant(numeratorValue);
}
There was a problem hiding this comment.
and also yayy thanks for fixing this case, it was pretty gross/weird before how it added the 1
-fixed issue #232
-changed such that in the expression 5 / 128 + 32 / (32 * 4) the denominator in the second fractions is the first to be evaluated since both denominators are equal.