#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 )
cmpStatisticsTracker.IncreaseLootCollectedCounter(resourcesToAdd);
is called identically twice, and there are some other things that can be cleaned.
Attachments (4)
Change History (11)
by , 9 years ago
by , 9 years ago
Attachment: | t3424_cleanup_looting_v1.patch added |
---|
comment:1 by , 9 years ago
Description: | modified (diff) |
---|---|
Keywords: | patch review added |
Type: | defect → enhancement |
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 updateresources
with the return value. In doubt, revert that line. - conditional operator to improve readability
- let instead of var
by , 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 , 9 years ago
Attachment: | t3424_cleanup_looting_v2.patch added |
---|
Reverts that ApplyValueModificationsToEntity
line and further simplifies those resources[carriedGoods.type] += carriedGoods.amount.traderGain || 0
lines.
comment:2 by , 9 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|
comment:3 by , 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 , 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:6 by , 8 years ago
Keywords: | review removed |
---|
Initial Elexis's patch