Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Follow-up posted here: Refactor this method to a cleaner state (continued)

I am refactoring a legacy application. I DO NOT have the ability to change the Permissions, CustomerReport or the ListItems classes. They are in external dlls that are given to me. I realize that the ref usage is dumb and it makes NO SENSE. I am going to work with my peers to follow better practices.

This whole class looks ugly and mind you I have refactored some of it out into methods but it still is too large and unwieldy:

public class ReportService : IReportService
{
    private readonly Permissions _Permissions;


    public ReportService()
    {
        _Permissions = new Permissions();
    }

    public IList<Node> GetFor(string userGroup)
    {
        var nodes = new List<Node>();
        var ListItems = new List<ListItem>();

        //Yes this is silly since statusReturn is never used
        var statusReturn = _Permissions.GetReportsForUserGroupName(userGroup, ref ListItems);

        if (ListItems.Any())
        {
            var initialReportType = ListItems[0].value;
            var initialCrs = ListItems[0].key;
            var parent = AssembleNode(initialReportType);
            var child = AssembleNode(initialCrs, parent);
            foreach (var item in ListItems)
            {
                var customerReport = (CustomerReport)item.dataObject;
                var grandChild = AssembleGrandChild(child, customerReport);
                if (item.value.Equals(initialReportType))
                    if (item.key.Equals(initialCrs))
                        child.Children.Add(grandChild);
                    else
                    {
                        initialCrs = item.key;
                        parent.Children.Add(child);
                        child = AssembleNode(item.key, parent);
                        child.Children.Add(grandChild);
                    }
                else
                {
                    initialReportType = item.value;
                    initialCrs = item.key;
                    parent.Children.Add(child);
                    nodes.Add(parent);
                    parent = AssembleNode(item.value);
                    child = AssembleNode(item.key, parent);
                    grandChild = AssembleGrandChild(child, customerReport);
                    child.Children.Add(grandChild);
                }
            }
            parent.Children.Add(child);
            nodes.Add(parent);
        }
        return nodes;
    }



    private static Node AssembleNode(string value, Node parent = null)
    {
        //This is an extension method on object that was used at my last dev shop.
        //I got used to using it so out of habit it is here
        return parent.IsNotNull() ? 
            new Node(parent){ Text = value, Value = value } : 
            new Node { Text = value, Value = value };
    }

    private static Node AssembleGrandChild(Node child, CustomerReport customerReport)
    {
        return new Node(child)
        {
            Text = customerReport.FileName,
            Value = customerReport.ReportKey.ToString(),
            Title = customerReport.Description
        };
    }
}

I really want to refactor this to a better state; any suggestions?

share|improve this question
3  
What is up with the IsNotNull method call in there? Why not just return (parent != null) ? ... ? –  Lasse V. Karlsen Jul 1 at 14:30
    
I used the exact same IsNotNull extension method at a dev shop for a while, but we eventually dropped it because it's kinda stupid and doesn't play well with code analysis. –  craftworkgames Jul 2 at 11:27

4 Answers 4

up vote 4 down vote accepted

I would put:

var ListItems = new List<ListItem>();
//Yes this is silly since statusReturn is never used
var statusReturn = _Permissions.GetReportsForUserGroupName(userGroup, ref ListItems);

in a method that just returns the list. So you are isolating the madness a bit.

I'm not a C# programmer, but wouldn't listItems be better than ListItems?

I honestly did not understand what the code is doing. Maybe you could explain it a little bit in your post.

It seems that you have a tree structure in there. So why transform a list which originally seems to contain a tree to another list, which should also be a tree?

share|improve this answer
    
That is done, and Upvoted. I am hoping to clean up the AssembleNodes Method more specifically. –  Robert Jul 1 at 15:23

you should be consistent with your code blocks and how you use brackets

if (item.value.Equals(initialReportType))
    if (item.key.Equals(initialCrs))
        child.Children.Add(grandChild);
    else
    {
        initialCrs = item.key;
        parent.Children.Add(child);
        child = AssembleNode(item.key, parent);
        child.Children.Add(grandChild);
    }
