current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 4 down vote favorite
1

I observe the same pattern in a number of my classes but can't extract/abstract it due the tight coupling inside each particular implementation.

Having the class accepting an optional number of dependencies onto its constructor:

public Repository(IBuilder, IFormatter, ILoader, ISelector)

With the single method passing its arguments to the first dependency. Passing result to the second. And so on. Returning result of the last dependency:

public XElement Do(int i)
{
    string q = _builder.Build(i);
    string f = _formatter.Format(q);
    XDocument d = _loader.Load(f);
    XElement r = _selector.Select(d);
    return r;
}

(names of the classes, methods and variables are random, just to illustrate the pattern)

Can it be refactored to decouple the flow from the types of dependencies, arguments and return values?

It also will simplify the (unit) testing because currently I make sure that a dependency N passes its result to a dependency N+1:

[TestMethod]
public void Do_Should_Pass_Result_Of_Build_To_Format()
share|improve this question
1  
Do you use many different implementations of these interfaces, or always the same? –  Gert Arnold Dec 27 '13 at 10:57
    
Post some real code. –  abuzittin gillifirca Dec 27 '13 at 15:55
    
@GertArnold: I have different interfaces (about 5 now) with different implementation and number of dependencies but the patter is same: D(i) calls D(i+1). –  abatishchev Dec 27 '13 at 19:28
    
@abuzittingillifirca: the real code is very similar: 4 dependencies with almost same names call each other in same order. Or 3 of them, or 5. –  abatishchev Dec 27 '13 at 19:30
    
@abatishchev Do_Should_Pass_Result_Of_Build_To_Format: You clearly are testing the implementation details. –  abuzittin gillifirca Dec 27 '13 at 20:10

3 Answers 3

up vote 1 down vote accepted

This is basically a pipe and filter pattern - where you construct some number of filters into a chain and just pass the outputs to the next filter. If you control all instances, then the simple thing is to abstract out to an interface:

 interface IFilter {
     object Filter(object o);
 }

Note the weak typing of using object - if these classes are used somewhere else, implementing IFilter as an explicit interface would probably be preferable. You could do generics, but that makes the chaining harder.

 interface IBuilder : IFilter {
     string Build(int i);
 }

 class Builder : IBuilder {
     public string Build(int i) {
         // Do stuff
     }

     object IBuilder.Filter(object o) {
         return (object)this.Build((int)o);
     }
 }

Now, each filter simply takes an input and transforms to an output. We need a pipe to tie to it all together:

 class Pipeline {
     private IFilter[] Filters { get; set; }

     public Pipeline(params IFilter[] filters) {
        this.Filters = filters;
     }

     public object Execute(object input) {
        foreach (var f in this.Filters) {
            input = f.Filter(input);
        }
        return input;
     }
 }

And, in use:

 class Repository {
     private Pipeline { get; set; }

     public Repository(IBuilder b, IFormatter f, ILoader l, ISelector s) {
         this.Pipeline = new Pipeline(b, f, l, s);
     }

     public XElement Do(int i) {
         return (XElement)this.Pipeline.Execute(i);
     }
 }

Your unit tests for Repository now only need to be concerned with the results of Do - which verifies the Pipeline was constructed properly (an important bit to test, since it's weakly typed).

You can dress it up with generics, extension methods, builder patterns, etc. - but that's the basic pattern.

share|improve this answer
    
Looks like that's exactly I was looking for. Awesome, thanks! –  abatishchev Dec 27 '13 at 20:54
    
But a question please: how to make it generic? I tried to roll out something similar by myself but stuck making it generic. –  abatishchev Dec 27 '13 at 20:55
    
The post under last link is in top of Google for "c# pipes filters generic". It's good but (besides can be refactored nicely with ops.Aggregate(input, (i, op) => op.Exec(i))) its weak part is all input and output parameters have the same type T what makes Aggregate applicable to it and it not applicable to my case as 1:1. –  abatishchev Dec 28 '13 at 6:35
    
If you make it strongly typed, you'll end up having to specify the exact steps as delegates (as others have mentioned). You could do a BasePipeline and then derive an XElementPipeline from it with the steps to prevent having to respecify it in your tests. You may also want to look at Task.ContinueWith which could be useful for chaining with or without async behavior. –  Mark Brackett Dec 30 '13 at 14:54
up vote 3 down vote

One option would be to create a generic method that accepts delegates to the methods:

var combined = Combine(_builder.Build, _formatter.Format, _loader.Load, _selector.Select);
return combined(i);

The implementation would look like this:

public static Func<T1, T5> Combine<T1, T2, T3, T4, T5>(
    Func<T1, T2> f1, Func<T2, T3> f2, Func<T3, T4> f3, Func<T4, T5> f4)
{
    return input =>
    {
        var r1 = f1(input);
        var r2 = f2(r1);
        var r3 = f3(r2);
        var r4 = f4(r3);
        return r4;
    };
}

The implementation would need to have overloads for different number of delegates.

Also, I'm not completely sure whether this actually improves the situation. Your original code is very straightforward, this code is harder to understand, because it's not immediatelly clear what does Combine() do.

share|improve this answer
    
Maybe if you call it Chain it becomes clearer? –  ChrisWue Dec 27 '13 at 17:35
    
Thanks for your answer! I agree that the code is straightforward but its testing is unfortunately not, it's messy. I need to mock all dependencies turn by turn and make sure that the flow takes place (passing result from one to another). I feel like I'm testing a wrong thing. I'd like to have a set of strongly-typed flow steps and just make sure that SUT uses the flow manager. –  abatishchev Dec 27 '13 at 20:00
    
Confusion. This code sample could/should be what the TEST method signature might be, yes? –  radarbob Dec 27 '13 at 23:01
    
No, that's current test name. I have such test for each member/dependency in the underlying flow. Everything is mocked up but still too messy. –  abatishchev Dec 28 '13 at 7:22
    
The main drawback of your solution for me is that the number of "steps" is not dynamic but mandatory and hard-coded. So I need to have separate Chain() for a case of 2,3,4,etc. Also having T1,T2,T3,etc. degrades readability significantly :( –  abatishchev Dec 28 '13 at 7:27
up vote 2 down vote

An alternative to @svick's answer is to use an extension method which only takes in two functions, and then chain them together, as follows:

public static class ExtensionHelper
{
    public static Func<T1,T3> Chain<T1,T2,T3> (this Func<T1,T2> f1, Func<T2,T3> f2)
    {
       return item => f2(f1(item));
    }
}

Then, you can chain any number of functions as follows:

static void Main(string[] args)
{
   Func<int,int> f1 = AddOne;

   var chained3 = f1.Chain(Double).Chain(i => i - 1); // overall: i => ((i + 1) * 2) - 1
   var val = chained3(1); // returns 3
}

public static int AddOne(int i)
{
   return i + 1;
}

public static int Double(int i)
{
   return i * 2;
}

The downside is that you have to declare the initial function as a Func type to start the chain. You cannot use the method group or a lambda directly.

share|improve this answer
    
Add this static method to your extension class: Identitx<T>() = (x)=>x. Then you can say Identity<int>.Chain(class1.method1).Chain(class2.method2). –  abuzittin gillifirca Dec 29 '13 at 9:15

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.