开发者

How would I clean this up?

I'm working with an application that utilizes the Google Weather API, right now I got some real ugly code just to populate the 3 day forecast, which looks like so (remember no laughing)

groupBox3.Text = set.Forecast[1].DayOfTheWeek;
label4.Text = string.Format("High {0}", set.Forecast[1].High);
label3.Text = string.Format("Low: {0}", set.Forecast[1].Low);
label11.Text = string.Format("Condition: {0}", set.Forecast[1].Condition);
pictureBox1.Load(set.Forecast[1].Icon);

groupBox4.Text = set.Forecast[2].DayOfTheWeek;
label14.Text = string.Format("High {0}", set.Forecast[2].High);
label13.Text = string.Format("Low: {0}", set.Forecast[2].Low);
la开发者_如何学Pythonbel20.Text = string.Format("Condition: {0}", set.Forecast[2].Condition);
pictureBox2.Load(set.Forecast[2].Icon);

groupBox5.Text = set.Forecast[3].DayOfTheWeek;
label7.Text = string.Format("High {0}", set.Forecast[3].High);
label6.Text = string.Format("Low: {0}", set.Forecast[3].Low);
label11.Text = string.Format("Condition: {0}", set.Forecast[3].Condition);
pictureBox3.Load(set.Forecast[3].Icon);

I told you it was ugly. What I would like to do is have a single loop (and possible Generics) to accomplish the same task as this ugly code. I've tried a couple different ways but they always fail. The weather is being generated like so

WeatherSet set = WeatherService.Response(GoogleWeatherRequest.RequestData(new WeatherService("99109")));

And inside the variable set there is information for current information and 3 day forecast, but having it run like this drives me insane because it so noobish and not very efficient. So anyone got any cool & efficient way toaccomplish this task?


I think you should create your own custom control ForecastView for that with groupbox, three labels and picturebox on it. Let it have a property named Forecast and put the logic that fills the fields inside its setter. That way the code of the form will be really clean, like

forecastView1.ForeCast = set.Forecast[1];
forecastView2.ForeCast = set.Forecast[2];
// etc

Seeing as you are already putting your field inside of the groupbox, placement shouldn't be a problem.

Also, putting it all in separate control will give you a lot of code reuse options if one day you need to present this data on another form or even in another app. Not to mention that you'll be able to change the appearance of all forecasts at once if you decide to put the pictureBox left instead of right, etc.


There's nothing inefficient about your code. It's a bit sad to repeat yourself, but there's nothing really wrong with it.

If you want to avoid the repetition, simply put your controls in arrays so that you can do something like:

for (index in number of forecasts) {
   groupBox[index].Text = set.Forecast[index]....;
   hiLabel[index].Text  = ...:
   lowLabel[index].Text = ...:
   ...
}


Have your tried grouping the controls or genreating the controls on the fly? That way you don't have to index into the Forecast list you can just enumerate the collection and instead of using different controls for each iteration you only have that single set to populate.

foreach (var item in set.Forecast.Take(3))
{
    var groupBox = new GroupBox(); // it don't if the class is actually called GroupBox...
    groupBox.Text = item.DayOfTheWeek;
    labelHigh.Text = string.Format("High {0}", item.High);
    labelLow.Text = string.Format("Low: {0}", item.Low);
    labelCondition.Text = string.Format("Condition: {0}", item.Condition);
    pictureBox.Load(item.Icon);
    groupBox.Controls.Add(labelHigh)
    ...
    this.Controls.Add(groupBox);
}

The above code is just to give you an idea.

And actually, there's not much that can go wrong with your code, the only thing I'm really missing is an assertation that your index isn't out of range.


This is the solution I ended with (thanks @dyppl). For the user control idea

public partial class forecastView : UserControl
{    
    public forecastView()
    {
        InitializeComponent();
    }

    public forecastView(int x, int y, int index,WeatherSet set)
    {
        InitializeComponent();

        label7.Text = string.Format("High:{0}", set.Forecast[index].High);
        label8.Text = string.Format("Low: {0}", set.Forecast[index].Low);
        pictureBox3.Load(string.Format("http://www.google.com/{0}", set.Forecast[index].Icon));
        groupBox1.Text = set.Forecast[index].DayOfTheWeek;

        this.Location = new System.Drawing.Point(x, y);
    }
}

And I load them this way

private void LoadControls(WeatherSet set)
{
    RemoveConrols();
    //form2.Dispose();

    form = new forecastView(12, 136, 1, set);
    form1 = new forecastView(155, 136, 2, set);
    form2 = new forecastView(12, 218, 3, set);

    this.Controls.Add(form);
    this.Controls.Add(form1);
    this.Controls.Add(form2);
}

So thanks to all that helped me with this issue ;)

0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