patterncsharpModerate
Sorting an List of tiles based on size and row limitation
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?
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 classpublic 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
Naming
One letter parameter names for counter/index fields are ok, but for a
Brackets {}
You should always use brackets for
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
Instead of using a
Next we should refactor the
but wait, we can still do better. We can use the
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.
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 thisprivate 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.