useReducer in calculator #1
Conversation
|
Commenting in revery-ui#174 (comment) 🙂 |
|
This looks really good to me. I'll review this as soon as I'm done with lunch |
examples/Calculator.re
Outdated
| } | ||
| | ClearKeyPressed(ac) => | ||
| ac ? | ||
| {...state, operator: `Nop, result: 0., display: ""} : |
There was a problem hiding this comment.
Shouldn't this also clear the number field? Otherwise, this'll only clear the current equation but leave the current value "typed in"
There was a problem hiding this comment.
Oh right, great catch! I thought the existing statement was an if/else but the reset of curNum was always happening. Fixed in 240b993. 👍
| }; | ||
| setCurNum(""); | ||
| }; | ||
| let eval = (state, newOp) => { |
There was a problem hiding this comment.
Now that we have the reducer, maybe we can refactor eval into an action? Thoughts on this?
There was a problem hiding this comment.
I think eval wouldn't really fit an action-like role. My mental model for actions is "actions are like newspapers", in the sense that actions need to tell you something that happened in your app (generally user originated, or network originated). This talk has a really great overview on what are good candidates for actions: https://youtu.be/Dm9NgjR5Jn4?t=485
eval would fit more a "command" role. Because multiple actions (in this case 2) need to perform the evaluation, I think it makes sense to keep eval as just a helper function that encapsulates the common logic in the two actions reducers. There are plenty of ways to solve this though 😄 Do you think we should rethink the way eval and the current actions fit together?
There was a problem hiding this comment.
One rule of thumb for actions is to try to use the past tense to model them. So things like KeyPressed or NetworkMessageReceived would be good candidates for actions, but ProcessQueue or CalculateDeviation would not be as good (they are more "command like" and probably need to live in the reducer as helper functions). I hope that makes sense!
|
Thanks @OhadRau for the quick review! I fixed the issue with |
|
Great! Merging this now |
Test using
useReducerin calculator