else
{
    initialReportType = item.value;
    initialCrs = item.key;
    parent.Children.Add(child);
    nodes.Add(parent);
    parent = AssembleNode(item.value);
    child = AssembleNode(item.key, parent);
    grandChild = AssembleGrandChild(child, customerReport);
    child.Children.Add(grandChild);
}

This is kind of hard to read.

If you use brackets on the else statement, then you should use them on the if statement attached to it.

if (item.value.Equals(initialReportType)) {
    if (item.key.Equals(initialCrs)) {
        child.Children.Add(grandChild);
    } else {
        initialCrs = item.key;
        parent.Children.Add(child);
        child = AssembleNode(item.key, parent);
        child.Children.Add(grandChild);
    }
} else {
    initialReportType = item.value;
    initialCrs = item.key;
    parent.Children.Add(child);
    nodes.Add(parent);
    parent = AssembleNode(item.value);
    child = AssembleNode(item.key, parent);
    grandChild = AssembleGrandChild(child, customerReport);
    child.Children.Add(grandChild);
}

That is how I would write it, but it may look better if we add some newlines in there

if (item.value.Equals(initialReportType)) 
{
    if (item.key.Equals(initialCrs)) 
    {
        child.Children.Add(grandChild);
    } 
    else 
    {
        initialCrs = item.key;
        parent.Children.Add(child);
        child = AssembleNode(item.key, parent);
        child.Children.Add(grandChild);
    }
} 
else 
{
    initialReportType = item.value;
    initialCrs = item.key;
    parent.Children.Add(child);
    nodes.Add(parent);
    parent = AssembleNode(item.value);
    child = AssembleNode(item.key, parent);
    grandChild = AssembleGrandChild(child, customerReport);
    child.Children.Add(grandChild);
}
share|improve this answer

To remove one level of nested if's you can negate first condition and return empty list

if (! ListItems.Any())
    return nodes;

One more thing which wasn't mention yet is

if (item.key.Equals(initialCrs)) 
{
    child.Children.Add(grandChild);
} 
else 
{
    initialCrs = item.key;
    parent.Children.Add(child);
    child = AssembleNode(item.key, parent);
    child.Children.Add(grandChild);
}

Can be change to

if (! item.key.Equals(initialCrs))  
{
    initialCrs = item.key;
    parent.Children.Add(child);
    child = AssembleNode(item.key, parent);
}

child.Children.Add(grandChild);
share|improve this answer
    
These two pieces of code are not equivalent. In the first, child.Children.Add(grandChild) under the if clause is executed only if that condition is true. In the second, it is always executed regardless of the value of the condition. It needs an else around it to make it the same. –  cbojar Jul 2 at 20:15
    
@cbojar you are wrong –  miniBill Jul 2 at 20:45
    
Ah, actually I am. I didn't see that he took it out of the else clause as well. In the follow-up of this question the updated version of this part is wrong, but here it is correct. My bad. –  cbojar Jul 2 at 22:10
private readonly Permissions _Permissions;

I don't understand this. Is this supposed to be a Property, the backing field for a property, or a member? If the latter, it should be lowercase.

share|improve this answer
1  
How do you know what the company's coding style is? While it definitely isn't the most common C# convention, if the rest of the codebase uses an initial uppercase letter on members, then this code should as well. –  Philip Kendall Jul 2 at 7:54
    
@PhilipKendall because in other places, he uses an initial lowercase on members. Also he doesn't use an initial underscore anywhere else. –  Snowbody Jul 2 at 13:33
2  
Unless I'm missing something obvious, there's only one member variable in the whole code sample. There's a bunch of local variables, but that's not the same thing. –  Philip Kendall Jul 2 at 13:42
    
You're right, I got members confused with method locals. But I still think initial underscore and then uppercase is an unusual convention. –  Snowbody Jul 2 at 18:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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