refactor/1697 introduce parse function which returns ParsedCommand #1698
Conversation
|
I just had an idea... |
There was a problem hiding this comment.
I’m not sure I understand the name change here. CommandParser describes and action. It does something that the name describes. ParsedCommand, however, suggests a thing has already been done. This name describes an immutable thing.
Perhaps that’s the direction? I want to call it out now, tho, cause naming is incredibly important.
|
@n8rzz Your totally right, I updated the PR now such that there is an actual |
JonasCir
force-pushed
the
refactor/1697
branch
from
264ef64
to
c357ea3
Compare
JonasCir
force-pushed
the
refactor/1697
branch
from
c357ea3
to
076617d
Compare
JonasCir
changed the title
JonasCir
force-pushed
the
refactor/1697
branch
from
e87333b
to
52f106c
Compare
|
This is better :) The parser logic is now separated from the command that is returned. |
erikquinn
added
the
refactor
|
|
||
| if (aircraftCommandParser.command !== PARSED_COMMAND_NAME.TRANSMIT) { | ||
| return this.processSystemCommand(aircraftCommandParser); | ||
| const parsedCommand = cmd; |
There was a problem hiding this comment.
Why are we reassigning here? Could we continue to use cmd?
There was a problem hiding this comment.
the reason is that I want the parsed command to be const, but I cannot do it in the try block as it is then only scoped to the block. To make it const, I need to trick and use a reassign here.
erikquinn
force-pushed
the
refactor/1697
branch
from
2992535
to
1b5dfa2
Compare
Closes #1697
By this, the call site in
InputController.jsgets more readable.Consider this PR as a trick to reduce the diff of the following PR. Once this is merged, I can start to separate the parsing process from the command that is returned and handled.