4
\$\begingroup\$

In my app, I have this code to display different tutorials about OneNote. This clears and resets certain values, and updates some variables so we know which version we are working with. The main problem is it has a lot of repetition in the three ifs, although all comments are welcomed.

private void SwitchVersion(string version)
{
    // This programatically selects the top value that is always the same
    // to prevent crashes.  Items is a ListBox.
    Items.SelectedValue = "OneNote";

    // This clears all values except the top value.
    // Data.ItemList is an ObservableCollection<string> bound to Items.
    while (Data.ItemList.Count > 1)
    {
        Data.ItemList.RemoveAt(1);
    }

    // This clears the ObservableCollection<Type> of XAML pages navigated to with
    // the ListBox.  These values are tied to the values in ItemList.
    Data.Pages.Clear();

    if (version == "Phone")
    {
        foreach (string s in GlobalVars.PHome())
        {
            if (s == "OneNote") 
            {
                continue;
            }
            Data.ItemList.Add(s);
        }

        foreach (Type t in GlobalVars.PHomeP())
        {
            Data.Pages.Add(t);
        }

        // This is the method called when a value in the ListBox is chosen.
        // We need to call this to update some submenus.
        Data.NewSelectionP("OneNote");
    }

    if(version == "WStore")
    {
        foreach (string s in GlobalVars.Home())
        {
            if (s == "OneNote") continue;
            Data.ItemList.Add(s);
        }

        foreach (Type t in GlobalVars.HomeP()) Data.Pages.Add(t);
        Data.NewSelectionWS("OneNote");
    }

    if (version == "2013")
    {
        foreach (string s in GlobalVars.Home2013())
        {
            if (s == "OneNote") continue;
            Data.ItemList.Add(s);
        }

        foreach (Type t in GlobalVars.HomeP2013()) Data.Pages.Add(t);
        Data.NewSelection2013("OneNote");
    }

    // This is the Frame used to display the data.
    DataFrame.Navigate(Data.Pages[Items.SelectedIndex]);
}
\$\endgroup\$
0

2 Answers 2

2
\$\begingroup\$

Each Version should be its own Method that is called by this one and not nested inside an if statement. Granted there should be a decision made, but then the program should call the SwitchToPhoneVersion Method or one of the other Methods

\$\endgroup\$
0
2
\$\begingroup\$
while (Data.ItemList.Count > 1)
{
    Data.ItemList.RemoveAt(1);
}

I'm not exactly sure what ItemList is, but I would implement a Clear method to do this. If it's not your code, write an extension method.

As for your actual question, it's very similar to the switch smell and could probably be solved the same way.

if (version == "Phone")
if(version == "WStore")
if (version == "2013")

Sometimes you're taking an identical action, sometimes your not. So, you could definitely start with extracting the duplicated logic out into a private method, but I would also consider a better OOP approach.

I don't really have enough context to show you how I would go about that, but the general gist is you should have a base class that implements the "default" behavior of your class. Then, you would implement a child class for each each of these different versions. The child class would override the appropriate behavior.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.