[pcaddict]

I have a program that requests and processes historical weather data for a location between a date range. I have implemented the most important pieces and thought it would be a good time and use for Tasks. The problem I am running into is that when wrapped in a task, the returned data gets added to my listbox in an unexpected manner. The number of returned results is correct but it appears that the date is not incrementing between the time the task finishes and the next time it is called. Without using tasks, the returned data is correct. Putting a break point in for the task, curDate does increment like it is supposed to. I don't seem to be able to see inside of the task so I can't tell what data is being passed through the task during run time. I am thinking that the task's result is pointing to the wrong instance of the returned WxData but don't know if that even makes sense.

I have attached a screen shot to show exactly what is happening and here is the code.

namespace Wunderground
{
    public partial class Form1 : Form
    {
        private TaskScheduler scheduler = null;
        
        public Form1()
        {
            InitializeComponent();

            this.scheduler = TaskScheduler.FromCurrentSynchronizationContext();
        }

        private string apiKey = "my key";
        private int numberOfDays = 0;
        //private List<WxData> historicWeather = new List<WxData>();

        public DateTime StartDate
        {
            get { return this.dateTimePicker1.Value; }
        }

        public DateTime EndDate
        {
            get { return this.dateTimePicker2.Value; }
        }

        private Uri GenerateHistoricURI(string apiKey, DateTime date)
        {
            string requestUri = String.Format("http://api.wunderground.com/api/{0}/history_{1}/q/MGE.json", apiKey, date.ToString("yyyyMMdd"));
            
            return new Uri(requestUri);
        }

        private WxData GetWeatherForDate(DateTime date)
        {
            Uri uriForDate = GenerateHistoricURI(apiKey, date);
            string rawWxData = GetJson(uriForDate);
            WxData wxForDate = ParseWeatherInfo(rawWxData, date);
            //historicWeather.Add(wxForDate);

            return wxForDate;
        }

        private int GetNumberOfDays()
        {
            TimeSpan ts = EndDate - StartDate;

            return ts.Days;
        }

        private string GetJson(Uri uri)
        {
            string json = String.Empty;
            using (WebClient wc = new WebClient())
            {
                json = wc.DownloadString(uri);
            }

            return json;
        }

        private WxData ParseWeatherInfo(string jsonString, DateTime curDate)
        {
            //Units in C
            int maxTempIndex = jsonString.IndexOf("\"maxtempm\":") + "\"maxtempm\":".Length;
            int minTempIndex = jsonString.IndexOf("\"mintempm\":") + "\"mintempm\":".Length;
            
            //Get max and min temps. Regex would be less brute-force
            string maxTemp = jsonString.Substring(maxTempIndex, 4).Replace("\"", "").Replace(",", "");
            string minTemp = jsonString.Substring(minTempIndex, 4).Replace("\"", "").Replace(",", "");

            return new WxData(curDate, maxTemp, minTemp);
        }

        private void CreateCSV()
        {
            string path = Path.GetDirectoryName(Application.ExecutablePath);
            string fileName = @"\weather.csv";

            try
            {
                using (StreamWriter file = new StreamWriter(path + fileName))
                {
                    foreach (string wx in listBox1.Items)
                    {
                        file.WriteLine(wx);
                    }
                }
            }
            catch (IOException ioex)
            {
                MessageBox.Show(ioex.ToString());
            }
            finally
            {
                MessageBox.Show("Successfully wrote file to " + path);
            }
        }

        private void button1_Click(object sender, EventArgs e)
        {
            numberOfDays = GetNumberOfDays();
            DateTime curDate;

            for (int i = 0; i <= numberOfDays; i++)
            {
                curDate = StartDate.AddDays(i);
                //Pause after every 9 requests for 1 minute to comply with Weather Underground api TOS
                if (i % 10 == 9)
                {
                    //Find better way to pause execution
                    Thread.Sleep(60000);
                }
                else
                {
                    if (curDate <= EndDate)
                    {
                        Task.Factory.StartNew<WxData>(() =>
                            GetWeatherForDate(curDate)).ContinueWith((r) =>
                                this.listBox1.Items.Add(String.Format("{0},{1},{2}", r.Result.Date.Date, r.Result.MaxTemp, r.Result.MinTemp)),this.scheduler);
                        //WxData data = GetWeatherForDate(curDate);
                        //listBox1.Items.Add(String.Format("{0},{1},{2}", data.Date.Date, data.MaxTemp, data.MinTemp));
                    }
                }
            }
        }
    }
}

Any help, ideas, or comments on my implementation are welcome.

Thanks

Attached image(s)

• 

This post has been edited by pcaddict: 30 April 2012 - 08:25 AM

[pcaddict]

Since I left it out of my original post, the problem code is contained below line 101. Another curious behavior I noticed is that the results aren't being added to the listbox until the the loop finishes execution. My code for tasks was adapted from the Introduction to Tasks tutorial

[CodingSup3rnatur@l-360]

Hi,

I would combine the declaration and initialization of the currDate variable, and move its declaration so that it is inside the for loop, as so:

for (int i = 0; i <= numberOfDays; i++)
{
    DateTime curDate = StartDate.AddDays(i);
    ...
}

That should fix the problem with the dates display, of which was being caused by the way the C# compiled handles captured variables.

A good lesson to take away from this is to try not to extend the scope of variables when not necessary. Keep variable scopes as narrow as possible


As for the listbox not updating until the very end of the loop, you could extract the code in the button1 click event handler method into its own method (this is generally good practice when running code from event handler methods anyway), and run the method in a Task. This will keep the UI thread completely free, meaning it will be able to update the listbox, as well as process other events, which will in turn give the application a much more responsive feel

[pcaddict]

Thanks for the suggestions Sup3rnatur@l. I originally started with a method to call from the button click event and decided to go a different direction for some reason.

Per your suggestion I created a new method to call from the button's click event handler, which spawns the task to get and process the data. In the event handler I am spawning another (or initial?) task which calls the aforementioned method. Is this what you had in mind? I have been going over the TPL documentation on MSDN and am trying to determine the proper or recommended implementation.

I thought about returning a WxData object from my GetWeatherForDatesInRange method but couldn't get the intended execution with that.

private void GetWeatherForDatesInRange(int numberOfDays)
        {
            for (int i = 0; i <= numberOfDays; i++)
            {
                DateTime curDate = StartDate.AddDays(i);

                //Pause after every 9 requests for 1 minute to comply with Weather Underground api TOS
                if (i % 10 == 9)
                {
                    //Find better way to pause execution
                    Thread.Sleep(60000);
                }

                if (curDate <= EndDate)
                {
                    Task.Factory.StartNew<WxData>(() =>
                        this.GetWeatherForDate(curDate)).ContinueWith((r) =>
                            this.listBox1.Items.Add(String.Format("{0},{1},{2}", r.Result.Date, r.Result.MaxTemp, r.Result.MinTemp)), this.scheduler);
                }
            }
        }

        private void button1_Click(object sender, EventArgs e)
        {
            numberOfDays = GetNumberOfDays();

            Task.Factory.StartNew(() => GetWeatherForDatesInRange(numberOfDays));
        }

Again, the above code works, I am just looking for the "right" way to do it.