Refactor from a switch to an unordered map a.k.a javascript object.
Benefits:
- Cleaner code
- No break; necessary
- Each event type is his own function call
- Same performance
- Possible to change function event at runtime
Differential D1786
Refactor input.js nani on Mar 16 2019, 7:29 PM. Authored by Tags None Referenced Files
Details
Refactor from a switch to an unordered map a.k.a javascript object.
Everything should behave as before.
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions Cleanup
Fix
Comment Actions Cleanup
Stage
Now would be a matter of improving all the helper functions that the FSM calls. Comment Actions For now this patch moves:
Transforms it to:
Tries to follow #5387 and #2826 The fsm.js has been modified to be able to have a default function and "_" in state name Comment Actions
What uses the underscore?
Was it a lucky try? Because FeXoR is primarily a mapauthor, but I think he worked on the wallplacement code in this file, so perhaps you found him using the revision history. Might suceed with a ping @, but we have people who are more dedicated to the JS GUI, Imarok and bb would be a good bet, but one of them has broken bones and the other one is studying. Vladislav is another option, but he is also busy with various stuff. I guess I know another option to have this patch committed that involves me not committing it, but that would be new.
Comment Actions
None but given that the state can only be written with uppercase it might serve in the future as a word separator given that it doesn't conflict with the naming of the methods (but anyway I can just revert this change for now)
The default allows to ignore all the events that one might not want to process but still happen (e.g. if in a state "PLACING" you only want to process "onmousemotion" event and ignore all the other types of events) otherwise the FSM will warn you every time you try to process an event in that state that is not defined. Mostly the default method should be an empty function in this FSM case (unless we are talking about those two "default" that I had to define at "PRESELECTED" and "SELECTING.BOX" states given that I could not find a way to refactor them to non-defaults methods.
No luck
Comment Actions
So one of the problems is that all input events such as keydown, mousedown can be triggered, the FSM model expects all these to be defined, but we don't want to specify tons of empty functions? Would it actually be that many empty functions? Some empty lines here and there wouldn't hurt so much and it could possibly yield a more complete picture. Empty call handlers could also be considered as an incentive to think about filling them.
The events have entirely different arguments (keydown the button, mousemove the coordinates), so the arguments of the default function is unable to consume the provided argument. Isn't InputEvents.prototype.events.default unneeded either way? Why are unit actions (patrol, gather, ...) mixed with input events (keydown, mousemove, ...) for some objects?
(I meant whether you had found him by looking through the authors of this file) You're welcome. As mentioned in a previous post, I post my comments to help educate about good practices, but I'm currently not working on code until I fixed some problems with working on code that I had over the years.
Comment Actions Preliminary note: If you can, move the non-refactoring changes outside of this diff. It's big enough. Git is convenient for this. I am not sure this is the right way to go about refactoring input.js. The switch-as-FSM isn't the big issue here, the issue is that everything is a global and it's a huge mess. Since all actions appear revert back to "Normal" (i.e. none switch to another state / at most they switch to one of their own sub-state), I don't think we need an FSM. Comment Actions Didn't look closely at it, but we definitely shouldn't copy the general fsm definition, but instead put it somewhere where UnitAI and input.js can use it. |