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

factor out subcomponents of WaveInterferenceControlPanel #283

Closed
pixelzoom opened this issue Dec 19, 2018 · 5 comments
Closed

factor out subcomponents of WaveInterferenceControlPanel #283

pixelzoom opened this issue Dec 19, 2018 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 19, 2018

Related to code review #259.

This was promoted to an issue after beginning its life as these REVIEW comments:

//REVIEW There are definitely similarities in this control panel for the 3 scenes. But the reuse of what's common in
// this implementation feels forced/complicated. I'd hate to have to maintain this.
//REVIEW* Isn't the alternative a significant amount of duplicated code?

The main problem here is that there are big chunks of "responsibility" that could be encapsulated, but are instead sprinkled throughout the WaveInterferenceControlPanel constructor. A few suggestions to make this panel more readable/maintainable:

(1) Factor out new class FrequencyControl with these responsibilities: creates the 3 frequency sliders (waterFrequencySlider, soundFrequencySlider, lightFrequencySlider) and their container; creates frequencyInHzProperty; creates and positions the "Frequency" title; toggles visibility of the sliders based on model.sceneProperty. waterFrequencySlider, soundFrequencySlider, lightFrequencySlider, frequencyInHzProperty, frequencySliderContainer and frequencyTitle can all be removed from WaveInterferenceControlPanel , and replaced with:

const frequencyControl = new FrequencyControl( model );

(2) Factor out new class AmplitudeControl with these responsibilities: creates the amplitude slider and its container; creates and positions the 'Amplitude' title. amplitudeSliderContainer and amplitudeTitle can be removed from WaveInterferenceControlPanel, and replaced with:

const amplitudeControl = new AmplitudeControl( model );

(3) Factor out new class SoundViewTypeRadioButtonGroup that creates the VerticalAquaRadioButtonGroup for selecting the SoundViewType. const sceneRadioButtonGroup = new RadioButtonGroup(...) becomes const sceneRadioButtonGroup = new SoundViewTypeRadioButtonGroup(...).

@pixelzoom pixelzoom changed the title WaveInterferenceControlPanel is overly complicated factor out subcomponents of WaveInterferenceControlPanel Dec 19, 2018
pixelzoom added a commit that referenced this issue Dec 19, 2018
pixelzoom added a commit that referenced this issue Dec 19, 2018
pixelzoom added a commit that referenced this issue Dec 19, 2018
@pixelzoom
Copy link
Contributor Author

There's a similar (and related) chunk of code in InferenceScreenView that could be factored out.

(4) Factor out new class SeparationControl with these responsibilities: creates the 3 separation NumberControls; toggles visibility of the NumberControls based on model.sceneProperty. const toggleNode = new ToggleNode would be replaced with const separationControl = new SeparationControl( model ).

@pixelzoom pixelzoom mentioned this issue Dec 19, 2018
pixelzoom added a commit that referenced this issue Dec 19, 2018
@pixelzoom
Copy link
Contributor Author

Note that decisions made in #262 may introduce the need for additional refactoring. But the goal is encapsulate subcomponents that have significant code in their own class files.

@pixelzoom
Copy link
Contributor Author

Another thought... Responsibility for "nestling the title text into the slider" might be best to handle in WaveInterferenceSlider, using the Decorator pattern. That would be preferable to handling the titles separately in WaveInterferenceControlPanel, or duplicating the "nestling" in the proposed FrequencyControl and AmplitudeControl.

samreid added a commit that referenced this issue Dec 21, 2018
samreid added a commit that referenced this issue Dec 28, 2018
samreid added a commit that referenced this issue Dec 29, 2018
samreid added a commit that referenced this issue Dec 29, 2018
@samreid
Copy link
Member

samreid commented Dec 29, 2018

Responsibility for "nestling the title text into the slider" might be best to handle in WaveInterferenceSlider, using the Decorator pattern.

Because FrequencySlider is used in lieu of WaveInterferenceSlider for the light scene, the title "nesting" should be implemented in a place it can be used by both the FrequencySlider and other WaveInterferenceSlider instances. In the preceding commits, I moved the titles to FrequencyControl and AmplitudeControl, and factored out the spacing logic in WaveInterferenceUtils.getSliderTitleSpacing.

I also factored out FrequencyControl, AmplitudeControl, SoundViewTypeRadioButton and SeparationControl as prescribed. Ready for review.

@pixelzoom
Copy link
Contributor Author

👍 Looks great, much easier to digest. 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