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.

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.