Sorting an List of tiles based on size and row limitation

Question

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;
        var n = new List<MenuItems>();

        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;
                        n.Add(t[j]);
                        rowSize += t[j].Size;
                        t.RemoveAt(j);
                        break;
                    }
                }
                if (!suitableTileFound)
                    rowSize = 0;
            }
            else
            {
                //Add normally
                n.Add(t[0]);
                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 List<MenuItems> SubMenuItems { get; set; }

    public int Size { get; set; }
}

Answer 1

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;
    var sortedMenuItems = new List<MenuItems>();
}  

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;
    var sortedMenuItems = new List<MenuItems>();

    while (menuItems.Count > 0)
    {
        int minimumSize = magicNumber6 - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize < menuItem.Size)
        {
            bool suitableTileFound = false;

            for (int j = 1; j < menuItems.Count; j++)
            {
                menuItem = menuItems[j];
                if (minimumSize >= menuItem.Size)
                {
                    suitableTileFound = true;
                    sortedMenuItems.Add(menuItem);
                    rowSize += menuItem.Size;
                    menuItems.RemoveAt(j);
                    break;
                }
            }
            if (!suitableTileFound)
            {
                rowSize = 0;
            }
        }
        else
        {
            sortedMenuItems.Add(menuItem);
            rowSize += menuItem.Size;
            menuItems.RemoveAt(0);
        }

    }
    menuItems = sortedMenuItems;
}  

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;
    var sortedMenuItems = new List<MenuItems>();

    while (menuItems.Count > 0)
    {
        int minimumSize = maxRowSize - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize < menuItem.Size)
        {
            menuItem = menuItems.Find(item => minimumSize >= item.Size);
            if (menuItem != null)
            {
                sortedMenuItems.Add(menuItem);
                rowSize += menuItem.Size;
                menuItems.Remove(menuItem);
            }
            else
            {
                rowSize = 0;
            }
        }
        else
        {
            sortedMenuItems.Add(menuItem);
            rowSize += menuItem.Size;
            menuItems.RemoveAt(0);
        }
    }
    menuItems = sortedMenuItems;
}  

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;

    SortTiles(ref menuItems, maxRowSize);
}  

---

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

    while (menuItems.Count > 0)
    {
        int minimumSize = maxRowSize - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize < menuItem.Size)
        {
            menuItem = menuItems.Find(item => minimumSize >= item.Size);

            if (menuItem != null)
            {
                sortedMenuItems.Add(menuItem);
                rowSize += menuItem.Size;
                menuItems.Remove(menuItem);
            }
            else
            {
                rowSize = 0;
            }
        }
        else
        {
            sortedMenuItems.Add(menuItem);
            rowSize += menuItem.Size;
            menuItems.RemoveAt(0);
        }

    }
    menuItems = sortedMenuItems;
}

Answer 2

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.