3
\$\begingroup\$

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);
                    }
                }
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Wilbert Apr 25 '13 at 14:09
  • \$\begingroup\$ 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. \$\endgroup\$ – Jeff Vanzella Apr 25 '13 at 16:06
  • \$\begingroup\$ Thanks for the help! LINQ definitely seems like the way to go. \$\endgroup\$ – Duders Apr 25 '13 at 16:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.