That's why I said effectively global. It's in a closure but multiple parties are changing it, so it suffers from most of the problems that a true global does.
You could do something like
state = getStateWithNewTime(state);
And replace the state object with a new object when you need to mutate it. The outer scope still holds the state in a variable but the functions don't ever access it directly. Instead they take in the current state as a parameter and return a new state (important to still not mutate the parameter but instead return a new state object.. the spread operator makes this dead simple).
I understand that immutability has benefits and overall is a good practice, but in here I don't see any reason to force it. With getStateWithNewTime I still have a function that exists just to modify the time, still use the state in the closure and now I have to remember to replace the old state in every single place.
For a second I want it to use a central store and put the mutations in there, but with just three variables in the state I could not justify to myself doing that.
Here's an example. It's not perfect, but I think it's much easier to follow and roughly the same number of lines of code.
The main differences are:
The view (DOM updates) are separated from the logic. Instead of having a utility function to update each property of the state and updating the view in the same function, this updates the whole state in one step and updates the view in a separate step. The timer is also separated from the state.
The state object is only mutated in two places - nextWord() and in the interval callback. This makes it much easier to track changes to the state object. In your code I counted at least 7 different places where the state object gets mutated
Removed the Num class as it doesn't add any benefit - there are already numbers and you can already add, subtract, and retrieve them
You can extract everything above the game() function into separate files and easily write unit tests for the functions because they're pure. Testing the View class would require mocks/stubs because DOM interaction is an integration point. Everything left inside the game() function is an integration between the state/logic, view, and timer so you wouldn't write unit tests for that code.
I wouldn't call this "forcing" immutability. It is easier to grok, easier to follow, easier to debug, and easier to test. It follows separation of concerns and limits shared state to only the points of integration.
I wouldn't call this "forcing" immutability. It is easier to grok, easier to follow, easier to debug, and easier to test. It follows separation of concerns and limits shared state to only the points of integration.
You did refactor the entire thing. If the point you were trying to make was about immutability, I didn't get it. Using tecniques to make your data immutable in a messy code, still leaves you with a messy code.
If this was about separation of concerns and code organization, you should have said it from the beginning, I would have agreed with you.
The getStateWithNewWord() function gets your new state without mutating it and without calling 3 separate functions to update the state. It is much easier to see how the state is changing this way. That was the main point. Instead of 7 state mutations, it mostly happens in one place.
You're right, simply refactoring the state mutations into a single function left the code messy, hence the larger refactor. Separation of concerns goes hand in hand with techniques like immutability because it keeps the state change where it belongs.
I didn't mean to come off as negative in any way so apologies if it seems that way.
Your original question was whether your code is "procedural". I'd say it would be considered "spaghetti" due to the lack of separation of concerns.
I assumed the question was due to the negative connotation around procedural code. My refactor is procedural but I don't necessarily consider that a bad thing. It still follows good programming practice and gets the job done.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
"Global" in the
gamescope, but not really global global.I could, but I don't know exactly where should I store the new state.
That's why I said effectively global. It's in a closure but multiple parties are changing it, so it suffers from most of the problems that a true global does.
You could do something like
And replace the state object with a new object when you need to mutate it. The outer scope still holds the state in a variable but the functions don't ever access it directly. Instead they take in the current state as a parameter and return a new state (important to still not mutate the parameter but instead return a new state object.. the spread operator makes this dead simple).
I understand that immutability has benefits and overall is a good practice, but in here I don't see any reason to force it. With
getStateWithNewTimeI still have a function that exists just to modify the time, still use thestatein the closure and now I have to remember to replace the old state in every single place.For a second I want it to use a central store and put the mutations in there, but with just three variables in the state I could not justify to myself doing that.
Here's an example. It's not perfect, but I think it's much easier to follow and roughly the same number of lines of code.
The main differences are:
stateobject is only mutated in two places -nextWord()and in the interval callback. This makes it much easier to track changes to the state object. In your code I counted at least 7 different places where the state object gets mutatedNumclass as it doesn't add any benefit - there are already numbers and you can already add, subtract, and retrieve themYou can extract everything above the
game()function into separate files and easily write unit tests for the functions because they're pure. Testing theViewclass would require mocks/stubs because DOM interaction is an integration point. Everything left inside thegame()function is an integration between the state/logic, view, and timer so you wouldn't write unit tests for that code.I wouldn't call this "forcing" immutability. It is easier to grok, easier to follow, easier to debug, and easier to test. It follows separation of concerns and limits shared state to only the points of integration.
You did refactor the entire thing. If the point you were trying to make was about immutability, I didn't get it. Using tecniques to make your data immutable in a messy code, still leaves you with a messy code.
If this was about separation of concerns and code organization, you should have said it from the beginning, I would have agreed with you.
The
getStateWithNewWord()function gets your new state without mutating it and without calling 3 separate functions to update the state. It is much easier to see how the state is changing this way. That was the main point. Instead of 7 state mutations, it mostly happens in one place.You're right, simply refactoring the state mutations into a single function left the code messy, hence the larger refactor. Separation of concerns goes hand in hand with techniques like immutability because it keeps the state change where it belongs.
I didn't mean to come off as negative in any way so apologies if it seems that way.
Your original question was whether your code is "procedural". I'd say it would be considered "spaghetti" due to the lack of separation of concerns.
I assumed the question was due to the negative connotation around procedural code. My refactor is procedural but I don't necessarily consider that a bad thing. It still follows good programming practice and gets the job done.