views:

183

answers:

6

Some I'm reviewing some code within my team's code base, where we traverse over one hierarchical data structure and build a new data structure from it. There are no nested loops -- every level of the hierarchy has its own dedicated function.

So we have code like this:

public void DoA(A a, Transform transform)
{
    foreach(B b in a)
        DoB(b, transform);
}

public void DoB(B b, Transform transform)
{
    if (b != null && b.IsAvailable)
        return;

    foreach(C c in b)
        DoC(c, transform)
}

public void DoC(C c, Transform transform)
{
    var cAndAHalf = DoCAndAHalf(c.FindAll(x => x.Children > 0);

    foreach(D d in cAndAHalf)
        DoD(d, transform);
}

. . .

public void DoX(X x, Transform transform)
{
    Res res = new Res();
    if (x.Selected)
    {
        res.Selected = true;
        res.ResourceCount = 1;
    }

    transform.Add(res);
}

There are dozens of methods like this, where each method is 3 to 5 lines in length, similarly named, usually contains a trivial null check or filter, and a cursory review of the code shows that no method is really invoked more than once. Methods are public for unit testing purposes.

I personally find it the code very hard to navigate through, because dozens of public methods == dozens of potential entry points into the class guts.

Does this sort of coding pattern have a name? Is it an anti-pattern? Is this style more advantageous than simply nesting the loops in a single function?

+2  A: 

Yikes. That should most definitely qualify as something. Not sure it's an anti-pattern though. I'd go with anti-recursion.

It could also be a sign of Programming by Permutation (with the developer adding a level to the hierarchy each time the structure grows deeper).

Justin Niessner
A: 

It kind of seems like the decorator pattern, but only kind of.

http://en.wikipedia.org/wiki/Decorator_pattern

Look it up and see if it matches your codebase more specifically.

glowcoder
+1  A: 

This is something I would consider good coding style - small methods with a single responsibility. If the methods are well named this should yield code easy to understand and maintain.

Of course if many of the methods are very similar one could look for a common pattern and factor this out by using delegates or something similar - but that really depends on the actual code.

Daniel Brückner
Agreed! It's not because one doesn't use member-methods that one has no good code style.
xtofl
A: 

This looks a bit like Composite pattern, except that there you can take advantage of the similarity of parents and children in a hierarchical structure to minimize your coding.

You might be able to take advantage of this by having each element in the structure implement an interface that knows how to process its children, e.g.

public interface ChildProcessor { 
   public void process()
}
public A implements ChildProcessor { 
  public void process() { foreach (B b )... }
}
public B implements ChildProcessor { 
  public void process() { foreach (C c )... }
}

public void DoA(A a, Transform transform)
{
   A.process()... 
}
Steve B.
+4  A: 

Looks like Chain-of-Repsonisbility to me. You just process part of the incoming request and then transfer to next item in the chain.

Chain-of-responsibility

In your case it should look like this:

Action action = new Action;
action = action.SetNext(DoA);
action = action.SetNext(DoB);
action = action.SetNext(DoC);

A a = new A();
action.Process(a);
Andrei Taptunov
+1, +answer: I wrangled up the dev responsible for the code and asked what the motivation was for structuring the code the way he did. Apparently, all the methods are `public` for testing purposes, and `virtual` so they can be overridden by RhinoMocks and tested in relative isolation. The tree traversal logic is not altogether complex, and the structure of the code just sort of grew out of control as different people worked on it. I think this suggestion will help clean up the code, so the stack trace is more wide than deep. Thank you :)
Juliet
A: 

I suggest Chain Of Responsibility...

Eduardo Aquiles