Refactoring messy for loop

Questions & Answers
1

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 :slight_smile:
I hope someone has some pointers that will lead me down a better coding path :slight_smile:
Thanks in advance!

2

Wow, it’s kinda hard to know what’s going on there :slight_smile: 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… :stuck_out_tongue: 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 :smiley: