Fix redeclarations of variables
Fix variable shadowing
Remove duplicated cmpPlayer
Move querying of the same components out of loop
Change if (foo.length > 0) to if (foo.length)
Change if (foo == 0) to if(!foo)
Fix remaining var -> let
Remove white space from empty object { } to {}
Restyle more complicated objects.
Details
- Reviewers
- None
- Commits
- rP23322: Fix style, Cc and slightly refactor ProductionQueue
Jenkins does not report anything about variables redeclaration and shadowing.
Check nothing is broken.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 10328 Build 17602: Vulcan Build Jenkins Build 17601: Vulcan Build (Windows) Jenkins Build 17600: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/765/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1281/display/redirect
Fix redeclarations of variables, remove duplicated cmpPlayer and fix remaining var->let
Should mention that ProductionQueue is the subject of the patch, so that multiple patches changing var to let can be distinguished by the title.
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
2 | Perhaps move those two into the prototype | |
206 | coding convention is to use ++i except where the return value matters | |
395–402 | (spaces between operators) | |
417 | (Wondering if the two if (type == cases should go to helper functions, but scope creep I guess) | |
520–529 | return this.queue.map(item => ({ ... })) | |
562–563 | (&& ... .length) if you will | |
793 | { } -> {} if it needs a value, same L730 | |
810–811 | If objects are written this way: let notification = { "players": [cmpPlayer.GetPlayerID()], "message": markForTranslation("Can't find free space to spawn trained units"), "translateMessage": true }; it has the advantage that the reader can count and locate properties without reading all property values. Also the notification variable can be inlined. |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/768/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1284/display/redirect
i am not sure changes around ApplyValueModificationsToEntity functions are correctly done
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/769/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1285/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/770/display/redirect
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
726–728 | time can become negative time -= item.timeRemaining so time >0 is needed |
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
2 | does that not mean x times more variables where x is number of entities with production queue in game? |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (indent): | | Expected indentation of 3 tabs but found 4. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js | 283| 283| +template.Promotion.RequiredXp, | 284| 284| cmpPlayer.GetPlayerID(), | 285| 285| template) | 286| |- ) | | 286|+ ) | 287| 287| { | 288| 288| this.AddBatch(template.Promotion.Entity, type, count, metadata); | 289| 289| return; | | [NORMAL] ESLintBear (no-multi-spaces): | | Multiple spaces found before 'Engine'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/ProductionQueue.js | 721| 721| // with items that take fractions of a second) | 722| 722| let time = g_ProgressInterval; | 723| 723| let cmpPlayer = QueryOwnerInterface(this.entity); | 724| |- let cmpTemplateManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager); | | 724|+ let cmpTemplateManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager); | 725| 725| | 726| 726| while (time > 0 && this.queue.length) | 727| 727| { Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1286/display/redirect
I didn't check for semantic changes, but the style changes look okay.
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
2 | Nope, its stored in the prototype of the object, just like the functions. The component objects have owned properties and the prototype property which an object reference. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Inheritance_and_the_prototype_chain | |
182 | for...of if you want | |
286 | -\n | |
453 | perhaps for...of loop (I heard it was said it's slower, but what's the data for that actually, i.e. does that also include the cost for this.queue[i] lookup, and is it significant enough? I guess this function is only called per user click so its negligible here anyway but the iteration difference may still be relevant for code that is run often) | |
468 | inline unitCategory | |
740 | +1 tab should be enough | |
815 | (I've always used +1 tab instead of tabs for object properties, but its not set in stone I guess) |
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
2 | (Trivia: One could run some turns of a match with -ooslog and see the text representation of the savegame to see if these variables would end up in the savegame. The savegame only writes owned properties (see the JS_Enumerate call in CBinarySerializerScriptImpl::HandleScriptVal which https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_Enumerate If they were present in that file for each ProductionQueue entity component then it'd be stored per class instance. If they're not in the file, it wouldn't rule out that there is still one copy of the number per class instance in memory, so for that we have to either trust the specs or read the spidermonkey code) |
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
2 | (Other than that usually the first association of hardcoded literals in simulation components I have is to move them to the template files, since the purpose of the template files is to store all the values of simulation components to begin with |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/894/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1410/display/redirect
(Still not a fan of the period additions, I guess I don't have to be. What we ought to strive for is to gain the most quality change per line of code changed, and if we don't add anything useful to a hunk we get more quality/line of code changed if we remove the diff. But if you guys want that period I wont stop you in your diffs)
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
2 | ^ This, not? | |
187 | space after { and before } -a | |
259 | (ideally there should be no speculation in code) | |
502 | (I suppose its slightly more performant to pass undefined than creating a garbage-collection tracked empty object, I suppose semantic changes are out of scope of this diff) | |
604 | cmpPosition is only needed in one of the cases, so (unless the JIT optimizer is smart), it would become less performant for no benefit or did I miss something? | |
613 | // so | |
719 | 1000msecs shouldnt be hardcoded in the comment | |
750 | (Comment redundant with function name?) | |
799 | (This is not a sentence, so the period would be grammatically wrong?) | |
815 | Its one convention people recommend, I personally didn't like nor use it (as 4 spaces require 4 more keypresses to insert and navigate and provide more surface area for too many or too few indentation). I don't care much, only problem is that if we dont use the same CC then we'll have inconsistent code, one patch adding it this way the next the other way, perhaps even some patches changing the same lines back and forth, so in theory it should become conventional or perhaps the consequences of different styles aren't so impactful, whatever) | |
884 | // If |
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
604 | One of the cases is in for loop and it is when unit is not going to be auto-garrisoned what happens in most cases when spawning units |
I wonder if all the ApplyValueModification calls affecting other components shouldn't be functions in said components.They would be cached when possible and updated on ValueModification.
in GetBatchTime could get cached, but i am not sure about GetTechCostMultiplier (because number of different resources) and others are modification to templates and that are no go, as that would probably result in a lot of used memory if done.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1435/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/917/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/12/display/redirect
You are right about cmpPosition being more performant if its moved out of the loop (in the average case at least).
From a brief look, the patch looks okay.
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
529 | It seems like a big waste to recreate the array and every item of it when it seems only one property of every object changes, or did I miss something? I suppose it's cloning the thing to avoid modification. It could avoid doing so by letting the consumer of the return value make that one computation. It also opens up the door for the consumer to unintentionally modify the object, but that probably should have lower precedence than copying it. From a quick glance, it doesn't seem to be used that rarely (AIProxy, GetEntityState). (Its not a regression if its not addressed in this revision.) |
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
590 | timer is cancelled and what is in variable this.timer does not matter because this entity is going to be destroyed |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/924/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/19/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1442/display/redirect
The patch looks good now.
binaries/data/mods/public/simulation/components/ProductionQueue.js | ||
---|---|---|
305 | Move below? Inline? |