Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChoiceManager refactoring #771

Merged
merged 9 commits into from May 24, 2022
Merged

ChoiceManager refactoring #771

merged 9 commits into from May 24, 2022

Conversation

Veracity0
Copy link
Contributor

@Veracity0 Veracity0 commented May 22, 2022

ChoiceManager is unwieldy. It is gigantic. I split it into three separate files:

ChoiceManager.java

This contains the various control flow variables - ChoiceManager.handlingChoice, ChoiceManager.lastChoice, ChoiceManager.lastResponseText, and so on - we are familiar with.
It contains the various methods called by GenericRequest - preChoice, postChoice0, postChoice1, postChoice2, visitChoice, registerRequest - to update those variables appropriately.

It also contains the code for automating choices via CHOICE_HANDLER.
All of the choice-specific code has been moved to ChoiceControl, with the exception of the automation code that knows how to choose "appropriate" decisions for specific choices based on current play state.

ChoiceControl.java

This contains the massive switch statements which used to be in postChoice1, visitChoice, and the other control flow methods called by GenericRequest.

ChoiceAdventures.java

This contains the data for all the choices we support in the GUI (with "whichchoiceXXXX" properties - used for automation) or in the Relay Browser (choice spoilers).

In addition to refactoring the massive file into three coherent modules, I made the following improvements:

  • No more storing & passing data around in Object[] and Object[][] structures. We now have actual classes.
  • No more array searches of large arrays to locate the choice data for a particular choice number. HashMaps work well.

My testing looks good, so far. The GUI for choices is unchanged. I see choice spoilers in the Relay Browser. I'm going to test more tomorrow (well, today. After I sleep. :)

I'm marking this ready to review, in case anybody wants to get a head start with it and give me comments.

@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #771 (864ba4c) into main (1a0b987) will increase coverage by 0.25%.
The diff coverage is 18.46%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #771      +/-   ##
============================================
+ Coverage     24.27%   24.52%   +0.25%     
- Complexity    10897    10900       +3     
============================================
  Files          1014     1016       +2     
  Lines        158244   158760     +516     
  Branches      34990    34980      -10     
============================================
+ Hits          38406    38939     +533     
+ Misses       112839   112814      -25     
- Partials       6999     7007       +8     
Impacted Files Coverage Δ
src/net/sourceforge/kolmafia/EdServantData.java 29.08% <0.00%> (ø)
src/net/sourceforge/kolmafia/RequestEditorKit.java 20.78% <0.00%> (+0.01%) ⬆️
...sourceforge/kolmafia/request/BeachCombRequest.java 0.00% <0.00%> (ø)
...rceforge/kolmafia/request/DecorateTentRequest.java 0.00% <0.00%> (ø)
.../sourceforge/kolmafia/request/SpelunkyRequest.java 11.24% <0.00%> (ø)
...t/sourceforge/kolmafia/request/UseItemRequest.java 8.44% <0.00%> (ø)
...sourceforge/kolmafia/session/ChoiceAdventures.java 30.74% <ø> (ø)
...et/sourceforge/kolmafia/session/ChoiceControl.java 4.90% <ø> (ø)
...et/sourceforge/kolmafia/session/ChoiceManager.java 11.67% <ø> (+4.94%) ⬆️
.../sourceforge/kolmafia/session/HaciendaManager.java 0.00% <0.00%> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a0b987...864ba4c. Read the comment docs.

@Veracity0 Veracity0 marked this pull request as ready for review May 24, 2022 03:09
@Veracity0 Veracity0 requested a review from a team as a code owner May 24, 2022 03:09
Copy link
Member

@midgleyc midgleyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice to see moving away from Object[][] :)

src/net/sourceforge/kolmafia/RequestEditorKit.java Outdated Show resolved Hide resolved
src/net/sourceforge/kolmafia/session/WumpusManager.java Outdated Show resolved Hide resolved
@Veracity0 Veracity0 changed the title Move some public methods from ChoiceManager to ChoiceUtilities ChoiceManager refactoring May 24, 2022
@Veracity0
Copy link
Contributor Author

I think this is good to go.

@Veracity0 Veracity0 merged commit 6e8a7db into main May 24, 2022
@Veracity0 Veracity0 deleted the choice-refactor-1 branch May 24, 2022 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants