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

ElectronDensityNode and ElectrostaticPotentialNode are basically the same file #61

Closed
zepumph opened this issue Jul 18, 2017 · 3 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jul 18, 2017

from code review #30

When comparing the two files in intellij, it looks more like a commit diff than different files, there are about 10 or so changes. It seems like this could probably be combined into a "2DSurfaceNode" or something that both of these nodes can use with options. @pixelzoom please comment if you think this refactoring is not worth the effort.

To me it was a bit confusing to see two fully separate files for the same looking node (just different colors). It also isn't parallel to the general use "SurfaceColorKey.js" which creates a key for any of the surfaces.

@pixelzoom
Copy link
Contributor

Yeah, looks like there should be a SurfaceNode based type.

pixelzoom added a commit that referenced this issue Jul 19, 2017
@pixelzoom
Copy link
Contributor

I factored out SurfaceNode, an abstract base type, in the above commit. The subtypes (ElectronDensityNode and ElectrostaticPotentialNode) implement the updateFill method, which is the bit that differs for these 2 surface types.

@zepumph please review.

@zepumph
Copy link
Member Author

zepumph commented Jul 19, 2017

Looks great. Closing

@zepumph zepumph closed this as completed Jul 19, 2017
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