HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMajor

Extension methods, is this too dirty?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thistooextensionmethodsdirty

Problem

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

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

public static IEnumerable ForEach(this IEnumerable source, Action 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 WithEach(this IEnumerable source, Action 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 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));

Solution

My advice: don't do this.

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

-
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.

-
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.

Context

StackExchange Code Review Q#3491, answer score: 25

Revisions (0)

No revisions yet.