- Early returns
- Doxygen
- If/else removal
- This.isInfinite is no longer serialized
Details
- Reviewers
Imarok - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22103: Clean ResourceSupply up.
Make sure all the tests pass ask for other tests.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 6948 Build 11371: Vulcan Build Jenkins
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | 34| 34| | 35| 35| // List of IDs for each player | 36| 36| this.gatherers = []; | 37| |- let numPlayers = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers() | | 37|+ let numPlayers = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers(); | 38| 38| for (let i = 0; i < numPlayers; ++i) | 39| 39| this.gatherers.push([]); | 40| 40| | | [NORMAL] ESLintBear (no-trailing-spaces): | | Trailing spaces not allowed. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | 99| 99| { | 100| 100| if (!this.template.DiminishingReturns) | 101| 101| return null; | 102| |- | | 102|+ | 103| 103| let diminishingReturns = ApplyValueModificationsToEntity("ResourceSupply/DiminishingReturns", +this.template.DiminishingReturns, this.entity); | 104| 104| if (!diminishingReturns) | 105| 105| return null; | | [NORMAL] ESLintBear (no-trailing-spaces): | | Trailing spaces not allowed. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | 101| 101| return null; | 102| 102| | 103| 103| let diminishingReturns = ApplyValueModificationsToEntity("ResourceSupply/DiminishingReturns", +this.template.DiminishingReturns, this.entity); | 104| |- if (!diminishingReturns) | | 104|+ if (!diminishingReturns) | 105| 105| return null; | 106| 106| | 107| 107| let numGatherers = this.GetNumGatherers(); | | [NORMAL] ESLintBear (no-trailing-spaces): | | Trailing spaces not allowed. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | 164| 164| ResourceSupply.prototype.RemoveGatherer = function(gathererID, player) | 165| 165| { | 166| 166| if (player != undefined && player != INVALID_PLAYER) | 167| |- { | | 167|+ { | 168| 168| let index = this.gatherers[player].indexOf(gathererID); | 169| 169| if (index == -1) | 170| 170| return; binaries/data/mods/public/simulation/components/ResourceSupply.js | 37| » let·numPlayers·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_PlayerManager).GetNumPlayers() | | [NORMAL] JSHintBear: | | Missing semicolon. binaries/data/mods/public/simulation/components/ResourceSupply.js | 109| » » return·diminishingReturns·==·1·?·1·:·(1.·-·Math.pow(diminishingReturns,·numGatherers))·/·(1.·-·diminishingReturns)·/·numGatherers; | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '1.'. binaries/data/mods/public/simulation/components/ResourceSupply.js | 109| » » return·diminishingReturns·==·1·?·1·:·(1.·-·Math.pow(diminishingReturns,·numGatherers))·/·(1.·-·diminishingReturns)·/·numGatherers; | | [NORMAL] JSHintBear: | | A trailing decimal point can be confused with a dot: '1.'. Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1022/
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1023/
binaries/data/mods/public/simulation/components/ResourceSupply.js | ||
---|---|---|
76 | "An object containing both the subtype, and the generic type. All resources must have both."→"An object containing the subtype and the generic type. All resources must have both."? | |
93 | additionnal→additional | |
114 | resource→resources? | |
124–125 | Why have you added that? | |
131 | I think this comment should be one line lower. | |
140–141 | Aren't those in the wrong order? | |
141 | "The gatherer's id."→"The gatherer's player id." | |
162 | "The gatherer's id."→"The gatherer's player id." | |
168–183 | I would stay with the if else, but remove the return. |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (no-else-return): | | Unnecessary 'else' after 'return'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/ResourceSupply.js | 176| 176| Engine.PostMessage(this.entity, MT_ResourceSupplyNumGatherersChanged, { "to": this.GetNumGatherers() }); | 177| 177| return; | 178| 178| } | 179| |- else | 180| |- for (let i = 0; i < this.gatherers.length; ++i) | | 179|+ for (let i = 0; i < this.gatherers.length; ++i) | 181| 180| this.RemoveGatherer(gathererID, i); | 182| 181| }; | 183| 182| Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1078/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1079/display/redirect
Rest looks good.
binaries/data/mods/public/simulation/components/ResourceSupply.js | ||
---|---|---|
168–183 | Imho the way how the original code is structured is more readable. |
binaries/data/mods/public/simulation/components/ResourceSupply.js | ||
---|---|---|
168–183 | Fair enough. The recursive call could be replaced by this, but not sure its more readable let playerID = this.gatherers.findIndex(player => player.findIndex(gatherer => gatherer == gathererID) != -1) this.RemoveGatherer(gathererID, playerID); |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1080/display/redirect