Add fuel to scrambling #3292

ron-murhammer · 2018-03-17T03:58:55Z

Address point 6 on https://forums.triplea-game.org/topic/558/fuel-enhancements

Functional Changes

Scramble dialog now shows estimated fuel costs for any units selected to move there and back
Fuel costs are treated just like movement during a player's turn so 'fuelFlatCost' is charged once if unit is scrambled, 'fuelCost' is charged for each move
Fuel validation and game data changes for selected scramble units added
Fixed scramble window to have proper icon and close if game is exited to main menu
Updated Actions and Operations window to remove remaining resources since its duplicate information of the bottom bar:

Testing
Manually edited Global 40 2nd to add this to fighters:

	  <option name="fuelFlatCost" value="PUs" count="4"/>
	  <option name="fuelCost" value="PUs" count="1"/>

And this property:

    <property name="Use Fuel Cost" value="true" editable="false">
      <boolean/>
    </property>

Result:

More examples: https://forums.triplea-game.org/topic/558/fuel-enhancements/227

codecov-io · 2018-03-17T06:17:45Z

Codecov Report

Merging #3292 into master will decrease coverage by 0.01%.
The diff coverage is 10.71%.

@@             Coverage Diff             @@
##             master   #3292      +/-   ##
===========================================
- Coverage     21.91%   21.9%   -0.02%     
- Complexity     5872    5875       +3     
===========================================
  Files           837     837              
  Lines         72620   72696      +76     
  Branches      11852   11865      +13     
===========================================
+ Hits          15916   15921       +5     
- Misses        54598   54671      +73     
+ Partials       2106    2104       -2

Impacted Files	Coverage Δ	Complexity Δ
...ames/strategy/triplea/delegate/BattleDelegate.java	`22.31% <0%> (-0.4%)`	`53 <0> (ø)`
...ava/games/strategy/triplea/ui/ProductionPanel.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...n/java/games/strategy/triplea/ui/TripleAFrame.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...in/java/games/strategy/triplea/ui/UnitChooser.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ava/games/strategy/triplea/ui/UserActionPanel.java	`1.76% <0%> (+0.06%)`	`4 <0> (ø)`	⬇️
...s/strategy/triplea/image/ResourceImageFactory.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...games/strategy/triplea/delegate/MovePerformer.java	`72.83% <100%> (-0.11%)`	`44 <0> (ø)`
...rc/main/java/games/strategy/engine/data/Route.java	`57.36% <40.74%> (-4.62%)`	`59 <6> (+2)`
...rc/main/java/games/strategy/net/nio/NioReader.java	`72.63% <0%> (-2.11%)`	`17% <0%> (ø)`
.../src/main/java/games/strategy/net/nio/Decoder.java	`72.82% <0%> (-1.09%)`	`24% <0%> (-1%)`
... and 5 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 e73be10...91b0f52. Read the comment docs.

DanVanAtta

Looks generally okay. Some of the code is a bit verbose, a refactoring pass could potentially help.

I am a bit concerned that the updates are all in-line for the most part without unit test. Given the existing code it's not too unreasonable the decisions made; I suspect we will often be able to say that as a reason for not unit testing.

I've took a few passes at this now; again looks pretty good overall. Just need to do some additional play testing and I would give this an okay. Anyone else is welcome to approve in the meantime.

DanVanAtta · 2018-03-19T02:58:01Z

game-core/src/main/java/games/strategy/engine/data/Route.java

+      final ResourceCollection cost = new ResourceCollection(data);
+      cost.add(getMovementFuelCostCharge(Collections.singleton(unit), toRoute, player, data));
+      cost.add(getFuelCostsAndUnitsChargedFlatFuelCost(
+          Collections.singleton(unit), returnRoute, player, data, true).getFirst());


Noting the APIs, it looks like we could improve it a bit and have more work be done by the 'callee' methods, noting:

getFuelCostsAndUnitsChargedFlatFuelCost(Collections.singleton(unit), returnRoute, player, data, true).getFirst()

In this example we could have the first arg take a unit directly, avoid the true parameter by creating a method overload, and could avoid the getFirst by having the method overload return that directly for us.

That would give something like:

I think there is also a method like computeIfAbsent that could replace the if/else if structure for the map update. Going that route, if the body of the for loop were extracted to a helper method I think you could break this up into 2 or 3 pretty clean private methods.

I actually originally created a method that I think was:

getFuelCostsIgnoringFlatFuel(Unit, Route, PlayerID, GameData)

I ended up not seeing much value in slightly cleaner parameters at the cost of adding misdirection given that the call is to a private method pretty much right below this one. If the call was to a public method then I'd completely agree. I'm open to adding an overloaded method back in if there is a strong consensus/opinion that it improves readability/maintainability.

I will leave it up to you.

I like the cleaner and reduced parameters as it can make the method a bit more self-descriptive. If you can understand a method by it's name and args, then a person does not need to start hopping down the indirection tree. On the flip side, the boolean parameter being non-indirected requires reverse engineering of its usage to know what it really does.

Second comment/note I'll make, I tend to favor cleaner client code over server ("callee") code. If the client is not clean, then I'll iterate until it is. Then I drop down to the "callee" code and clean that up and it's dependent API's until that is also clean. This way down the whole stack there is no code you can't just read and understand.

Third and final comment here:
Normally unit tests would be the driving factor in this decision. The biggest benefit of unit testing is for decoupling design, not for catching bugs.

I'm inclined to leave as is for now since I'm not really strongly convinced one way is much better than the other. I agree with most of you approach and tend to do something fairly similar. And I think the public API calls of Route involving fuel are pretty decent. Unless you have any more specific questions/asks then I'd like to move forward with it and we can always revisit.

DanVanAtta

Finished reviewing 100%
Found 3 nitpicky comments, they're low level. If we disagree in theory it would be an interesting conversation, but otherwise not a blocker - looks good otherwise, approving 🥂

DanVanAtta · 2018-03-20T04:08:55Z

game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java

@@ -1268,6 +1309,16 @@ public Component getListCellRendererComponent(final JList<? extends Unit> list,
    return selection;
  }

+  private JOptionPane getOptionPane(final JComponent parent) {
+    JOptionPane pane = null;


A ternary IMO would clean this up quite a bit:

return !(parent instanceof JOptionPane) ? getOptionPane((JComponent) parent.getParent()) : (JOptionPane) parent;

If going with the old pattern, not setting the variable to null and then marking it final to ensure (compiler-enforced) that ti would get set in the if/else block is a good practice IMO. It's a signal to reader that the variable would be set, eg: final JOptionPane pane;.

DanVanAtta · 2018-03-20T04:11:50Z

game-core/src/main/java/games/strategy/triplea/ui/UnitChooser.java

@@ -302,6 +302,10 @@ private void addToCollection(final Collection<Unit> addTo, final ChooserEntry en
    });
  }

+  public void addChangeListener(final ScrollableTextFieldListener listener) {
+    entries.stream().forEach(entry -> entry.addChangeListener(listener));


List has a convenience forEach method attached to it. FYI can simplify a little bit with it:

entries.forEach(entry -> entry.addChangeListener(listener));

DanVanAtta · 2018-03-20T04:12:32Z

game-core/src/main/java/games/strategy/triplea/ui/UnitChooser.java

@@ -450,6 +454,10 @@ boolean hasMultipleHitPoints() {
      return hasMultipleHits;
    }

+    void addChangeListener(final ScrollableTextFieldListener listener) {
+      hitTexts.stream().forEach(field -> field.addChangeListener(listener));


ditto on simplification to remove stream() and call forEach directly on the list.

ron-murhammer · 2018-03-20T04:22:44Z

@DanVanAtta Yep, good catch on those 3. Updated. Not sure that even I could come up with a reasonable argument against those :)

ron-murhammer · 2018-03-20T15:16:44Z

Merging based on all feedback being addressed.

asvitkine · 2018-06-30T03:26:53Z

I think this change introduced this error when scrambling on the WW2 Pacific Map:

java.lang.ClassCastException: java.lang.String cannot be cast to javax.swing.JButton
at games.strategy.triplea.ui.TripleAFrame.lambda$scrambleUnitsQuery$39(TripleAFrame.java:1261)
at java.beans.PropertyChangeSupport.fire(PropertyChangeSupport.java:335)
at java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:327)
at java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:263)
at java.awt.Component.firePropertyChange(Component.java:8428)
at javax.swing.JComponent.putClientProperty(JComponent.java:4087)
at com.apple.laf.AquaRootPaneUI.updateComponentTreeUIActivation(AquaRootPaneUI.java:305)
at com.apple.laf.AquaRootPaneUI.updateComponentTreeUIActivation(AquaRootPaneUI.java:319)
at com.apple.laf.AquaRootPaneUI.updateComponentTreeUIActivation(AquaRootPaneUI.java:319)
at com.apple.laf.AquaRootPaneUI.updateComponentTreeUIActivation(AquaRootPaneUI.java:319)
at com.apple.laf.AquaRootPaneUI.windowDeactivated(AquaRootPaneUI.java:281)
at java.awt.Window.processWindowEvent(Window.java:2069)
at javax.swing.JDialog.processWindowEvent(JDialog.java:683)
at java.awt.Window.processEvent(Window.java:2013)

ron-murhammer · 2018-06-30T03:40:21Z

@asvitkine Is this a older save game or a lobby game? Do you have a save game that reproduces this?

asvitkine · 2018-06-30T03:45:12Z

This is caused by JOptionPane.UNINITIALIZED_VALUE being returned when a choice hasn't been made yet, which is a String.

I'll send a PR to fix.

asvitkine · 2018-06-30T03:50:33Z

#3493

ron-murhammer added 9 commits March 16, 2018 22:31

Add listener method to UnitChooser

3cec84c

Add new scramble fuel method

b0e26f6

Refactor resource panel logic to be reused

fcf5f19

Refactor ProductionPanel to use new method

bbe67d6

Refactor MovePerformer based on Route changes

66d4348

Add backend scrambling fuel validation and data changes

5f43506

Update scrambling dialog to show and validate fuel costs

227a850

Fix getting JOptionPane button click

86c7744

Clean up

5e77100

ron-murhammer added 3 commits March 17, 2018 11:58

Fix parent of dialog so it shows proper icon and closes properly

058b347

Update UserActionPanel to use resource icons for remaining resources

83d72a4

Remove remaining resources from Actions window since its duplicate info

4a6eac8

DanVanAtta reviewed Mar 19, 2018

View reviewed changes

DanVanAtta approved these changes Mar 20, 2018

View reviewed changes

Code clean up

91b0f52

ron-murhammer merged commit d7eb4bc into master Mar 20, 2018

RoiEXLab deleted the Add_Fuel_To_Scrambling branch March 20, 2018 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fuel to scrambling #3292

Add fuel to scrambling #3292

ron-murhammer commented Mar 17, 2018 •

edited

Loading

codecov-io commented Mar 17, 2018 •

edited

Loading

DanVanAtta left a comment

DanVanAtta Mar 19, 2018

ron-murhammer Mar 19, 2018 •

edited

Loading

DanVanAtta Mar 19, 2018

DanVanAtta Mar 19, 2018

ron-murhammer Mar 19, 2018

DanVanAtta left a comment

DanVanAtta Mar 20, 2018 •

edited

Loading

DanVanAtta Mar 20, 2018

DanVanAtta Mar 20, 2018

ron-murhammer commented Mar 20, 2018

ron-murhammer commented Mar 20, 2018

asvitkine commented Jun 30, 2018

ron-murhammer commented Jun 30, 2018

asvitkine commented Jun 30, 2018

asvitkine commented Jun 30, 2018

Add fuel to scrambling #3292

Add fuel to scrambling #3292

Conversation

ron-murhammer commented Mar 17, 2018 • edited Loading

codecov-io commented Mar 17, 2018 • edited Loading

Codecov Report

DanVanAtta left a comment

Choose a reason for hiding this comment

DanVanAtta Mar 19, 2018

Choose a reason for hiding this comment

ron-murhammer Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

DanVanAtta Mar 19, 2018

Choose a reason for hiding this comment

DanVanAtta Mar 19, 2018

Choose a reason for hiding this comment

ron-murhammer Mar 19, 2018

Choose a reason for hiding this comment

DanVanAtta left a comment

Choose a reason for hiding this comment

DanVanAtta Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

DanVanAtta Mar 20, 2018

Choose a reason for hiding this comment

DanVanAtta Mar 20, 2018

Choose a reason for hiding this comment

ron-murhammer commented Mar 20, 2018

ron-murhammer commented Mar 20, 2018

asvitkine commented Jun 30, 2018

ron-murhammer commented Jun 30, 2018

asvitkine commented Jun 30, 2018

asvitkine commented Jun 30, 2018

ron-murhammer commented Mar 17, 2018 •

edited

Loading

codecov-io commented Mar 17, 2018 •

edited

Loading

ron-murhammer Mar 19, 2018 •

edited

Loading

DanVanAtta Mar 20, 2018 •

edited

Loading