# Sorting an List of tiles based on size and row limitation

We are using the below code to optimally display our menu items (metro style). We are looping through an array of menu items and automatically shifting them based on size.

Our tiles could be a size of 1, 2, 3. The row length is 6

Could you help me optimize the code?

private void SortTiles(ref List<MenuItems> t)
{
int rowSize = 0;

while (t.Count > 0)
{
if (6 - rowSize < t[0].Size) //Too Big
{
bool suitableTileFound = false;

for (int j = 1; j < t.Count; j++) //Go find smaller tile
{
if (6 - rowSize >= t[j].Size) //Use this one
{
suitableTileFound = true;
rowSize += t[j].Size;
t.RemoveAt(j);
break;
}
}
if (!suitableTileFound)
rowSize = 0;
}
else
{
rowSize += t[0].Size;
t.RemoveAt(0);
}

}
t = n;
}


MenuItem class

public class MenuItems
{
public int MenuId { get; set; }

public string MenuTitle { get; set; }

public string MenuUrl { get; set; }

public int SuiteId { get; set; }

public string Target { get; set; }

public int? Count { get; set; }

public int Size { get; set; }
}

-

Magic numbers
You use 6 - rowSize and Mr.Maintainer has no idea what the 6 is standing for. Extract it to a meaningful named constant.

Naming
MenuItems seems to be plural of MenuItem, so what is a List<MenuItems> ?
One letter parameter names for counter/index fields are ok, but for a List<T> you should use something else neither t nor n.

Brackets {}
You should always use brackets for if statements at least if you use a new line for the action to take.

Refactoring
Let us name the parameter and local field in a better way and like already told, we should refactor the magic number to a const. As of the missing context this needs to be renamed by you later

private void SortTiles(ref List<MenuItems> menuItems)
{
const int magicNumber6 = 6;
int rowSize = 0;
}


Instead of using a while() loop, we should use a for loop, so we can skip the two calls to menuItems.RemoveAt() and just call menuItems.Clear() after the loop.

Next we should refactor the magicNumber6 - rowSize outside of the inner loop, as this needs to be calculated only once for the inner loop. Next a small adjustment of the inner loop startindex to int j = i + 1 and we have this

private void SortTiles(ref List<MenuItems> menuItems)
{
const int magicNumber6 = 6;
int rowSize = 0;

{
int minimumSize = magicNumber6 - rowSize;
{
bool suitableTileFound = false;

for (int j = 1; j < menuItems.Count; j++)
{
{
suitableTileFound = true;
break;
}
}
if (!suitableTileFound)
{
rowSize = 0;
}
}
else
{
}

}
}


but wait, we can still do better. We can use the List<T>.Find() method to remove the inner loop at all and while we do this we can also get rid of some of the fields we had used and as change the const to the correct name maxRowSize

private void SortTiles(ref List<MenuItems> menuItems)
{
const int maxRowSize = 6;
int rowSize = 0;

{
int minimumSize = maxRowSize - rowSize;
{
{
}
else
{
rowSize = 0;
}
}
else
{
}
}
}


but what if we need to adjust the maxRowSize...? Let us pass it as a parameter then. Because we don't want now to change the code using this method, we add an overloaded method also.

private void SortTiles(ref List<MenuItems> menuItems)
{
const int maxRowSize = 6;

}


private void SortTiles(ref List<MenuItems> menuItems,int maxRowSize)
{
int rowSize = 0;

{
int minimumSize = maxRowSize - rowSize;
{

{
}
else
{
rowSize = 0;
}
}
else
{
}

}
}

-
Thanks for you in depth review! This is definitely an improvement – Stingervz Aug 20 '14 at 11:30
@Josefvz, no, not really as it is not producing the same results. I will edit my answer. – Heslacher Aug 20 '14 at 11:31
Well i haven't check it yet, so thanks for the answer anyway, – Stingervz Aug 20 '14 at 11:32

In a sentence, "Don't roll your own."

If MenuItems had a default comparer using Size, you could use List's built in sort, which will be much better and faster than rolling your own.

Except from List Sort Documentation:

This method uses the Array.Sort method, which applies the introspective sort as follows:

• If the partition size is fewer than 16 elements, it uses an insertion sort algorithm.

• If the number of partitions exceeds 2 * LogN, where N is the range of the input array, it uses a Heapsort algorithm.

• Otherwise, it uses a Quicksort algorithm.

-
It looks like an another answer was posted while I was typing this, so if you're determined to roll your own, please see that answer. – RubberDuck Aug 20 '14 at 11:26