r93654 MediaWiki - Code Review archive

The indentation looks weird here too. Why not just make it two var statements and then it's totally clear?

I don't care how the indention is here, I changed it because someone thought it was better and I didn't disagree.

Alternatives:

var
	// Foobar
	foo = 'bar',
	bar = 'barbarabar',
	// Lorem ipsum
	lorem = 'ipsum';

var	foo = 'bar', // Foobar
	bar = 'barbarabar',
	lorem = 'ipsum'; // Lorem ipsum

var foo = 'bar', // Foobar
	bar = 'barbarabar',
	lorem = 'ipsum'; // Lorem ipsum

Currently:

// Foobar
var	foo = 'bar',
	bar = 'barbarabar',
		// Lorem ipsum
		lorem = 'ipsum';

Previously:

// Foobar
var foo = 'bar',
bar = 'barbarabar',
// Lorem ipsum
lorem = 'ipsum';

I don't care how the indention is here, I changed it because someone thought it was better and I didn't disagree.

Alternatives:

var
	// Foobar
	foo = 'bar',
	bar = 'barbarabar',
	// Lorem ipsum
	lorem = 'ipsum';

var	foo = 'bar', // Foobar
	bar = 'barbarabar',
	lorem = 'ipsum'; // Lorem ipsum

var foo = 'bar', // Foobar
	bar = 'barbarabar',
	lorem = 'ipsum'; // Lorem ipsum

Currently:

	// Foobar
	var	foo = 'bar',
		bar = 'barbarabar',
		// Lorem ipsum
		lorem = 'ipsum';

Previously:

	// Foobar
	var foo = 'bar',
	bar = 'barbarabar',
	// Lorem ipsum
	lorem = 'ipsum';

So yeah, pick whatever you think is right. But I don't see why we should create multiple var statements when it can perfectly go into one.

I'd go with option #3:

var foo = 'bar', // Foobar
	bar = 'barbarabar',
	lorem = 'ipsum'; // Lorem ipsum

I don't see a good reason why you even need two vars in that var statement, regItemDepLen makes more sense being defined along with n as they're both only used within the context of that for loop.

var regItemDeps = registry[dependencies[0]].dependencies;
for ( var n = 0, regItemDepLen = regItemDeps.length; n < regItemDepLen; n++ ) {
	dependencies.push(regItemDeps[n]);
}

Though I've already mentioned how while awkwardly phrased, JS already has the functionality to append an array onto the end of another array without doing an inefficient loop like this in the first place.

For loops don't have their own context, there's no difference between

var foo = 'bar', len = 10;
for ( var n = 0; n < len; n++ ) {

}

and

var foo = 'bar';
for ( var n = 0, len = 10; n < len; n++ ) {

}

I agree that looking at what it actually does though, we don't need the loop at all actually (originally introduced in r70220, merged to trunk in r72349). But I don't think we need concatenation or appending either, since we're not actually dealing with two arrays. We're dealing with one array and one string (which for the purpose of appending was converted to an array (dependencies = [dependencies])).

The simplest solution is probably to clone the dependencies array and unshift the string to the beginning of it.

 				var modulename = dependencies;
 				if ( modulename in registry ) {
 					// If called with a single module name, get it's dependencies as well
					dependencies = registry[dependencies[0]].dependencies.slice( 0 ); // clone, keep original object in tact
					dependencies.unshift( modulename );
 				}

What do you think ?

Sure, though you'll want to include an } else { dependencies = [modulename]; } or } else { dependencies = []; } (I'm not sure which makes more sense in the context of the code, the former is functionally equiv to the previous code, but if having that there means extra calls are made for a module we already know will be ignored the latter makes more sense.

Just saying to make sure that the dependencies variable always ends up as an array as the current code does.

I know that variables are function scoped not block scoped (well, ;) unless you use let, but we don't need to go there). I just said it makes more sense semantically for the code being written to put the len inside the for () loop because while the var is technically function scoped, so is n and they are both only relevant inside the context of the for() loop.

For loops don't have their own context, there's no difference between

var foo = 'bar', len = 10;
for ( var n = 0; n < len; n++ ) {
 
}

and

var foo = 'bar';
for ( var n = 0, len = 10; n < len; n++ ) {
 
}

I agree that looking at what it actually does though, we don't need the loop at all actually (originally introduced in r70220, merged to trunk in r72349). But I don't think we need concatenation or appending either, since we're not actually dealing with two arrays. We're dealing with one array and one string (which for the purpose of appending was converted to an array (dependencies = [dependencies])). The simplest solution is probably to clone the dependencies array and unshift the string to the beginning of it.

 	var modulename = dependencies;
 	if ( modulename in registry ) {
		// If called with a single module name, get it's dependencies as well
		dependencies = registry[modulename].dependencies.slice( 0 ); // clone, keep original object in tact
		dependencies.unshift( modulename );
	}

What do you think ?