A while back I put together a quick and dirty implementation of a FindControl
extension method:
public static T FindControl<T>(this Control root, string id) where T : Control { Control c = root; Queue<Control> q = new Queue<Control>(); if (c == null || c.ID == id) return c as T; do { foreach (Control child in c.Controls) { if (child.ID == id) return child as T; if (child.HasControls()) q.Enqueue(child); } c = q.Dequeue(); } while (c != null); return null; }
It got the job done (if the control exists!), but I think we can do better.
Refactoring with Iterators
My first concern is that the method is doing too much. Rather than searching for the provided ID, the majority of the code is devoted to navigating the control’s descendents. Let’s factor out that logic into its own method:
public static IEnumerable<Control> GetDescendantControls(this Control root) { var q = new Queue<Control>(); var current = root; while (true) { if (current != null && current.HasControls()) foreach (Control child in current.Controls) q.Enqueue(child); if (q.Count == 0) yield break; current = q.Dequeue(); yield return current; } }
The new method is almost as long as the old one, but now satisfies the Single Responsibility Principle. I also added a check to prevent calling Dequeue()
on an empty queue. For those that have studied algorithms, note that this is a breadth-first tree traversal.
Now we can update FindControl
:
public static T FindControl<T>(this Control root, string id) where T : Control { Control c = root; if (c == null || c.ID == id) return c as T; foreach (Control child in c.GetDescendantControls()) { if (child.ID == id) return child as T; } return null; }
With the control tree traversal logic extracted, this updated version is already starting to smell better. But we’re not done yet.
DRY? Don’t Repeat Someone Else, Either
My second concern is how we’re checking for the ID in question. It’s not that the equality operator is a bad choice, as it will work in many scenarios, but rather that it’s not consistent with the existing FindControl
method. In particular, the existing FindControl
understands naming containers (IDs that contain ‘$’ or ‘:’). Rather than implement our own comparison logic, we should just leverage the framework’s existing implementation:
public static T FindControl<T>(this Control root, string id) where T : Control { if (id == null) throw new ArgumentNullException("id"); if (root == null) return null; Control c = root.FindControl(id); if (c != null) return c as T; foreach (Control child in c.GetDescendantControls()) { c = child.FindControl(id); if (c != null) return child as T; } return null; }
Fun fact: FindControl
will throw a NullReferenceException
if id
is null
.
Refactoring with LINQ
So we have extracted the descendant logic and leaned on the framework for finding the controls, but I’m still not quite satisfied. The method just feels too…procedural. Let’s break down what we’re really trying to do:
- Look at the current control and all its descendants.
- Use
FindControl
on each with the specified ID. - When we find the control, return it as type T.
As the subheading might suggest, we can express these steps quite nicely with LINQ:
-
var controls = root.AsSingleton().Concat(root.GetDescendantControls());
-
var foundControls = from c in controls let found = c.FindControl(id) where found != null select found;
-
return foundControls.FirstOrDefault() as T;
Behind the scenes, this is how I might have thought through this code:
- We use
AsSingleton()
(my new preferred name, to align with F#’sSeq.singleton
, forAsEnumerable()
, which I introduced here) andConcat()
to prependroot
to the list of its descendants, returned as a lazy enumeration. - We use a query over those controls to retrieve matches from
FindControl()
, again returned as a lazy enumeration. - We grab the first control found, or
null
if none match, and return itas T
.
Because all our enumerations are lazy, we put off traversal of the entire control tree until we know we need to. In fact, if our ID is found in the root control, GetDescendantControls()
won’t even be called! Through just a bit of refactoring, we have both an efficient and readable solution.
For completeness, here’s the final version with a more descriptive name to contrast with the existing FindControl()
:
public static T FindDescendantControl<T>(this Control root, string id) where T : Control { if (id == null) throw new ArgumentNullException("id"); if (root == null) return null; var controls = root.AsSingleton().Concat(root.GetDescendantControls()); var foundControls = from c in controls let found = c.FindControl(id) where found != null select found; return foundControls.FirstOrDefault() as T; }
I have added these methods, along with AsSingleton()
and a host of others, to the SharePoint Extensions Lib project. Check it out!
July 22, 2009 at 9:04 pm
[…] Refactoring with LINQ & Iterators: FindDescendantControl and GetDescendantControls […]
September 23, 2009 at 3:43 am
Does AsSingleton return a singleton (as the name suggests)?
September 23, 2009 at 6:34 am
Singleton is used here in the mathematical sense (set with cardinality 1) as opposed to the design pattern (single instance of an object). Feel free to call it AsEnumerable() instead; I use singleton to align with F#.