My name
is
Jon Skeet

Reading a random entry from XML in Windows Phone

I'm writing my first app for Windows Phone and I encountered a problem. I've created a button that is supposed to generate a random entry from my XML on the textblock. The XML itself looks like this:

  <Activities>

    <Activity Type="Cooking">
      Cook some Mexican food
    </Activity>

    <Activity Type="Art">
      Draw a picture
    </Activity>

    <Activity Type="Language">
      Learn 5 words in Polish you didn't know yet
    </Activity>

    <Activity Type="Science">
      Write a simple program in C
    </Activity>

    <Activity Type="Sport">
      Go to the swimming pool
    </Activity>

  </Activities>

However there is a problem. When I press a button it sometimes generates random entry, but sometimes it just prints a "Type" of an entry, and sometimes both entry and the type. How can I prevent this from happening? I still want to load a Type of an entry for future functionality, but I don't want it to be displayed in a textblock. Here is my code:

Class loading an XML file:

namespace ReadXMLfromFile
{
    public class Activities
    {
        private List<String> activities;

        public String getActivities(int i)
        {
            return activities.ElementAt(i);

        }

        public List<String> getActivities()
        {
            return activities;
        }

        public Activities()
        {

            IsolatedStorageFile ISF = IsolatedStorageFile.GetUserStoreForApplication();

            activities = new List<String>();

            if (ISF.FileExists("Activities.xml"))
            {
                IsolatedStorageFileStream isoStream = new IsolatedStorageFileStream("Activities.xml", FileMode.Open, FileAccess.Read, FileShare.Read, ISF);

                var aclist = from c in XElement.Load(isoStream).Element("Activities").Elements()
                             select c;
                ISF.Dispose();
                isoStream.Dispose();

                foreach (var el in aclist)
                {
                    this.activities.Add(el.Value);

                    if ((string)el.Attribute("Type") == "Cooking")
                        this.activities.Add("Cooking");
                    else

                    if ((string)el.Attribute("Type") == "Art")
                         this.activities.Add("Art");
                    else

                    if ((string)el.Attribute("Type") == "Language")
                        this.activities.Add("Language");
                    else

                    if ((string)el.Attribute("Type") == "Science")
                        this.activities.Add("Science");
                    else

                    if ((string)el.Attribute("Type") == "Sport")
                        this.activities.Add("Sport");
                } 
             }
            else
            {
                ISF.Dispose();
                var aclist = from c in XElement.Load("Assets/Activities.xml").Element("Activities").Elements()
                             select c;

                foreach (var el in aclist)
                {
                    this.activities.Add(el.Value);

                    if ((string)el.Attribute("Type") == "Cooking")
                        this.activities.Add("Cooking");
                    else

                        if ((string)el.Attribute("Type") == "Art")
                            this.activities.Add("Art");
                        else

                            if ((string)el.Attribute("Type") == "Language")
                                this.activities.Add("Language");
                            else

                                if ((string)el.Attribute("Type") == "Science")
                                    this.activities.Add("Science");
                                else

                                    if ((string)el.Attribute("Type") == "Sport")
                                        this.activities.Add("Sport");
                } 
            }

        }
    }
}

And here is my MainPage class:

namespace ActivityManager
{
    public partial class MainPage : PhoneApplicationPage
    {

        private Activities ac;
        private Random activityNumber;
        private int actualActivityNumber;


        // Constructor
        public MainPage()
        {
            InitializeComponent();
            ac = new Activities();

        }


        private void RandomButton_Click(object sender, RoutedEventArgs e)
        {
            RandomTextBox.Text="0";
           activityNumber = new Random();
            actualActivityNumber = activityNumber.Next(0, ac.getActivities().Count);
            RandomTextBox.Text = ac.getActivities(actualActivityNumber);

        }
    }
}

Thank you in advance.

I would structure the code very differently:

Create a class for both the type and description
Don't use all those if statements - you're doing basically the same thing in every situation. You can always use a HashSet<string> for known types, if you want to skip other ones
Don't have two different bits of code which both read the same XML structure
Use a using statement for IsolatedStorageFile

So something like:

public class Activity
{
    public string Type { get; private set; }
    public string Description { get; private set; }

    public Activity(string type, string description)
    {
         Type = type;
         Description = description;
    }

    public static List<Activity> LoadActivities(XDocument doc)
    {
        return doc.Root
                  .Elements("Activity")
                  .Select(el => new Activity((string) el.Attribute("Type"),
                                             el.Value));
                  .ToList();
    }

    public static List<Activity> LoadActivities()
    {
        using (var store = IsolatedStorageFile.GetUserStoreForApplication())
        {
            if (store.FileExists("Activities.xml"))
            {
                 using (var stream = store.OpenFile("Activities.xml",
                                                    FileMode.Open))
                 {
                     return LoadActivities(XDocument.Load(stream));
                 }
            }
        }
        // No user file... load the default activities
        return LoadActivities(XDocument.Load("Assets/Activities.xml"));
    }
}

...

namespace ActivityManager
{
    public partial class MainPage : PhoneApplicationPage
    {
        private List<Activity> activities;
        private Random random;
        private int actualActivityNumber;


        // Constructor
        public MainPage()
        {
            InitializeComponent();
            activities = Activity.LoadActivities();
            // Initialize this *once* - not once per click
            random = new Random();
        }


        private void RandomButton_Click(object sender, RoutedEventArgs e)
        {
            int index = random.Next(activities.Count);
            RandomTextBox.Text = activities[index].Description;
        }
    }
}

See more on this question at Stackoverflow