views:

2403

answers:

4

I have a _Click event that is firing when a chart is clicked on. In this event I immediately cast the sender to type Chart. I often follow this paradigm but feel icky each time that I do.

In this particular instance I am also immediately running out to find a corresponding UpdatePanel so I can add a dynamically rendered GridView to it. The Chart and UpdatePanel are cobbled together by having similar IDs. Both the chart and updatepanel are dynamically created at runtime.

I am wondering if there is a better/preferred way to implement this sort of behavior.

protected void Chart_Click(object sender, ImageMapEventArgs e)
{
    Chart chart = (Chart)sender;
    UpdatePanel up = (UpdatePanel)chart.Parent.FindControl(chart.ID + "UP");

    GridView gv = new GridView();
    Dictionary<string, string> displayFields =
        new Dictionary<string, string>();

    // add data to displayFields by using the ImageMapEventArgs.PostBackValue
    // to create data for dictionary ...

    gv.DataSource = displayFields;
    gv.DataBind();
    up.ContentTemplateContainer.Controls.Add(gv);
}
+1  A: 

I think it's safe to cast sender to Chart because you know it's always a Chart. Note the method name is even Chart_Click.

You could, however, save a reference to the UpdatePanel in the Chart's Tag property. That saves the ugliness and risk of a name-search in the parent. Could get hard to maintain if you're constantly switching parents or moving panels around.

If you know 100% the UpdatePanel's going to be there and named properly, however, there's nothing necessarily "wrong" with your approach IMHO. You might want to throw a "don't change this name" comment next to the UpdatePanel.Name = line where you initialize it though for security. (BTW, if you're not the only one with access to your code, the 100% instantly drops to at most a 99.9%.)

lc
+1  A: 

Use as instead.

Your cast will throw if it fails.

Chart chart = sender as Chart;
if (chart == null)
   return; // or do something else

// the rest of your handler.
plinth
I think throwing is the proper response. In my book, nobody but a Chart should be calling Chart_Click. Using as adds extra unnecessary overhead and checking.
lc
throwing in an event handler will result in an unhandled exception.
Michael Meadows
+3  A: 

I'm not sure what else can be done about casting sender to a chart (other than using as), but there are many ways to deal with the associated controls problem.

  • Add a property to the Chart called "LinkedPanel" and assign your update panel to it

    You'll see a similar pattern used in a notification icon, where you associate it with a context menu)

    I think this is particularly nice, because when it's done right you can just assign the linked control in the form designer. (Of course it won't help with your dynamically generated controls)

  • Combine both controls into a single UserControl (if the controls always show right next to each other, this might be the right thing to do)

  • Create an object that knows about both controls, and have it handle the events that they raise

I agree that depending on the name just feels wrong; I'd be too embarassed/nervous to use it in production code :-/ (If you're the only developer, then I guess it's up to you...)

Using these other approaches makes it less likely that an "innocent change" made by another developer is going to start causing unexpected exceptions.

Daniel LeCheminant
Adding a property to the Chart assumes it's not a sealed class of course.
lc
@lc: Yeah, hopefully that's not the case ... but if it is, there are other routes that can be taken.
Daniel LeCheminant
Yeah. Adding the property is certainly the cleanest way to go though.
lc
I like the idea of adding a property to the chart. The only way I can see to do this would be to create a new class inheriting from chart which implemented the new property. Is that the way or am I missing something? FYI I am using the new .NET chart component http://code.msdn.microsoft.com/mschart
ahsteele
@ahsteele: Yes, you would have to inherit the chart to add the new property.
Daniel LeCheminant
A: 

There is nothing really wrong in what you have done. But you can prevent typecasting error by using 'as' instaed of explicit casting as follows.

if you want to find if the control is an UpdatePanel you can do so by using GetType() method

example: sender.GetType().name will give you the control name

protected void Chart_Click(object sender, ImageMapEventArgs e)
{
    Chart chart = (Chart)sender;
    control c = Parent.FindControl(chart.ID + "UP");
    UpdatePanel up ;

    if (c != null)
    {
       up = c as UpdatePanel;**


       GridView gv = new GridView();
       Dictionary<string, string> displayFields =
           new Dictionary<string, string>();

       // add data to displayFields by using the ImageMapEventArgs.PostBackValue
       // to create data for dictionary ...

       gv.DataSource = displayFields;
       gv.DataBind();
       up.ContentTemplateContainer.Controls.Add(gv);
   }
}
DK