Go Back to Previous Item

Question

I have a Back function, and it is growing. I also think this is terrible, but I don't seem to get how it can be improved:

public void GoBack()
{
 NavButtonUsed = true;
 Forward.Insert(0, Back[0]);
 Back.RemoveAt(0);
 if (Back[0].Title.StartsWith(" ") && CurrentItem.Menu != Back[0].Menu)
 {
 switch (Back[0].Menu)
 {
 case Menus.WSOneNote:
 CurrentItem = ItemList[0];
 break;
 case Menus.WSMainMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("MainMenu"), typeof(WindowsData.MainMenu), Menus.WSMainMenu))];
 break;
 case Menus.WSTextMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("TextMenu"), typeof(WindowsData.TextMenu), Menus.WSTextMenu))];
 break;
 case Menus.WSTextBlockMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("TextBlockMenu"), typeof(WindowsData.TextBlockMenu), Menus.WSTextBlockMenu))];
 break;
 case Menus.WSTableMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("TableMenu"), typeof(WindowsData.TableMenu), Menus.WSTableMenu))];
 break;
 case Menus.WSTableCellsMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("TableCellsMenu"), typeof(WindowsData.TableCellsMenu), Menus.WSTableCellsMenu))];
 break;
 case Menus.WSDrawMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("DrawMenu"), typeof(WindowsData.DrawMenu), Menus.WSDrawMenu))];
 break;
 case Menus.WSDrawnItemsMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("DrawnItemsMenu"), typeof(WindowsData.DrawnItemsMenu), Menus.WSDrawnItemsMenu))];
 break;
 case Menus.WSPictureMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("PictureMenu"), typeof(WindowsData.PictureMenu), Menus.WSPictureMenu))];
 break;
 case Menus.WSFileMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("FileMenu"), typeof(WindowsData.File), Menus.WSFileMenu))];
 break;
 case Menus.WSAppBarsMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("AppBarsMenu"), typeof(WindowsData.AppBar), Menus.WSAppBarsMenu))];
 break;
 case Menus.WSCharmsMenu:
 CurrentItem = ItemList[ItemList.IndexOf(MenuItem.CreateMenuItem(resourceFile.GetString("CharmsMenu"), typeof(WindowsData.Charms), Menus.WSCharmsMenu))];
 break;
 default:
 break;
 }
 }
 CurrentItem = ItemList[ItemList.IndexOf(Back[0])];
 NavButtonUsed = false;
}

I have more menus to account for, but I haven't implemented going back to them yet as I do not have them fully implemented. What is really going on here is I need to programmatically select a menu to open it so I can select a menu item within it. If I do not open the menu, ItemList will not have CurrentItem, so the app will crash.

This is CurrentItem:

private MenuItem _currentItem = new MenuItem(resourceFile.GetString("OneNote"), typeof(WindowsData.OneNote), Menus.WSOneNote);
public MenuItem CurrentItem
{
 get { return _currentItem; }
 set
 {
 if (value == _currentItem) { return; }
 _currentItem = value;
 OnPropertyChanged();
 }
}

ItemList, Back, and Forward are just ObservableCollections of MenuItems (they can't be Lists because they are bound to XAML objects that need to know when they are updated).

Answer 1

Your GoBack() method could be simplified in its current pattern to

public void GoBack()
{
 NavButtonUsed = true;
 Forward.Insert(0, Back[0]);
 Back.RemoveAt(0);
 CurrentItem = ItemList[ItemList.IndexOf(Back[0])];
 NavButtonUsed = false;
} 

The whole switch..case is senseless, because the only thing it is doing is triggering the OnPropertyChanged() event twice.

Inside the switch..case you are assigning a MenuItem to the CurrentItem property (triggering an OnPropertyChanged() event) and you are breaking out of the switch.

Then you are assigning ItemList[ItemList.IndexOf(Back[0])] to the CurrentItem property (triggering an OnPropertyChanged() event), so you are just overwriting the property.

So, if this method works like intended, the switch..case can be removed.

• user34073: This won't work because the menu is closed and CurrentItem is not in ItemList. If I call CurrentItem = ItemList[ItemList.IndexOf(Back[0])]; without opening the menu first, the app will crash. I specified this in my question.
• Heslacher: What type is Back ? I assume a List<MenuItem>.
• user34073: Well, it is an ObservableCollection because it is bound to the Back button's visibility.
• Heslacher: ObservableCollection of MenuItem ?
• user34073: Yes. Maybe I should add the all the calls to Back into the question?

Answer 2

instead of

get { return _currentItem; }
set
{
 if (value == _currentItem) { return; }
 _currentItem = value;
 OnPropertyChanged();
}

Where you are checking whether the _currentItem is equivalent to the value that you are trying to set it to, you should do the opposite and make it less cluttered.

set
{
 if (!(value == _currentItem))
 {
 _currentItem = value;
 OnPropertyChanged();
 }
 }

This is the exact same except it is straightforward, "change the value if it's not the same"

---

I also don't like your get statement, I would write it like this

get
{
 return _currentItem;
}

or if you are going for compact, you could also write the get statement like this.

get;

Some people don't like this though.