[RESOLVED] better way to do this?

[Crash893]

I came up with this little trick on my own but i have a feeling that its wildly inefficient


I tried saying for each MatchType1 in the tabpage.controls

but it didn't seem to understand that i would onlly want to retreive matchtyp1's and not labels or textboxes


so i set it to loop through everything but if i do that i can't access the contols methods directly i have to create a new one and then set it equal to what C is pointing at.

anyway i thought i would run this past the experts to see if there is a better way to do this

private void ClearRegion(string Region)
        {
            foreach (Control C in StringtoTabPage(Region).Controls)
            {
            if (C is MatchType1)
            {
                MatchType1 holder = new MatchType1();
                holder = (MatchType1)C;
 
                holder.ClearInfo();
            
            }
                if(C is MatchType2)
                {
                    MatchType2 holder = new MatchType2();
                    holder = (MatchType2)C;
 
                    holder.ClearInfo();
                }
                if (C is TextBox)
                {
                    TextBox holder = new TextBox();
                    holder = (TextBox) C;
                    holder.Clear();
                
                }
            
            }
        }

[Techno]

there are ways to improve it... but since im cut for time, ill quickly (And probably missing some things) point out a couple of things

check for null objects, otherwise you will get a NullReferenceException when dealing with the object in question

you can also do this:



if (C is MatchType1)
{

MatchType1 holder = ((MatchType1)C);
// OR even better:
// MatchType1 holder = C as MatchType1;

if (holder != null)
{

holder.ClearInfo();

}
}

[jmcilhinney]

There are several issues with your code. First up, when you find the type of the control you're creating a new instance of that type, then discarding that and using the one on the form:

MatchType1 holder = new MatchType1();
holder = (MatchType1)C;

You ONLY use the 'new' key word to invoke a constructor and create a new instance. Do you want to create a new instance here? Of course not.

Next, you should be using 'else if' for the second and third blocks. If the first condition is true and C is type MatchType1 then what's the point of testing to see if it's MatchType2 or TextBox? You know for a fact that it's not so why check?

Finally, I could be wrong but I'm guessing that MatchType1 and MatchType2 are both derived from the same base class. If not then I'm guessing that they should be. If the ClearInfo method is inherited from that base class then you don't have to test for both separately. You can just test for the base type and call its ClearInfo method.

Techno's idea is what you generally should have used in the past and quite possibly should now too. It's good to keep LINQ in mind too though. It's probably less attractive as you're looking for more than one type but you can enumerate the result of a LINQ query, which allows you to filter:

foreach (TextBox txtbx in from Control ctl in this.Controls
                          where ctl is Button
                          select ctl)
{
    txtbx.Clear();
}

[Techno]

thats a good demo about using LINQ - interesting. havent seen that before. must investigate further on that....

[jmcilhinney]

thats a good demo about using LINQ - interesting. havent seen that before. must investigate further on that....

People may tend to assign the result of the query to a variable and then use that in the foreach loop but I hate using intermediate variables that don't serve to clarify. The IDE formats the LINQ query nicely so it's quite clear what's happening, as long as you've got a bit of a grasp of LINQ. A LINQ to Objects query returns an IEnumerable object and that's all that a foreach loop requires.

[Techno]

absolutely. awesome!

[Crash893]

again thank you I'll give it a whirrl at work and post my final results or any questions to clarify i have.

thanks

[Crash893]

Better?

also im using the generic method to return the type i need and access it directly rather than do a shell game with creating new contorls

private void ClearRegion(string Region)
        {
            foreach (Control C in StringtoTabPage(Region).Controls)
            {
                if (C is MatchType1)
                {
                    FindControl<MatchType1>(C.Name).ClearInfo();
                }
                else if (C is MatchType2)
                {
                    FindControl<MatchType2>(C.Name).ClearInfo();
                }
                else if (C is TextBox)
                {
                    FindControl<TextBox>(C.Name).Clear();
                }
            }
        }

[jmcilhinney]

Um, that doesn't really make a lot of sense. You're looping through the controls and, for each one, you get it's Name and then pass that to another method to go and get the very same control. Why do you need this FindControl method to find the control when you've already got it? If you're going to loop through all the controls anyway then the FindControl method is pointless. Just cast C as the appropriate type and then call the method:

if (C is MatchType1)
{
    ((MatchType1)C).ClearInfo();
}

From what I can see of this thread and the other one of yours related to this one, what you should be doing is this:

foreach (TextBox tb in this.Controls.OfType<TextBox>())
{
    tb.Clear();
}

You just do that three times: once for each type.