CodeSOD: Control Tree

User interfaces tend to map fairly naturally to trees as a data structure. In the modern world where we've forgotten how to make native UIs and do everything as web applications pretending to be native, the "tree" model is explicit via the DOM. But even in native UIs, we tend to group controls into containers and panels and tabs.

Now, there may be cases where we need to traverse the tree, and trees present us with a very natural way to traverse them- recursion. To search the tree, first search the subtree. Recursion can have its own problems, but the UI tree of a native application is rarely deep enough for those problems to be an issue.

Nick inherited some VB .Net code that iterates across all the controls in the screen. It does not use recursion, which isn't a problem in and of itself. Every other choice in the code is the problem.

For Each x In Me.Controls Select Case (x.GetType.ToString) Case "System.Windows.Forms.Panel" For Each y In x.Controls Select Case (y.GetType.ToString) Case "System.Windows.Forms.TabControl" For Each z In y.Controls Select Case (z.GetType.ToString) Case "System.Windows.Forms.TabPage" For Each w In z.Controls If w.Tag = "Store Info" Then strName = CheckForExceptions(w.Name) iIndexStoreInfoValues = (arrStoreInfoValues.Length - 1) If w.GetType().ToString() = "System.Windows.Forms.CheckBox" Then arrStoreInfoValues(iIndexStoreInfoValues) = CType(w, CheckBox).CheckState Else arrStoreInfoValues(iIndexStoreInfoValues) = w.Text End If ReDim Preserve arrStoreInfoValues(iIndexStoreInfoValues + 1) End If Next End Select Next Case "System.Windows.Forms.GroupBox" For Each z In y.Controls If z.Tag = "Store Info" Then iIndexStoreInfoValues = (arrStoreInfoValues.Length - 1) arrStoreInfoValues(iIndexStoreInfoValues) = z.Text ReDim Preserve arrStoreInfoValues(iIndexStoreInfoValues + 1) End If Next Case Else If y.Tag = "Store Info" Then iIndexStoreInfoValues = (arrStoreInfoValues.Length - 1) strName = CheckForExceptions(y.Name) arrStoreInfoValues(iIndexStoreInfoValues) = y.Text ReDim Preserve arrStoreInfoValues(iIndexStoreInfoValues + 1) End If End Select Next Case Else '"System.Windows.Forms.TabControl" For Each y In x.Controls If y.Tag = "Store Info" Then iIndexStoreInfoValues = (arrStoreInfoValues.Length - 1) arrStoreInfoValues(iIndexStoreInfoValues) = y.Text ReDim Preserve arrStoreInfoValues(iIndexStoreInfoValues + 1) End If Next End SelectNext

At its deepest, this particular pile of code has 12 levels of indenting. Which I've cleaned up a bit here- as submitted, the code was using 8 spaces per tab stop, which made it entirely unreadable in a window of reasonable width. Not that it'd be readable at any width, mind you.

I'm not going to go line by line through this code. It's not worth the psychic damage I'd inflict upon you. But I do want to call out a few highlights.

Note the repeated uses of ReDim Preserve arrStoreInfoValues.... This is an ugly VB hack for pretending to have arrays that can grow- ReDim creates an entirely new array, at the new size, and Preserve ensures that all the data in the original array gets copied over to the new array. It's expensive and time consuming, and .NET has plenty of built-in data structures that make this unnecessary. Likely, this was inherited from some VB6 code that got ported over to .NET. It was a weirdly common convention in VB6, instead of using any sort of data structure that can grow.

In a few cases, we use a Select with only one branch to it- making it a fancy If statement but less readable. Now, there's a good logic to this, I suppose, in that you may want more branches at some point, but I still hate reading it. Even worse, those switch statements are switching on the string representation of the type of the control. That particular code stench is always a "fun" thing to see, and it hints at an easy way to clean up this code: refactor those branches into methods, and use function overloading and the type system to replace the switches- if that were even necessary, as many of the branches do basically the same thing anyway.

But there's no real point to cleaning up this code, because as Nick discovered: it didn't actually work. The assumptions the code makes about how the controls are organized are false assumptions. It misses loads of controls in its traversal.

Nick replaced it with a 10 line recursive function that never needs to check the GetType of any of the controls.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!