search

mordrax / cotwelm

Castle of the Winds (A Remake in Elm)
http://game.castleofthewinds.com
103 stars 11 forks source link

exit button in inventory screen #5

Closed djuretic closed 5 years ago

djuretic commented 8 years ago

Screenshot: shopexit

djuretic commented 7 years ago

I've pushed a new commit (thanks a lot @JulianLeviston for your guidance), feedback is welcome.

mordrax commented 7 years ago

The refactored version still has a problem that: The child module is the one that has the functionality but the parent module's msg is passed into the child to activate the functionality. This seems weird.

This may be controversial but my current preference is to go with the simplest approach possible for the specific problem. I did it in this way with CharCreation:

  1. The update function returns a tuple of the updated model and a single argument https://github.com/mordrax/cotwelm/blob/master/src/CharCreation/CharCreation.elm#L53
  2. Whoever calls it then just uses the returned tuple to decide what to do. https://github.com/mordrax/cotwelm/blob/master/src/main.elm#L114

This has the advantage that there is no coupling. The logic should be fairly obvious to anyone looking at it. And since there will only ever be two msgs (StartGame, CancelGame), the output will be a 3-tuple.

I guess it's controversial because we're always looking for a good pattern or abstraction to a problem and this feels like it's a hardcoded hack that does not scale. But listening and thinking about what Richard Feldman says, I like his idea of solving the specific problem we have at hand rather than abstracting to solve a generalised architectural problem and then applying it to this simple case.

JulianLeviston commented 7 years ago

Sorry if I led you up the garden path a bit here. This solution is quite a bit more complex than what I was trying to encourage.

It might be worth looking at my toy todo example to understand what I meant more clearly. I think that encapsulates how to connect multi-level functions together (where one elm architecture is within another): https://github.com/GetContented/todo in particular https://github.com/GetContented/todo/blob/master/todo/tasksview.elm

I personally think this is quite a bit cleaner and simpler than passing tuples of data around (which seems to be a lot like just adding a conditional to me), but if you both don't think it's simpler, best not to use it. Nothing should substitute for one's ability to understand the code one writes, even if that means writing less efficient and more verbose code for a while.