Populate ItemList

In my app, I have an ObservableCollection of MenuItems:

public ObservableCollection ItemList { get; set; }


When an item in it is selected (either programatically or by the user), this method is run:

`public void SelectionChangedWS()
{
if (!NavButtonUsed) { Back.Insert(0, CurrentItem); }

if (Forward.Count != 0 && !NavButtonUsed) { Forward.Clear(); }

if (CurrentItem.Title.StartsWith(" ")) { return; }

// Remove menu items to close menu - title starts with space
ItemList.RemoveAll(item => item.Title.StartsWith(" "));

switch (CurrentItem.Menu)
{
case Menus.WSOneNote:
ItemList.Insert(1, MenuItem.CreateMenuItem(resourceFile.GetString("WSNotes"), typeof(WindowsData.Submenus.Notes), Menus.WSOneNote));
ItemList.Insert(1, MenuItem.CreateMenuItem(resourceFile.GetString("WSPages"), typeof(WindowsData.Submenus.Pages), Menus.WSOneNote));
ItemList.Insert(1, MenuItem.CreateMenuItem(resourceFile.GetString("WSSections"), typeof(WindowsData.Submenus.Section), Menus.WSOneNote));
ItemList.Insert(1, MenuItem.CreateMenuItem(resourceFile.GetString("WSNotebooks"), typeof(WindowsData.Submenus.Notebook), Menus.WSOneNote));
break;

case Menus.WSMainMenu:
ItemList.Insert(2, MenuItem.CreateMenuItem(resourceFile.GetString("WSDraw"), typeof(WindowsData.Submenus.Draw), Menus.WSMainMenu));
ItemList.Insert(2, MenuItem.CreateMenuItem(resourceFile.GetString("WSUndo"), typeof(WindowsData.Submenus.Undo), Menus.WSMainMenu));
ItemList.Insert(2, MenuItem.CreateMenuItem(resourceFile.GetString("WSTag"), typeof(WindowsData.Submenus.Tag), Menus.WSMainMenu));
ItemList.Insert(2, MenuItem.CreateMenuItem(resourceFile.GetString("WSPaste"), typeof(WindowsData.Submenus.Paste), Menus.WSMainMenu));
ItemList.Insert(2, MenuItem.CreateMenuItem(resourceFile.GetString("WSList"), typeof(WindowsData.Submenus.List), Menus.WSMainMenu));
It

Extract repeated code into methods.

private void Insert(int index, string key, Type type, Menus menu)
{
  ItemList.Insert(index, MenuItem.CreateMenuItem(resourceFile.GetString(key), type, menu));
}

Now the calling code is simpler and the differences stand out.

Insert(1, "WSNotes", typeof(WindowsData.Submenus.Notes), Menus.WSOneNote);
Insert(1, "WSPages", typeof(WindowsData.Submenus.Pages), Menus.WSOneNote);

You trimmed off code from code that already is a bit long and repetitive. That might mean it would be better to statically define the relevant parts in a dictionary (or some other more structured format). This way, you could just access the structured definition by the enum value, then write one bit of code that is common and can generate the menu items based on the definition format.

Why are you repeatedly inserting into the same index? It seems like you want them grouped together, but don't care about what the actual index is inside of the collection. It is also confusing that the order will end up reversed compared to how they are inserted in the code. Just calling Add() will append values for you and the order will match the code.

Unless there is some other logic manipulating this collection, calling this method with repeatedly with two different values of CurrentItem.Menu will result in the first group of items being broken up.

Result of first call with WSOneNote:

0 = //something? this code never indicates why this index is not used.
1 = "WSNotebooks"
2 = "WSSections"
3 = "WSPages"
4 = "WSNotes"

Result after next call with WSMainMenu:

0 = //something? this code never indicates why this index is not used.
1 = "WSNotebooks"
2 = "WSPicture"
// ... other items
10 = "WSSections"
11 = "WSPages"
12 = "WSNotes"

Unless there is other information we don't have, the RemoveAll() might not remove anything. If it does remove everything, the inserts will fail because the given index would not be valid.

It is not clear at all what this collection is being used for or why the insertion index matters.

Assuming there is some significance to the insertion index, it would be better to have them defined as named constants instead of magic numbers in the code.

Putting an if statement on one line only decreases readability.

StackExchange Code Review Q#79209, answer score: 3