HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Sorting an List of tiles based on size and row limitation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sortingsizetilesbasedlimitationandlistrow

Problem

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 t)
    {
        int rowSize = 0;
        var n = new List();

        while (t.Count > 0)
        {
            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 SubMenuItems { get; set; }

    public int Size { get; set; }
}

Solution

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 ?

One letter parameter names for counter/index fields are ok, but for a List 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)
{
    const int magicNumber6 = 6;
    int rowSize = 0;
    var sortedMenuItems = new List();
}


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)
{
    const int magicNumber6 = 6;
    int rowSize = 0;
    var sortedMenuItems = new List();

    while (menuItems.Count > 0)
    {
        int minimumSize = magicNumber6 - rowSize;
        MenuItems menuItem = menuItems[0];
        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.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)
{
    const int maxRowSize = 6;
    int rowSize = 0;
    var sortedMenuItems = new List();

    while (menuItems.Count > 0)
    {
        int minimumSize = maxRowSize - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize  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)
{
    const int maxRowSize = 6;

    SortTiles(ref menuItems, maxRowSize);
}


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

    while (menuItems.Count > 0)
    {
        int minimumSize = maxRowSize - rowSize;
        MenuItems menuItem = menuItems[0];
        if (minimumSize  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;
}

Code Snippets

private void SortTiles(ref List<MenuItems> menuItems)
{
    const int magicNumber6 = 6;
    int rowSize = 0;
    var sortedMenuItems = new List<MenuItems>();
}
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;
}
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;
}
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;
}

Context

StackExchange Code Review Q#60563, answer score: 13

Revisions (0)

No revisions yet.