CODEWITHSUNDEEP
c#generics
computerfreak07
computerfreak07

Reputation: 19

Could Generics Be Of Use?

I have the following code at least 10 different times in my code. Seems kind of smelly to me.

        public void DisplayTransitInfo(TransitInfo transitInfo)
    {
        if (InvokeRequired)
            EndInvoke(BeginInvoke(new MethodInvoker(() => DisplayTransitInfo(transitInfo))));
        else
        {
            var control = (from string key in _visiblePanes.Keys
                           where key == "transitInfo"
                           select _visiblePanes[key].Control).ToList();

            TransitInfoControl cntl = (TransitInfoControl)control[0];
            //TODO: Transit Info
        }
    }

    public void ModifyParties(UltraTreeNode node)
    {
        if (InvokeRequired)
            EndInvoke(BeginInvoke(new MethodInvoker(() => ModifyParties(node))));
        else
        {
            var control = (from string key in _visiblePanes.Keys
                           where key == "parties"
                           select _visiblePanes[key].Control).ToList();

            PartiesControl cntl = (PartiesControl)control[0];
            cntl.ModifyParties(node);
        }
    }

I feel that it could be possible to use generics in this situation. I have also considered moving:

                var control = (from string key in _visiblePanes.Keys
                           where key == "parties"
                           select _visiblePanes[key].Control).ToList();

to its own function that would return the instance of the control in the dictionary.

Is this code smelly or do I just need to get my nose worked on?

Thanks as always!

Upvotes: 0

Views: 128

Answers (3)

Jim Mischel
Jim Mischel

Reputation: 133995

I may be missing something, but it seems to me you can get rid of the whole LINQ expression there there. That is, this code:

 var control = (from string key in _visiblePanes.Keys
                where key == "parties"
                select _visiblePanes[key].Control).ToList();
 PartiesControl cntl = (PartiesControl)control[0];

If I'm reading that right, you're enumerating a keyed collection to find all of the items that have the key "parties". But since you write _visiblePanes[key].Control, it seems like there's just one of them. I'm going to assume that _visiblePanes is a Dictionary<string, BaseControl>, where BaseControl is whatever class PartiesControl and your other control types inherit from.

BaseControl cntl;
if (_visiblePanes.TryGetValue("parties", out cntl))
{
    PartiesControl pcntl = cntl as PartiesControl;
    // do whatever
}

Upvotes: 2

phoog
phoog

Reputation: 43046

If you are looking to simplify 

        var control = (from string key in _visiblePanes.Keys  
                       where key == "transitInfo"  
                       select _visiblePanes[key].Control).ToList();  

        TransitInfoControl cntl = (TransitInfoControl)control[0];

and 

        var control = (from string key in _visiblePanes.Keys   
                       where key == "parties"   
                       select _visiblePanes[key].Control).ToList();   

        PartiesControl cntl = (PartiesControl)control[0];

And if _visiblePanes is a dictionary, which implies it can have no duplicate keys, then you don't need generics, just use this instead:

var cntl = (TransitInfoControl)_visiblePanes["transitInfo"].Control;

and

var cntl = (PartiesControl)_visiblePanes["parties"].Control;

EDIT

(added casts above for equivalence with the original code)

This suggestion, compared with Jim Mischel's, is not only simpler to type and read, it is also closer to the original code, because it throws an exception if the key is not present in the dictionary. If the original code used Linq in an attempt to cover that possibility, it fails at PartiesControl cntl = (PartiesControl)control[0];. Assuming that the absence of a key in the dictionary is non-exceptional, then, of course, Jim's TryGetValue solution is better.

To return to the original question about generics, there is a possible advantage in extracting the get-and-cast logic into a generic method:

bool TryGetCast<T>(IDictionary<string, BaseControl> dict, string key, out T value) where T : BaseControl
{
    BaseControl tryGet;
    if (dict.TryGetValue(key, out tryGet)
    {
        value = (T)tryGet;
        return true;
    }
    value = default(T);
    return false;
}

can be called thus:

PartiesControl cntl;
if (TryGetCast(_visiblePanes, "parties", out cntl))
{
    //do whatever;
}

However, the benefit is slight. Jim's solution plus a cast is not much more verbose:

BaseControl c; 
if (_visiblePanes.TryGetValue("parties", out c)) 
{
    var cntl = (PartiesControl)c;
    // do whatever
}

Upvotes: 3

escargot agile
escargot agile

Reputation: 22379

If you have the same code 10 times in your application, it's definitely time to refactor!

Why do you write 

 (from string key in _visiblePanes.Keys
                       where key == "parties"
                       select _visiblePanes[key].Control)

when you can just use _visiblePanes["parties"] (or use TryGetValue if you're not sure whether the key exists)?

And why is control a list? control doesn't sound like the name of a collection.

Upvotes: 3

Related Questions