Skip to content
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

unused dispose functions #158

Closed
pixelzoom opened this issue Aug 29, 2019 · 2 comments
Closed

unused dispose functions #158

pixelzoom opened this issue Aug 29, 2019 · 2 comments

Comments

@pixelzoom
Copy link
Contributor

Related to this item for code review #143:

  • Do all types that require a dispose function have one? This should expose a public dispose function that calls this.disposeMyType(), where disposeMyType is a private function declared in the constructor. MyType should exactly match the filename.

Implementing dispose functions that are not used is also a problem. It's unexercised code, and it confuses future maintainers about what really needs to be disposed. That said...

I see implementations of dispose for BarometerNode, BucketNode, EquationNode, PointNode. But the only call to dispose that I see being used is for PointNode, in BucketNode:

        const removalListener = removedPoint => {
          if ( removedPoint === addedPoint ) {
            this.removeChild( pointNode );
            pointNode.dispose();
            points.removeItemRemovedListener( removalListener );
          }
        };

If the dispose implementations for BarometerNode, BucketNode, and EquationNode are really not needed, then I recommend deleting them.

I also recommend putting a note in implementation-notes.md about what needs to be disposed any why.

@SaurabhTotey
Copy link
Member

I removed the dispose functions for BarometerNode and EquationNode, but I was unable to find BucketNode's dispose. Did I miss something, or is this all good now?

@pixelzoom
Copy link
Contributor Author

Looks like you got them all @SaurabhTotey. I must've been hallucinating about BucketNode, or might have been looking at FRACTIONS_COMMON/BucketNode.

Closing.

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

No branches or pull requests

2 participants