Page MenuHomeWildfire Games

Clean var values from Trader.js
ClosedPublic

Authored by Angen on Jul 21 2019, 3:44 PM.

Details

Summary

var -> let
This file has only one additional opened revision (D2106) and it is by me, so noone other than me has merge conflicts.

Test Plan

Check nothing is broken by that change.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Angen created this revision.Jul 21 2019, 3:44 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/166/display/redirect

Angen edited the summary of this revision. (Show Details)Jul 21 2019, 3:45 PM
Nescio added a subscriber: Nescio.Jul 21 2019, 4:09 PM

Out of curiosity, what's the difference between var and let? And if it should be let, then why do so many files have var? (Try running grep -cr 'var ' in the ai, components, and helpers folders.)

Angen added a comment.EditedJul 21 2019, 4:37 PM

var is global declaration of variable
if you do var once in some function, that variable exists outside the scope
you can do multiple var with the same name and variable gets redeclared

let is scope variable, it stops exists after scope ends (if, while, function) and if you do multiple let in one scope you get error

I am not sure why, mostly it is old code

We are aware of that, thats why me and others (some of them) inline var->let if someone has diff changes around that variable

And I am planing to clear that on files that have no merge conflicts one by one to keep it small

Also it is CC https://trac.wildfiregames.com/wiki/Coding_Conventions#JavaScript

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
|  93|  93| 		cmpTargetMarket.AddTrader(this.entity);
|  94|  94| 	}
|  95|  95| 	else if (this.markets.length == 1)
|  96|    |-	{
|    |  96|+	
|  97|  97| 		// If we have only one market and target is different from it,
|  98|  98| 		// set the target as second one
|  99|  99| 		if (target == this.markets[0])
| 105| 105| 			cmpTargetMarket.AddTrader(this.entity);
| 106| 106| 			this.goods.amount = this.CalculateGain(this.markets[0], this.markets[1]);
| 107| 107| 		}
| 108|    |-	}
|    | 108|+	
| 109| 109| 	else
| 110| 110| 	{
| 111| 111| 		// Else we don't have target markets at all,
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
|  98|  98| 		// set the target as second one
|  99|  99| 		if (target == this.markets[0])
| 100| 100| 			return false;
| 101|    |-		else
| 102|    |-		{
|    | 101|+		
| 103| 102| 			this.index = 0;
| 104| 103| 			this.markets.push(target);
| 105| 104| 			cmpTargetMarket.AddTrader(this.entity);
| 106| 105| 			this.goods.amount = this.CalculateGain(this.markets[0], this.markets[1]);
| 107|    |-		}
|    | 106|+		
| 108| 107| 	}
| 109| 108| 	else
| 110| 109| 	{
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js
| 274| 274| 	let max = 1;
| 275| 275| 	if (cmpObstruction)
| 276| 276| 		max += cmpObstruction.GetUnitRadius()*1.5;
| 277|    |-	return { "min": 0, "max": max};
|    | 277|+	return { "min": 0, "max": max };
| 278| 278| };
| 279| 279| 
| 280| 280| Trader.prototype.OnGarrisonedUnitsChanged = function()
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/172/display/redirect

elexis accepted this revision.Jul 21 2019, 7:49 PM
This revision is now accepted and ready to land.Jul 21 2019, 7:49 PM

What `let' is:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let
As mentioned the benefit is that the variable is only declared in the innermost scope, whereas var is global to the function.
So sometimes there were two scopes using the same var and overwriting or reusing values without being aware of that, whereas with let that's impossible.
Or var defined in a local scope but used outside of the scope, so that the reader was mislead to believe that the var was used only in that scope when it wasn't.
Also var can be read from before it was defined, while for let it bugs out.

Why let only occurs in new code:

Initial definition:
ECMAScript 2015 (6th Edition, ECMA-262)

binaries/data/mods/public/simulation/components/Trader.js
49 ↗(On Diff #9034)

++x while at it

277 ↗(On Diff #9034)

+\ as the bot pointed out

smiley added a subscriber: smiley.EditedJul 21 2019, 8:04 PM

That's an oversimplification.

function test()
{
    var a = 6;
    console.log(a);
}

test();
console.log(a);

would produce a ref error.

Angen updated this revision to Diff 9043.Jul 21 2019, 8:07 PM

++, space, also removes not needed else branching because if before use return, and inlines one variable

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/177/display/redirect

smiley removed a subscriber: smiley.Jul 21 2019, 8:08 PM
elexis accepted this revision.Jul 21 2019, 10:17 PM

Patch achieves the objective of silencing the linter in this file (except for one controversial brace, the rule will probably be nuked D2070).
Tested quickly with self-trading works.

(With "global to the function" I meant referable to in the entire scope of the function, specs phrase it as "locally to an entire function regardless of block scope"; at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/statements/var its defined as "execution context, which is either the enclosing function or, for variables declared outside any function, global".)

binaries/data/mods/public/simulation/components/Trader.js
106 ↗(On Diff #9043)

Good

159 ↗(On Diff #9043)

Okay with this inlining since it's a number.

(There is always some debate whether one should check for QueryOwnerInterface returning an object in cases where one expects it always to do that, so inlining the other variables might already be controversial)

This revision was landed with ongoing or failed builds.Jul 21 2019, 10:20 PM
Closed by commit rP22523: Trader linting, refs D2106. (authored by elexis). · Explain Why
This revision was automatically updated to reflect the committed changes.