I created the following recursive routine to get all the checked nodes in a treeview:
Sub GetAllCheckedNodes(ByVal tn As TreeNode, ByRef NodesList As List(Of TreeNode))
If tn.Checked Then NodesList.Add(tn)
For Each nd As TreeNode In tn.Nodes
GetAllCheckedNodes(nd, NodesList)
Next
End Sub
Basically, the way I use it would be to declare an empty List(Of Treenode) and then pass it into this routine as the second argument.
My question is that I've been told this is "not great" programming practice - Why and what could go wrong?
It seems to be the most elegant solution I could come up with to get this to work.
There is no need to pass the list ByRef -- it's already a reference type. Just pass it by value and you'll still be able to add checked items to it.
The more common solution here would be, rather than mutating a collection that's passed in, to return a collection/sequence that represents all of the nodes that meet the condition. Personally I think IEnumerable fits this situation well, as you can either use an iterator block or just concat the results together to so that the method optionally adds itself and then returns everything from the recursive call.
This has several benefits. First, it's easier to call; there's no need to create a list, pass it in, ensure there's no threading/concurrent modification issues, and then use it later, you can just call the method and use the result.
The problem is that of expectations from other programmers and the fact that you are relying on side-effects (that is, changes to a passed in parameter).
When someone sees a Get*, the expectation is that it will return a value.
You can still have a recursive function and not have to rely on changing a passed in parameter.
