Extract AtB Event Types to enum #5824
Conversation
There was a problem hiding this comment.
Do we want to pull the event logic and enum into their own class? So that rather than living in AtBContract.java we just include a call to AtBContractEvent.java and have all the event logic handled there? The enum could be contained within AtBContractEvent.java, too. Keep it all together.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5824 +/- ##
============================================
- Coverage 10.28% 10.28% -0.01%
- Complexity 6123 6124 +1
============================================
Files 1039 1040 +1
Lines 139328 139362 +34
Branches 20632 20636 +4
============================================
- Hits 14331 14327 -4
- Misses 123589 123612 +23
- Partials 1408 1423 +15 ☔ View full report in Codecov by Sentry. |
|
Good thought--I'll look more into that this weekend when I have some time |
|
So looking at it a bit more, the event logic in Sorry for the long-winded and confusing explanation...in short, I'm concerned about splitting the logic into a different class while having it remain tightly coupled to the class we extracted it from. As a side note, I am trying different strategies for the CamOps Contract Market and one may actually involve just extending the AtBContract class after all, and having AtBContract just represent the base class for any campaign against the computer rather than a specific game type. If I go that route then your idea may be less of a concern, so maybe we can explore that then? |
| public AtBEventType generateStratConEvent() { | ||
| final int roll = Compute.randomInt(20) + 1; |
There was a problem hiding this comment.
this really should be a static map or something like it.
There was a problem hiding this comment.
Thanks, that probably does make sense. I'm guessing we'd need to simplify the rolls to avoid a bunch of duplicate values (e.g., for keys 10-13 and 14-15), right?
By the way, with the current direction I'm heading with the Contract classes, this PR isn't as useful as it was, so I'm also OK closing it unless we want to take an opportunity to clean up those roll tables like @Scoppio suggested.
| return AtBContract.EVT_BONUSROLL; | ||
| return AtBEventType.BONUSROLL; | ||
| } else if (roll < 14) { | ||
| return AtBContract.EVT_SPECIAL_SCENARIO; | ||
| return AtBEventType.SPECIAL_SCENARIO; | ||
| } else if (roll < 16) { | ||
| return AtBContract.EVT_BETRAYAL; | ||
| return AtBEventType.BETRAYAL; | ||
| } else if (roll < 17) { | ||
| return AtBContract.EVT_TREACHERY; | ||
| return AtBEventType.TREACHERY; | ||
| } else if (roll < 18) { | ||
| return AtBContract.EVT_LOGISTICSFAILURE; | ||
| return AtBEventType.LOGISTICSFAILURE; | ||
| } else if (roll < 19) { | ||
| return AtBContract.EVT_REINFORCEMENTS; | ||
| return AtBEventType.REINFORCEMENTS; | ||
| } else if (roll < 20) { | ||
| return AtBContract.EVT_SPECIALEVENTS; | ||
| return AtBEventType.SPECIALEVENTS; | ||
| } else { | ||
| return AtBContract.EVT_BIGBATTLE; | ||
| return AtBEventType.BIGBATTLE; | ||
| } |
There was a problem hiding this comment.
These, and all following if/else chains, can be simplified using switch/case with fall through.
Start at 20 and work down.
It'll increase readability for most.
|
I have no clue if this is ready to go. Leaving it till it gets approved by a coder. |
|
Sorry @HammerGS , don't merge yet. I mentioned in a separate comment thread that this change could be more trouble than it's worth since I had to change direction on the Contract refactoring, but I'm also happy to implement the reviewers' suggestions if they think there is value in moving those static ints to an enum stored outside the class. Just let me know and I can either finish it or close it. Thanks! |
|
I'll flip it to a draft till things get sorted. |
This simply moves the hardcoded ATB Event Type constants from static variables to an enum. This supports further refactoring work on AtBContract by reducing the number of public variables that other classes need to call, therefore simplifying the public interface. My understanding was also that there was an initiative to try and use enums instead of static ints throughout the codebase.