Dynamic Treenode Creation

Question

I am trying to create a treenode structure based on a database table for a Dungeons and Dragons character creation program I am working on. I have come up with an incredibly confusing way to do it, but I am wondering if anyone can give me some better ideas on how to do this more efficiently. I have a tables called subfeats and feats. Subfeats contains all feats from the table feats that have subfeats. Really, all I want to do is create a parent/child treenode structure for all feats and their subfeats. Here is my code:

foreach (DataRow subrow in ds.subfeats.Rows)
{
 subfeatSet.Add(Convert.ToString(subrow[ds.subfeats.Columns.IndexOf("subfeat")]));
}
foreach (DataRow row in ds.feat.Rows)
{
 TreeNode newNode = new TreeNode(Convert.ToString(row[ds.feat.Columns.IndexOf("name")]));
 if (subfeatSet.Contains(newNode.Text))
 {
 // Do Nothing.
 }
 else
 {
 treeviewFeatsFeats.Nodes.Add(newNode);
 foreach (DataRow subrow in ds.subfeats.Rows)
 {
 if (Convert.ToString(subrow[ds.subfeats.Columns.IndexOf("feat")]).Equals(newNode.Text))
 {
 TreeNode subTreeNode = new TreeNode(Convert.ToString(subrow[ds.subfeats.Columns.IndexOf("subfeat")]));
 newNode.Nodes.Add(subTreeNode);
 foreach (DataRow subsubrow in ds.subfeats.Rows)
 {
 if (Convert.ToString(subsubrow[ds.subfeats.Columns.IndexOf("feat")]).Equals(subTreeNode.Text))
 {
 TreeNode subsubTreeNode = new TreeNode(Convert.ToString(subsubrow[ds.subfeats.Columns.IndexOf("subfeat")]));
 subTreeNode.Nodes.Add(subsubTreeNode);
 }
 }
 }
 }
 }
}

Answer 1

I like the use of white space and indentation, makes the code easy to read.

I would start using var for your variable declarations. It keeps the code cleaner, and easier for your eyes to scan.

Remove the // Do Nothing if statements. It is redundant and adds extra lines of code that are unneeded. This should work:

if (!subfeatSet.Contains(newNode.Text))
{
 treeviewFeatsFeats.Nodes.Add(newNode);
 foreach (DataRow subrow in ds.subfeats.Rows)
 {
 ...
}

I would also learn about linq, you can clean up most of your loops with linq. I would also look into moving some of the inner loops out into their own function. Moving to a function would remove some duplication you have in your code too.

private void ProcessNode(TreeNode parent, DataSet ds)
{
 var index = ds.subfeats.Columns.IndexOf("feat");
 foreach (var treeNode in (
 from row in ds.subfeats.Rows 
 let nodeName = Convert.ToString(row[index])
 where rows.Contains(nodeName)
 select new TreeNode(nodeName)))
 {
 ProcessNode(treeNode, rows);
 parent.Add(treeNode); 
 }
}

Now your function looks like this:

var index = ds.feat.Columns.IndexOf("name");
foreach (var treeNode in (
 from row in ds.feat.Rows,
 let nodeName = Convert.ToString(row[index])
 where rows.Contains(nodeName)
 select new TreeNode(nodeName)))
{
 ProcessNode(treeNode, ds);
}

This is a start. I'm not happy with having to pass DataSet into the functions, but without fully understanding the data structure, I can't think of another way of doing it.

• Wilbert: I would suggest to replace the '//Do nothing' with a continue: if (subfeatSet.Contains(newNode.Text)) continue; You save an indentation level for the rest of the code and the code is more readable.
• Jeff Vanzella: Personally I would take the indentation over continue in this case because its only one level. And also, in my final code, the if statement was actually combined into the linq statement.
• Duders: Thanks for the help! LINQ definitely seems like the way to go.