-
Notifications
You must be signed in to change notification settings - Fork 282
#76 - Better break-up fraction. #158
base: master
Are you sure you want to change the base?
Changes from 1 commit
aee08a1
6135dad
322a247
3c1cac0
1ccdc3b
a46849d
fd8ebf0
f779651
41e9223
308a90c
d88605f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| const canFindDenominatorInNumerator = require('../../checks/canFindDenominatorInNumerator'); | ||
| const ChangeTypes = require('../../ChangeTypes'); | ||
| const Node = require('../../node'); | ||
| const TreeSearch = require('../../TreeSearch'); | ||
|
|
@@ -27,8 +28,33 @@ function breakUpNumerator(node) { | |
| // At this point, we know that node is a fraction and its numerator is a sum | ||
| // of terms that can't be collected or combined, so we should break it up. | ||
| const fractionList = []; | ||
| const denominator = node.args[1]; | ||
| numerator.args.forEach(arg => { | ||
| let denominator = node.args[1]; | ||
|
|
||
| // Check if we can add a constant to make the fraction nicer | ||
| // fraction e.g. (2+x)/(5+x) -> (5+x)/(5+x) + 3/(5+x) | ||
| if (canFindDenominatorInNumerator(node)) { | ||
| let denominatorParenRemoved = false; | ||
| if (Node.Type.isParenthesis(denominator)) { | ||
| denominatorParenRemoved = true; | ||
| denominator = denominator.content; | ||
| } | ||
| const newNumerator = []; | ||
|
|
||
| // The constant value difference between the numerator and the denominator | ||
| const numeratorConstant = parseInt(numerator.args[1].value); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you ever make sure that this is actually a constant - do a check either here or in |
||
| const denominatorConstant = parseInt(denominator.args[1].value); | ||
| const addedConstant = numeratorConstant - denominatorConstant; | ||
|
|
||
| newNumerator.push(Node.Creator.constant(addedConstant)); | ||
| newNumerator.push(denominator); | ||
|
|
||
| numerator = newNumerator; | ||
|
|
||
| if (denominatorParenRemoved) { | ||
| denominator = Node.Creator.parenthesis(denominator); | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be best if we can break this up into two steps. the step you added, and the break up fraction that comes after (and then maybe we can group in the cancelling that comes after). and then we'd have substeps for breaking up the fraction. what do you think? (let me know if you want me to explain more what this means) |
||
| numerator.forEach(arg => { | ||
| const newFraction = Node.Creator.operator('/', [arg, denominator]); | ||
| newFraction.changeGroup = 1; | ||
| fractionList.push(newFraction); | ||
|
|
@@ -41,5 +67,4 @@ function breakUpNumerator(node) { | |
| return Node.Status.nodeChanged( | ||
| ChangeTypes.BREAK_UP_FRACTION, node, newNode, false); | ||
| } | ||
|
|
||
| module.exports = search; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,3 +29,10 @@ describe('canSimplifyPolynomialTerms addition', function() { | |
| ]; | ||
| tests.forEach(t => testCanCombine(t[0], t[1])); | ||
| }); | ||
|
|
||
| describe('canSimplifyPolynomialTerms denominator in numerator', function() { | ||
| const tests = [ | ||
| ['(2x + 3)/(2x + 2)', true], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a bunch more tests? especially a cases that should be false you can use some of my comments above for edge cases to test for too :) |
||
| ]; | ||
| tests.forEach(t => testCanCombine(t[0], t[1])); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ describe('breakUpNumerator', function() { | |
| ['(x+3+y)/3', '(x / 3 + 3/3 + y / 3)'], | ||
| ['(2+x)/4', '(2/4 + x / 4)'], | ||
| ['2(x+3)/3', '2 * (x / 3 + 3/3)'], | ||
| ['(2x + 3)/(2x + 2)', '(1 / (2x + 2) + (2x + 2) / (2x + 2))'], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here - try to think of edge cases. I can help you brainstorm if you dunno what to add |
||
| ]; | ||
| tests.forEach(t => testBreakUpNumeratorSearch(t[0], t[1])); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean - 3/ (5 + x) *.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, yeah haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so then you'd also want to say add/subtract ^_^