You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This issue consists of questions and suggestions concerning the architecture of Shower.
Question
The division of roles between Container and Player isn't entirely clear to me; where do the responsibilities of the one stop and where to those of the other start?
From what I see, Container is responsible for:
performing the transition from list mode to slide mode and back
correctly (re-)sizing the slide
reacting to key presses to transition into slide mode and back
Player is responsible for:
advancing slides
when slide mode is activated, choosing the starting slide
reacting to key presses for advancing slides
In other words: Container handles list and slide mode and changing from one to the other, whereas Player handles transitions from one slide to another. Is this correct?
Who is supposed to call methods from Container and Player? Can they call each other? Are they aware of each other?
Suggestions
Can the division of functionalities be made clear? There are brief comments about this in the source code, but they are not obvious. (I could help out here.)
Are dependencies between Container and Player desired? Should they know about each other's existence at all? In particular, I noted the following dependencies, which I think should be considered for removal:
In order to know whether slide mode is enabled, Player needs to ask Container (introduced in Spacebar should scroll in list mode core#29)
To fix this point, we probably need to move that switch somewhere else.
Can we avoid passing the Shower object to Container and Player? The reason that Container and Player can access each other in the first place, is because the Shower object is passed to them. Is this the desired situation?
Container needs Shower#options, Shower#events, and Shower#isHotkeysEnabled. Can't we pass options and events directly to the constructor? The hotkeys setting could be part of the options.
Player needs Shower#events, and slide methods such as getSlidesCount, .getSlideIndex, .get, isHotkeysEnabled. The events we can solve similarly. The slide access can be resolved by creating a separate component that represents a list of slides.
At the moment, it seems that Shower is a God object and also lacks a precise functionality description. Some refactoring would clear this up. At the very least, I believe that Shower#player and Shower#container should become private fields, and they should never reference each other. Components like Location should receive container and player as arguments rather than receiving the Shower object (see Law of Demeter).
I wonder if it makes sense to rename the components into something more specific, like SlideViewer for Container and SlideController or SlideAdvancer for Player.
Continuing this last remark, would it be possible to see Container as the view and Player as the controller (in the model-view-controller terminology)? And, by extension, a SlideDeck component that is to be extracted from Shower becomes the view? This seems like a logical architectural separation. The major change would then be that the mouse and key bindings code is removed from Container (and Player might need to become aware of Container, or they can be coupled loosely with events).
I'd be curious to hear your thoughts. I propose such a refactoring if you think it's a good idea. (Depends on #253 though.)
The text was updated successfully, but these errors were encountered:
@pepelsbey@zloylos I'd be willing to try this out to see what the resulting code looks like. (For a mergeable implementation, I'd prefer to have #253 sorted out first.)
This issue consists of questions and suggestions concerning the architecture of Shower.
Question
The division of roles between
Container
andPlayer
isn't entirely clear to me; where do the responsibilities of the one stop and where to those of the other start?From what I see,
Container
is responsible for:Player
is responsible for:In other words:
Container
handles list and slide mode and changing from one to the other, whereasPlayer
handles transitions from one slide to another. Is this correct?Who is supposed to call methods from
Container
andPlayer
? Can they call each other? Are they aware of each other?Suggestions
Container
andPlayer
desired? Should they know about each other's existence at all? In particular, I noted the following dependencies, which I think should be considered for removal:Container
calls a function onPlayer
(fixed in Remove dependencies between container and player core#30)Player
explicitly mentionsContainer
(fixed in Remove dependencies between container and player core#30)Player
needs to askContainer
(introduced in Spacebar should scroll in list mode core#29)To fix this point, we probably need to move that switch somewhere else.
Shower
object toContainer
andPlayer
? The reason thatContainer
andPlayer
can access each other in the first place, is because theShower
object is passed to them. Is this the desired situation?Container
needsShower#options
,Shower#events
, andShower#isHotkeysEnabled
. Can't we pass options and events directly to the constructor? The hotkeys setting could be part of the options.Player
needsShower#events
, and slide methods such asgetSlidesCount
,.getSlideIndex
,.get
,isHotkeysEnabled
. The events we can solve similarly. The slide access can be resolved by creating a separate component that represents a list of slides.Shower
is a God object and also lacks a precise functionality description. Some refactoring would clear this up. At the very least, I believe thatShower#player
andShower#container
should become private fields, and they should never reference each other. Components likeLocation
should receivecontainer
andplayer
as arguments rather than receiving theShower
object (see Law of Demeter).SlideViewer
forContainer
andSlideController
orSlideAdvancer
forPlayer
.Container
as the view andPlayer
as the controller (in the model-view-controller terminology)? And, by extension, aSlideDeck
component that is to be extracted fromShower
becomes the view? This seems like a logical architectural separation. The major change would then be that the mouse and key bindings code is removed fromContainer
(andPlayer
might need to become aware ofContainer
, or they can be coupled loosely with events).I'd be curious to hear your thoughts. I propose such a refactoring if you think it's a good idea. (Depends on #253 though.)
The text was updated successfully, but these errors were encountered: