Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#3424 closed enhancement (fixed)

[PATCH] Clean up Looter.js

Reported by: Stan Owned by: elexis
Priority: Should Have Milestone: Alpha 20
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by elexis)

cmpStatisticsTracker.IncreaseLootCollectedCounter(resourcesToAdd); is called identically twice, and there are some other things that can be cleaned.

Attachments (4)

3424.diff (3.0 KB ) - added by Stan 9 years ago.
Initial Elexis's patch
t3424_cleanup_looting_v1.patch (3.1 KB ) - added by elexis 9 years ago.
commands.txt (16.3 KB ) - added by elexis 9 years ago.
Visually replaying this file proves that my patch doesn't work as sometimes ApplyValueModificationsToEntity can return something different from what values it received. (Non-visual replay only checks every 20th turn, while visual-replay checks every turn).
t3424_cleanup_looting_v2.patch (2.9 KB ) - added by elexis 9 years ago.
Reverts that ApplyValueModificationsToEntity line and further simplifies those resources[carriedGoods.type] += carriedGoods.amount.traderGain || 0 lines.

Download all attachments as: .zip

Change History (11)

by Stan, 9 years ago

Attachment: 3424.diff added

Initial Elexis's patch

by elexis, 9 years ago

comment:1 by elexis, 9 years ago

Description: modified (diff)
Keywords: patch review added
Type: defectenhancement

fcxSanya implemented Looter.js in r9391 and r11279.

We should clean this code before adding more code, like #2732.

  • added comments
  • removed reundancy by summing up the looted resources and then transfering them and updating the statistics only once)
  • not 100% sure about the call to ApplyValueModificationsToEntity. In my testing it gave the same results, so we might not need to update resources with the return value. In doubt, revert that line.
  • conditional operator to improve readability
  • let instead of var

by elexis, 9 years ago

Attachment: commands.txt added

Visually replaying this file proves that my patch doesn't work as sometimes ApplyValueModificationsToEntity can return something different from what values it received. (Non-visual replay only checks every 20th turn, while visual-replay checks every turn).

by elexis, 9 years ago

Reverts that ApplyValueModificationsToEntity line and further simplifies those resources[carriedGoods.type] += carriedGoods.amount.traderGain || 0 lines.

comment:2 by Itms, 9 years ago

Milestone: Alpha 19Alpha 20

comment:3 by elexis, 9 years ago

This patch is a preparation for #2732. fcxSanya reviewed it today and thinks it is good to go (see irc today).

comment:4 by fcxSanya, 9 years ago

The second patch (t3424_cleanup_looting_v2.patch​) looks fine to me, but an active team member has to re-review and merge it in (after A19 release).

comment:5 by elexis, 8 years ago

Resolution: fixed
Status: newclosed

In 17318:

Cleans up Looter.js. Removes duplicate code and simplifies it. Fixes #3424. Thanks fcxSanya for the review!

comment:6 by elexis, 8 years ago

Keywords: review removed

comment:7 by elexis, 8 years ago

In 17353:

Remove wrong semicolons. Refs #3424

Note: See TracTickets for help on using tickets.