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

Duplicated code in control panels #97

Closed
samreid opened this issue Oct 29, 2018 · 1 comment
Closed

Duplicated code in control panels #97

samreid opened this issue Oct 29, 2018 · 1 comment
Assignees

Comments

@samreid
Copy link
Member

samreid commented Oct 29, 2018

Discovered during #14, I saw similar code duplicated between StandardFormGraphControlPanel.js and VertexFormGraphControlPanel.js. Here is a graphical depiction of the similarity:

image

I'm sure it was easier to develop as a separate copy (especially while the simulation was still under design), but now that we are preparing the simulation for long-term maintainability, perhaps it would be better to unify. For example, if we change the tandem for equationsCheckbox, it would need to be done in 4 places. Or if we decide that the axisOfSymmetryCheckbox should precede the vertexCheckbox (for all screens), that would need to be done in 3 places. Yes, the current implementation has the flexibility that if we need to put the vertexCheckbox before on one screen and after on another screen it would be very easy, but it seems much more likely that we would want to constrain this layout by design. If we end up deciding to leave the duplicated code, then I recommend to put comments at each site of duplication that indicate what the duplication is, and what other code likely needs to remain synchronized with the code in question.

Note that I'm not advocating to always eliminate 100% of duplicated code (in some cases it is appropriate to duplicate code), but just to eliminate duplication in places where it is straightforward and beneficial to eliminate. For instance, if there was a file js/common/view/GQControlPanel.js which had an option showRootsCheckbox, it could easily accommodate both of these use cases and productively constrain them to have the same behavior.

@pixelzoom
Copy link
Contributor

I mentioned this in #55 (comment), and we've had this specific discussion about what is appropriate to duplicate in this sim. As the responsible developer, I've decided to prefer clarity/maintainability at the expense of some duplication that was a non-issue during implementation (and actually made it easier to respond to change requests). So I'm not going to change this. 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