Refactor this code

Question

I am trying to populate a tree view based on a list of staff from various deparments (lstStaff).

public class Staff
{
    public int StaffID { get; set; }
    public string StaffName { get; set; }
    public int DeptID { get; set; }
    public string DepartmentName { get; set; }
}

The result should be like this:

Department A

• John
• Dave

Department B

• Andy
• Leon

I have written the following code. However, I think the code should be refactored further.

RadTreeNode deptNode;
RadTreeNode staffNode;
var departments = (from d in lstStaff
                   where !string.IsNullOrEmpty(d.DepartmentName) 
                select new { d.DeptId, d.DepartmentName })
                .Distinct();

foreach (var d in departments)
{
    //add department node
    deptNode = new RadTreeNode()
    {
        Value = d.DeptId,
        Text = d.DepartmentName
    };
    tvwDepartmentCases.Nodes.Add(deptNode);

    //add staff nodes to department node
    var staffs = lstStaff
                    .ToList()
                    .Where(x => x.DeptId == d.DeptId);

    foreach (var s in staffs)
    {
        staffNode = new RadTreeNode()
        {
            Value = s.StaffID,
            Text = s.StaffName
        };
        deptNode.Nodes.Add(staffNode);
    }
}

Any comments are welcomed. Thanks!

Answer 1

I think what you have is pretty straightforward and easy to read. It has room for performance improvement (probably negligible) but you would sacrifice some readability. With that said, what don't you like about your current solution that you'd want further refactoring?

Answer 2

I must run to work now, but I think you did too much querying in the loop. You should use .ToLookup to create lookup table for groups of the staff before the loop, and use the lookup table to find the staff members in each department instead.

Answer 3

var grping = from staff in lstStaff
             where !String.IsNullOrEmpty(staff.StaffName)
             group staff.StaffName by staff.DepartmentName;

foreach (var deptGrp in grping)
{
         //add a high level node using deptGrp.Key
         foreach (var staffName in deptGrp) 
               //add a lower level node
}

Answer 4

I would write -

foreach(var member in lstStaff)
 var deptNode = FindDeptNode(member.DeptId, member.DeptName);
 deptNode.Add(member.StaffId, member.StaffName);


.. // in FindDeptNode

if(!departmentsTreeView.ContainssNode(deptId, deptName))
 departmentsTreeView.Add(deptId, deptName);

return departmentsTreeView[deptId, deptName];

you can convert this into the compilable form.

Cheers.

Answer 5

Use the GroupBy operator, it's easy and looks so much nicer.

var departmentGroups = lstStaff.GroupBy(staff => 
    new { staff.DeptID, staff.DepartmentName });

foreach (var department in departmentGroups)
{
    var deptNode = new RadTreeNode()
    {
        Value = department.Key.DeptID,
        Text = department.Key.DepartmentName
    };

    tvwDepartmentCases.Nodes.Add(deptNode);

    foreach (var staffMember in department)
    {
        var staffNode = new RadTreeNode()
        {
            Value = staffMember.StaffID,
            Text = staffMember.StaffName
        };
        deptNode.Nodes.Add(staffNode);
    }
}

It's also lower complexity because you don't need to iterate through the entire collection of lstStaff for each department.

Answer 6

I'd go a step further and create the staff nodes from within the query. It's logically clearer to me since we have all the information necessary to create the node at that point. Unfortunately it isn't possible to create the department node within the query in a clean, readable way.

var depts = from staff in lstStaff
            where !string.IsNullOrEmpty(staff.DepartmentName)
            group   //staff nodes
                new RadTreeNode  //each item in the group is a node
                {
                    Value = staff.StaffID,
                    Text = staff.StaffName,
                }
            by      //departments
                new
                {
                    staff.DeptID,
                    staff.DepartmentName,
                };
foreach (var dept in depts)
{
    var deptNode = new RadTreeNode
    {
        Value = dept.Key.DeptID,
        Text = dept.Key.DepartmentName,
    };
    deptNode.Nodes.AddRange(dept.ToArray());
    tvwDepartmentCases.Nodes.Add(deptNode);
}