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

Improve bombard and AA unit tooltip info #3361

Merged
merged 2 commits into from Apr 11, 2018
Merged

Conversation

ron-murhammer
Copy link
Member

@ron-murhammer ron-murhammer commented Apr 11, 2018

Addressing some feedback from TWW thread like: https://forums.triplea-game.org/topic/493/total-world-war-december-1941-beta-2-8-0-4/222

Just changes the tooltip and unit help text to add more detail and improve format.

TWW BB and Sub Example:
image

@codecov-io
Copy link

codecov-io commented Apr 11, 2018

Codecov Report

Merging #3361 into master will increase coverage by 0.93%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3361      +/-   ##
============================================
+ Coverage     21.95%   22.89%   +0.93%     
- Complexity     5888     6634     +746     
============================================
  Files           832      833       +1     
  Lines         71877    75734    +3857     
  Branches      11588    13290    +1702     
============================================
+ Hits          15781    17336    +1555     
- Misses        54014    56167    +2153     
- Partials       2082     2231     +149
Impacted Files Coverage Δ Complexity Δ
...s/strategy/triplea/attachments/UnitAttachment.java 42.26% <0%> (+0.02%) 251 <0> (ø) ⬇️
...rategy/triplea/attachments/UnitTypeComparator.java 46.15% <0%> (-7.7%) 12% <0%> (-1%)
.../games/strategy/engine/data/BattleRecordsList.java 8.53% <0%> (-2.24%) 3% <0%> (ø)
...a/games/strategy/triplea/delegate/TechAdvance.java 87.67% <0%> (-2.02%) 60% <0%> (+23%)
.../strategy/triplea/odds/calculator/DummyPlayer.java 35.22% <0%> (-1.14%) 9% <0%> (-1%)
...s/strategy/triplea/delegate/RocketsFireHelper.java 5.55% <0%> (-0.75%) 5% <0%> (ø)
...trategy/triplea/delegate/NoPUPurchaseDelegate.java 1.42% <0%> (-0.46%) 1% <0%> (ø)
...java/games/strategy/triplea/delegate/DiceRoll.java 56.13% <0%> (-0.44%) 153% <0%> (+48%)
.../games/strategy/triplea/ui/screen/TileManager.java 3.47% <0%> (-0.3%) 12% <0%> (+4%)
...amework/headlessGameServer/HeadlessGameServer.java 0.63% <0%> (-0.09%) 3% <0%> (ø)
... and 50 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 695991f...3830dab. Read the comment docs.

Copy link
Member

@DanVanAtta DanVanAtta left a comment

Choose a reason for hiding this comment

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

Just the one small comment, otherwise LGTM

}
stats.append("with ").append(getMaxAaAttacks() != -1 ? getMaxAaAttacks() : "Unlimited").append(" Attacks for ");
Copy link
Member

Choose a reason for hiding this comment

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

On this condition: getMaxAaAttacks() != -1

I like the cleanup with ternary. The -1 becomes a magic number though, personally I'd like to see the method own the logic/knowledge of what is unlimited max attacks, eg something like getMaxAaAttacksHumanReadable() (maybe not exactly that, but as an example to convey the point).

That seems like a bit much for right now though, especially with consideration that this part of the code owns all of the string logic so far. In that case I'd suggest to simply alleviate the magic number problem by converting:
!= -1 -> > -1

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there are a lot of magic "-1" as its used to indicate unlimited or infinite for many parameters. I'm fine changing to ">" as I guess that helps a little bit.

@ron-murhammer ron-murhammer merged commit ed874fe into master Apr 11, 2018
@RoiEXLab RoiEXLab deleted the Improve_Unit_Tooltip branch April 11, 2018 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants