Extension methods, is this too dirty?

Question

I love lambdas, and functional programming etc. But sometimes I wonder if I take it too far..

public static T With<T>(this T source, Action<T> action)
{
    action(source);
    return source;
}

public static IEnumerable<T> ForEach<T>(this IEnumerable<T> source, Action<T> action)
{
    return source.Select(x => x.With(action));
}

thoughts?

UPDATE

Some good points, some I agree with, some I don't, but a few details:

I understand that Linq may not be intended to handle side effects, especially since List.ForEach returns void, however I disagree that functional programming can't contain side effects, a monad is simply a chunk of code to execute for a given structure.

The ForEach and With extensions, are meant to be a syntactic sugar. Perhaps the name is poorly chosen, and it should be something like OnEnumerate, however I wonder if this could get confusing down the line, as its whole point is to invoke side effects at that particular point in the expression, and this could occur multiple times, perhaps I should stick with my With convention and do WithEach.

The comments about using Select internally are probably true, as I'm not really projecting, and yield return saves a method call. (premature optimization anyone?)

public static IEnumerable<T> WithEach<T>(this IEnumerable<T> source, Action<T> action)
{
    foreach(var x in source)
        yield return x.With(action);
}

To be honest I probably wouldn't use WithEach often, unless its an edge case where I'm passing an IEnumerable<T> that may or may not be enumerated. I can imagine it being effective in generating complex hierarchies. However With I use quite often, usually as a way to chain things together, i.e:

this.SomeObservable = someObservable
    .With(o => o.Subscribe(this.OnNext)
        .With(this._disposables.Add));

Answer 1

My advice: don't do this.

1. Building devices that encourage you to write expressions that are useful only for their side effects is poor functional style.

2. You are abusing "Select" when you use it as an implementation detail of "ForEach". "Select" does not have the semantics of "ForEach"! The purpose of "Select" is to compute a projection, not to cause a side effect.

3. You are building yourself a little time bomb. Because your implemenation of ForEach is to build a Select query, the "ForEach" executes lazily. When you say "blah.ForEach(whatever);" that doesn't do anything. It just builds up an object that represents doing the side effects in the future. Since you never use that object for anything, the side effects remain undone. If, by contrast, you do use that object, you run the risk of using it twice and causing the same side effects again.

That said, the idea of what you're doing is sound; the idea of representing a sequence of side-effecting operations in a lazily-evaluated object is how side effects are represented in Haskell. But if that's what you want to do, then actually build yourself a real monad that clearly and unambiguosly represents the semantics of "I'm building an object that represents a sequence of side effects in the future". Don't be all coy about it! Be explicit that that's what you're doing.

Answer 2

I don't think this really provides a lot of value beyond the existing .Select() projection. Anything you write into your action you could also write into a .Select() lambda.

For example:

myItems.Select(item => 
{
   item.DoSomethingWeird();
   return item;
}).Select(item =>
{
   item.DoSomethingWeird(); //again
   return item;
});

Also, I wouldn't appropriate the ForEach name here. It has other, conflicting means. "WithAll()" might be a better name.

Answer 3

Enumerating something using an extension is not supposed to cause side effects.

If you make an extension that is intended to cause side effects, it can easily come back and bite you. As the enumeration uses deterred execution, something like this would have no effect:

myList.Foreach(x => Console.WriteLine(x));

As you are not consuming the result, there is nothing that pulls the iteration, so it won't iterate anything at all. It will just create code that is capable of iterating, and throw it away.

Answer 4

I am guessing EricLippert was referring to something like this when stating:

That said, the idea of what you're doing is sound; the idea of representing a sequence of side-effecting operations in a lazily-evaluated object is how side effects are represented in Haskell. But if that's what you want to do, then actually build yourself a real monad that clearly and unambiguosly represents the semantics of "I'm building an object that represents a sequence of side effects in the future".

//was ForEach
/// <summary>
/// Causes some action when the element in the enumeration is touched. 
/// The action will only happen the first time the element is touched.
/// </summary>
/// <typeparam name="T">The type of element in the enumerable</typeparam>
/// <param name="source">The enumerable source</param>
/// <param name="action">the action to execute</param>
/// <returns>An enumerable representing the source which will execute the action when elements are enumerated</returns>
public static IEnumerable<T> OnEnumeration<T>(this IEnumerable<T> source, Action<T> action) 
{
    return new FutureSideEffect<T>(source, x => x.With(action));
}

public class FutureSideEffect<T> : IEnumerable<T>, IEnumerator<T> 
{
    readonly IEnumerable<T> enumerable;
    readonly List<T> enumerated = new List<T>();
    readonly Stack<IEnumerator<T>> enumeratorStack = new Stack<IEnumerator<T>>();

    public FutureSideEffect(IEnumerable<T> enumerable, Action<T> action) 
    {
        this.enumerable = enumerable.Select(x =>
            {
                action(x); 
                return x;
            }
        );
    }

    public IEnumerator<T> GetEnumerator()
    {
        return this;
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public bool MoveNext()
    {
        IEnumerator<T> enumerator = null;
        if (enumeratorStack.Count != 0) 
        {
            enumerator = enumeratorStack.Peek();
            if (enumerator == null) 
            {
                enumeratorStack.Pop();
                enumerator = enumerated.GetEnumerator();
                enumeratorStack.Push(enumerator);
            }
        }
        else
        {
            enumerator = enumerable.GetEnumerator();
            enumeratorStack.Push(enumerator);
        }
        var result = enumerator.MoveNext();
        if (!result && enumeratorStack.Count > 1)
        {
            enumeratorStack.Pop();
            return MoveNext();
        }
        return result;
    }
    public void Reset() 
    {
        IEnumerator<T> enumerator = null;
        if (enumeratorStack.Count != 0) 
        {
            enumerator = enumeratorStack.Peek();
        }
        if (enumerator == null)
        {
            return;
        }
        enumeratorStack.Push(null);
    }
    public T Current 
    {
        get 
        {
            var current = enumeratorStack.Peek().Current;
            if (enumeratorStack.Count == 1)
            {
                enumerated.Add(current);
            }
            return current;
        }
    }
    object IEnumerator.Current 
    {
        get 
        {
            return Current;
        }
    }

    public void Dispose() 
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    //finalizer unnecessary because there are no unmanaged resources
    protected virtual void Dispose(bool disposing) 
    {
        if (disposing) 
        {
            enumerated.Clear();
            while (enumeratorStack.Count != 0)
            {
                var enumerator = enumeratorStack.Pop();
                if (enumerator != null) 
                {
                    enumerator.Dispose();
                    enumerator = null;
                }
            }
            //don't touch enumerable because I don't own it
        }
    }
}

I built a small console app to play around with this, but I can't really think of any reason I might want to use it and I didn't bother remotely trying to consider aspects of thread safety.

Answer 5

Because the List<T> already defines ForEach() as a void method, I would not want to get that mixed up with a "ForEach()" that is also a Select().

Answer 6

In addition to the semantic problems described by Eric Lippert, you should avoid this because it causes namespace pollution.

By adding extension methods to any object, you're diluting the usefulness of intellisense. You should avoid this because it can become messy (and confusing) quickly. C# already has syntax for calling methods, you should ask yourself if you really want another way of expressing the same thing.

Alternatively, consider a higher-order function:

static Func<T,T> SideEffect(Action<T> action) { return x=>{action(); return x;} }

Which you might use as follows:

var lazylist = 
    something.Select(
        SideEffect(item => Console.WriteLine("iteration logged "+item)));

Note that side effects make code much harder to understand; this is explained in other answers.

Answer 7

All that as opposed to this?:

foreach(var item in source)
{
     //do something?
}

Extension methods are great when they don't cause side effects but this would violate the standard functional approach taken with LINQ.

It can hide the point of your method chain:

 source.Where(x => x.IsValid)
     .With(x => x.MakeInvalid())
     .Count();

Obviously a contrived example but the point is that using chains with side effects can be a pattern which hide the code's meaning.