New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion for v4: refactor OperatorNode to have explicit number of arguments. #1025

Closed
harrysarson opened this Issue Jan 23, 2018 · 17 comments

Comments

Projects
None yet
3 participants
@harrysarson
Collaborator

harrysarson commented Jan 23, 2018

There have been a number of issues (#1013, #1014, #1002) related to the handling of the number of arguments of OperatorNodes.

I would like to suggest removing the OperatorNode class and replace it by UnaryOperatorNode and BinaryOperatorNode classes (a breaking change for v4 #682).

This way users cannot shoot them selves in the foot by doing new OperatorNode('subtract', '-', [ /* more than two values*/]).

Having add and multiply supporting more than two arguments is a good feature but I think it should be implemented without using OperatorNodes.

The API would be something like:

new UnaryOperatorNode(op, fn, value);

new BinaryOperatorNode(op, fn, leftHandSide, rightHandSide);

I don't know who implicit multiplication should be handled.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 23, 2018

Great idea, thanks for the suggestion!

As for the add and multiply: maybe we should we create a MultiOperatorNode which is basically the current OperatorNode?

@josdejong josdejong referenced this issue Jan 23, 2018

Closed

Breaking changes for v4 #682

13 of 13 tasks complete
@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jan 23, 2018

Is there a use case for MultiOperatorNode? Would nested BinaryOperatorNode's or a FunctionNode suffice

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Jan 23, 2018

For reference: #739

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 24, 2018

@firepick1 do you have any thoughts on this? Would having two or three separate classes for operators make life easier in math.simplify or harder?

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 27, 2018

@harrysarson thanks for this great start of implementing UnaryOperatorNode in #1027.

I was just thinking about the pros/cons of splitting in two classes (UnaryOperatorNode, OperatorNode) or three classes (UnaryOperatorNode, BinaryOperatorNode, OperatorNode). Less classes is generally better: the more classes you have, the more work it is to handle all of them everywhere. But unary operators are really a special case. And if we go this route, I think it's best to go for the three options, unary, binary, and n-ary. The first two (unary and binary) are very common, and the third one not so much, but it's important to distinguish them.

From a practical point of view: these three classes will be 90% the same, so it would be a waste of code to simply copy everything three times. I think we can extract methods like node.toString(options) in separate, static functions like operatorNodeToString(op, args, options), so we can reuse most of the code ofthe three classes.

I was just thinking in a completely different direction: maybe we could keep a single class OperatorNode, and extend it with properties node.isUnary and node.isBinary? How would that work out?

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jan 28, 2018

Maybe isUnary and isBinary methods are the way to go, it would certainly require a smaller change to the code base which is always a good thing.

I guess the questions I have are:

  • How to we find a good solution to #1014? Being able to do new OperatorNode('-', 'unaryMinus', [node1, node2]) without causing an Error to be thrown I think could be at the root of this behaviour.
  • Can an OperatorNode change from unary to binary by someone doing node.args.push(newNode)?

As a side point, my new UnaryOperatorNode.js file was about half the size of the old OperatorNode.js file as I was able to remove a lot of the code, especially in the stringificaiton methods, which handled cases where there were more than one argument.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 28, 2018

These are good points. About your questions:

  • I think this issue holds for both solutions. You could give an error in both cases: when a user creates a unaryMinus operator either by accidentally passing more than two arguments, or by accidentally putting it into an BinaryOperatorNode or MultiOperatorNode. I think though that in the case of three explicit classes it's less probable that a programmer makes such a mistake.
  • I indeed think that such a method isUnary() and isBinary() should indeed automatically change their output when the number of arguments are changed. This is an actual case in for example simplify, when replacing 0 - x with -x for example.

Here an overview with pros/cons of both approaches that pop in my mind:

Solution 1: Three classes (UnaryOperatorNode, BinaryOperatorNode, MultiOperatorNode)

pros:

  1. It's really explicit and unambiguous what an operator node is about, you cannot accidentally take a unary operator node for a binary one. This helps making the code more robust and less prone to bugs that we've suffered from.

Solution 2: One class (OperatorNode) with extra methods (.isUnary() and .isBinary())

pros:

  1. API remains concise and simple, easier to work with.
  2. Code that does not need to distinguish unary/binary/multi arguments does not suffer from having to deal with 3 classes.
  3. Code remains concise (just one class). When having three operator node classes, you have to take care of these three cases everywhere you work with operator nodes, I'm afraid that will lead to quite some extra code, which brings more risk for bugs.
@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jan 28, 2018

Is it possible to validate iin the constructor of an OperatorNode, so that the number of args has to match the function that will be called when the node is evaluated. i.e. it is impossible to create an unaryMinus node with two arguments or a minus node with one or three arguments.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 28, 2018

Yes, so that's solvable in both solutions.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jan 28, 2018

At the moment though you can (and the tests do) new Operator('*', 'foo', [ ... ]) this would fail validation.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 28, 2018

There are currently no checks in place to warn you about "infeasible" combinations. We could add these if needed, but I'm not sure whether we should: it will couple the parser to function implementations. Right now these are completely separate, which makes usage very flexible. The only valid combination for * is new Operator('*', 'multiply', [ ... ]), but if you restrict to that, you wouldn't allow users to import and use a function fastMultiply or specialMultiply or whatever. The parser can't know...

If going for Solution 2, we could look into creating a few different factory functions to create an OperatorNode, to help not making a mistake there. Like createUnaryOperatorNode(op, fn, arg) and createBinaryOperatorNode(op, fn, lhs, rhs), both returning an OperatorNode. If we go for Solution 1, this we be solved already because we have three different classes.

What is your feeling about Solution 1 vs Solution 2? I have a slight preference for solution Solution 2, mainly because Solution 1 feels a bit overengineered, I'm afraid it will get in the way instead of helping us not make mistakes with OperatorNode.

EDIT: I don't think we should aim in trying to create a 100% fool proof solution, but we should make it unobvious to make a mistake, and make mistakes easy to spot.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jan 30, 2018

If we can use Solution 2 to fix #1014 then it has my full backing

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 31, 2018

Yes, we can indeed use it to solve #1014. Ok let's go for Solution 2 then, thanks for all your input!

At all the places where we work with OperatorNode, we will have to check whether the OperatorNode is binary. Basically the same as checking whether node.args.length === 2. We do have to go through derivative, simplify, and rationalize thoroughly and add these checks where missing, but that's easy to spot.

I will give this approach a try right now, see where we end up.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 31, 2018

I've worked out the solution with isUnary() and isBinary(), I quite like the result, though there is still room for another refactoring step. @harrysarson would be great if you could have a look at #1032.

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Jan 31, 2018

I will have a look later today :)

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 31, 2018

thanks!

@josdejong

This comment has been minimized.

Owner

josdejong commented Feb 1, 2018

This has been implemented via #1032.

@josdejong josdejong closed this Feb 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment