AS3’s got some awkward rules about variable scope, due to its use of hoisting. I don’t like pointlessly leaving a bunch of compiler warnings lying around, but it’s more important to me for my code to readable and properly written, regardless. This produces a couple of duplicate variable definition warnings:
private function arrangeElementsLine(pElementSize:Dimensions):void
{
if (m_eOrientation == HORIZONTAL)
{
for (var i:uint = 0; i < target.numElements; i++)
{
var layoutElement:ILayoutElement = target.getElementAt(i);
layoutElement.setLayoutBoundsSize(pElementSize.x, pElementSize.y);
layoutElement.setLayoutBoundsPosition(pElementSize.x * i, 0);
}
}
else
{
for (var i:uint = 0; i < target.numElements; i++)
{
var layoutElement:ILayoutElement = target.getElementAt(i);
layoutElement.setLayoutBoundsSize(pElementSize.x, pElementSize.y);
layoutElement.setLayoutBoundsPosition(0, pElementSize.y * i);
}
}
}
Is it generally considered to be better and more conventional to remove the redundant var and :<type> syntax for i and layoutElement in the second for loop, or is it more proper to just leave them in there (due to the way their usage is isolated to those two for loops)? Thanks!
EDIT
This was asked a while back, but when I asked this, part of what I was working on was making something run more efficiently; so I put if outside of any for statements and avoided method calls inside the for statements.
1
Instead of using the same variable, make them unique by putting more of their purpose into their names:
var horizontalLayoutElement:ILayoutElement = target.getElementAt(i);
...
var verticalLayoutElement:ILayoutElement = target.getElementAt(i);
or if you don’t like such long variable names:
var hLayoutElement:ILayoutElement = target.getElementAt(i);
...
var vLayoutElement:ILayoutElement = target.getElementAt(i);
You can also emulate hoisting by putting all your variables declarations at the top of your functions, but since this makes code much harder to read due to the distance in code between declaration and usage, I would avoid it.
You have a lot of duplicate code here, which defeats the DRY principle. This would be better:
private function arrangeElementsLine(pElementSize:Dimensions):void {
for (var i:uint = 0; i < target.numElements; i++) {
var layoutElement:ILayoutElement = target.getElementAt(i);
layoutElement.setLayoutBoundsSize(pElementSize.x, pElementSize.y);
if (m_eOrientation == HORIZONTAL) {
layoutElement.setLayoutBoundsPosition(pElementSize.x * i, 0);
} else {
layoutElement.setLayoutBoundsPosition(0, pElementSize.y * i);
}
}
}
This also solves your issues with variable duplication.
1
Personally, I prefer reuse the sames variables when they are exactly the same. As the content is the same and the different loop don’t stuck each other by overriding the variables, reusing the variables will resolve your problem of warning without making the code less readable, and, in case of pure optimization, it will take less memory (even if 4 bytes is nothing, it’s 4 bytes lighter anyway).
So, personally, I prefer using a function like that:
private function arrangeElementsLine(pElementSize:Dimensions):void
{
var i:uint;
var layoutElement:ILayoutElement;
if (m_eOrientation == HORIZONTAL)
{
for (i = 0; i < target.numElements; i++)
{
layoutElement = target.getElementAt(i);
layoutElement.setLayoutBoundsSize(pElementSize.x, pElementSize.y);
layoutElement.setLayoutBoundsPosition(pElementSize.x * i, 0);
}
}
else
{
for (i = 0; i < target.numElements; i++)
{
layoutElement = target.getElementAt(i);
layoutElement.setLayoutBoundsSize(pElementSize.x, pElementSize.y);
layoutElement.setLayoutBoundsPosition(0, pElementSize.y * i);
}
}
}