koobas.hobune.stream
Refactoring messy for loop
Hi, I am wondering, and have been reading up on refactoring some of my messier code. Right now I am looking into making for loops better and/or more readable.
The example I have here is as method in which i try to figure out a lot of things before selecting the right item(which I also will want to split up into several methods. However for the time being, I am trying to change the way i tackle the issue, rather than what I am doing right now which is clearly not very nice.
for(int p = pBlock.width-1; p<= pBlock.width+1; p++)
{
for(int p2 = pBlock.length-1; p2<= pBlock.length+1; p2++)
{
if(p < 0 ) continue; // not outside on one end
if(p2 < 0) continue;
if(p >= interactBlockLength0) continue;
if(p2 >= interactBlockLength1) continue;
if(p != pBlock.width && p2 != pBlock.length)continue; //not diagonal
if(bControl.interactableBlockStates[p, p2] != 0 && !bControl.selectedBlocks.Contains(bControl.interactableBlocks[p,p2]) ) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 4) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 0) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 2) continue;
if(bControl.selectedBlocks.Contains(bControl.interactableBlocks[pBlock.width, pBlock.length])) continue;
if(uCon.movingStates == UnitController.MovingStates.SELECTING_UNIT)
{
bControl.AddSelected(bControl.interactableBlocks[pBlock.width, pBlock.length]);
expectedCost += GameController.instance.gameSettings.blockCost;
cmPControl.UpdateCommandPoints();
return;
}
}
}
So I have read a lot up on on using LINQ to make it "nicer", but I am having a hard time actually making it work, so I am turning here :) I hope someone has some pointers that will lead me down a better coding path :) Thanks in advance!
$$anonymous$$aybe I'm missing something, but line 10 seems to be excluding everything in your loops except for a single item - the one at pBlock.width, pBlock.length. This is backed up by lines 14-17 and 21 which are only looking at that same item no matter how many times you're looping.
If that's true, why do you even need the loops?
It is actually so you cannot select the blocks that are diagonal to the current block(pBlock) :)
Ha! Line 10 is certainly tricky -- the continue is like an extra not. I usually rewrite things like that as: bool adjacent= (p1==width || p2==length); if(!adjacent) continue;.
Then, it's four tests at most, right (possibly 4 adjacent blocks)? Sometimes it's clearer just to write a "compareBlocks" function and explicitly call it with (x+1,y) (x-1,y) (x,y-1) (x,y+1).
But that's all just generic program$$anonymous$$g. Nothing special to do with Unity. UA isn't really a code review site.
Wow, it's kinda hard to know what's going on there :) I recommend more descriptive names for the variables, and maybe a description of what each of them does. uCon, bControl, without any context at all it's a real puzzle to analyze (thank you btw), and calling the width and length of something p and p2 instead of wid and len or horiz and verti... :P I think I've got it though...
At first I noticed that you could optimize considerably with this:
int p = pBlock.width-1;
int p2 = pBlock.length-1;
if((p < 0)) // 1 if sentence instead of 9
{
p = pBlock.width;
}
if((p2 < 0)) // 1 if sentence instead of up to 9
{
p2 = pBlock.length;
}
for( ; p<= pBlock.width+1; p++)
{
for( ; p2<= pBlock.length+1; p2++)
{
// if(p < 0 ) continue; // Replaced by first if sentence
// if(p2 < 0) continue; // Replaced by second if sentence
if(p >= interactBlockLength0) continue;
if(p2 >= interactBlockLength1) continue;
-------------------
But when I realized you were going through 9 pairs just to throw 4 of them away, I had to move things around again and came up with:
int p;
int p2 = pBlock.length;
for(p = pBlock.width-1; p<= pBlock.width+1; p++)
{
if(p>=0)
{
if(p >= interactBlockLength0) continue;
if(p2 >= interactBlockLength1) continue;
if(bControl.interactableBlockStates[p, p2] != 0 && !bControl.selectedBlocks.Contains(bControl.interactableBlocks[p,p2]) ) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 4) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 0) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 2) continue;
if(bControl.selectedBlocks.Contains(bControl.interactableBlocks[pBlock.width, pBlock.length])) continue;
if(uCon.movingStates == UnitController.MovingStates.SELECTING_UNIT)
{
bControl.AddSelected(bControl.interactableBlocks[pBlock.width, pBlock.length]);
expectedCost += GameController.instance.gameSettings.blockCost;
cmPControl.UpdateCommandPoints();
return;
}
}
}
p = pBlock.width;
for( p2 = pBlock.length-1; p2<= pBlock.length+1; p2 += 2)
{
if(p2>=0)
{
if(p >= interactBlockLength0) continue;
if(p2 >= interactBlockLength1) continue;
if(bControl.interactableBlockStates[p, p2] != 0 && !bControl.selectedBlocks.Contains(bControl.interactableBlocks[p,p2]) ) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 4) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 0) continue;
if(bControl.interactableBlockStates[pBlock.width,pBlock.length] == 2) continue;
if(bControl.selectedBlocks.Contains(bControl.interactableBlocks[pBlock.width, pBlock.length])) continue;
if(uCon.movingStates == UnitController.MovingStates.SELECTING_UNIT)
{
bControl.AddSelected(bControl.interactableBlocks[pBlock.width, pBlock.length]);
expectedCost += GameController.instance.gameSettings.blockCost;
cmPControl.UpdateCommandPoints();
return;
}
}
}
And I can tell it can be further optimized, but it's already much more efficient (hard to tell how much, since iterations stop at different continues and I have no idea what bControl.interactableBlockStates is all about), but completely bypassing the 4 diagonals has to count for something :D
