AlbedoOrg / Albedo

A .NET library targeted at making Reflection programming more consistent, using a common set of abstractions and utilities
MIT License
166 stars 17 forks source link

ReflectionElementFactory #24

Closed ploeh closed 10 years ago

ploeh commented 10 years ago

It often happens (at least in the AutoFixture code base) that you have an object that may be some Reflection object, like PropertyInfo, or ParameterInfo, but you don't know which.

The whole point of Albedo is to provide a polymorphic API over Reflection, so it'd be nice to be able to automatically translate from object to IReflectionElement.

Something like

var re = obj.AsReflectionElement()

However, as far as I can tell, there are at least two different scenarios:

Perhaps there are more scenarios? This indicates to me that we should implement this as a proper extensible Factory class.

Something like this should do the trick:

public interface IReflectionElementPipe<T>
{
    IEnumerable<IReflectionElement> Pipe(IEnumerable<T> source);
}

(I'm not quite sure about the name, so suggestions are welcome...)

While T would often be object, the generic type argument exists to support scenarios where you know that you can only receive objects that implement IMemberInfo, in which case you can declare the T as IMemberInfo...

Now you can implement an individual pipe like:

public class PropertyInfoElementPipe<T> : IReflectionElementPipe<T>
{
    public IEnumerable<IReflectionElement> Pipe(IEnumerable<T> source)
    {
        return source
            .OfType<PropertyInfo>()
            .Select(pi => new PropertyInfoElement(pi));
    }
}

Given all these, you could implement the actual factory as a composite of all those...

ploeh commented 10 years ago

I'm not happy about the Pipe name - usually, a Pipe exhibits Closure of Operation, which the proposed interface doesn't.

Alternatives:

adamchester commented 10 years ago

What about implicit or explicit cast operators?

adamchester commented 10 years ago

What's interesting is (rightly or wrongly) I started to feel to reflection elements had the same identity as the object they wrap.

adamchester commented 10 years ago

Of course, since we can't overload operators via extension methods, that's not going to work.

moodmosaic commented 10 years ago

I think that, in our context, we care more about the transformation part – object to IReflectionElement.

Since input is a different type from the output, transformation tend to match closer with the concept of converting an object to an IReflectionElement.

Funnel, and Conduit, tend to fall in the same category with Pipe, I think.

ploeh commented 10 years ago

Regarding implicit operators, you can't create an implicit conversion to an interface - I wish you could, but that's illegal in C#.

Regarding naming, I'm actually liking Builder more and more... oO

moodmosaic commented 10 years ago

Ah, yes.. I focused only at the AsReflectionElement (feature envy/extension-)method :S

Perhaps that method could live in a class suffixed with Transformation/Translation...

Naming the actual (creator) class as Builder sounds great. Though, I was wondering if the creation part is going to be so complex to require a Builder...

ploeh commented 10 years ago

Builder may be a bit of a stretch, if you want to take the pattern literally, but on the other hand, if you consider the input of a heterogeneous sequence of objects, each object goes through a series of steps, where it's being tested for its type and subsequently, if a match is made, an appropriate IReflectionElement is created... so I don't think it's completely off, either...

None of the other options I've been able to think of are spot on...

adamchester commented 10 years ago
moodmosaic commented 10 years ago

I think, Builder that @ploeh suggested matches the scenario.

adamchester commented 10 years ago

How well does this fit?

http://en.m.wikipedia.org/wiki/Adapter_pattern

adamchester commented 10 years ago

I'm wondering in what scenario that @ploeh was thinking of where there would be a sequence of conversions at the same time?

The original example used IEnumerable.

adamchester commented 10 years ago

I suppose it's not hard to imagine actually, I wasn't thinking very far at all. Just doing a reflection query to get all methods is a sequence to convert :)

adamchester commented 10 years ago

Well, adapter is my pick, I don't like builder.

adamchester commented 10 years ago

Adapter might make more sense if the reflection elements exposed more about the underlying element, is that part of the plan?

moodmosaic commented 10 years ago

Is the problem we are trying to solve creational or structural?

adamchester commented 10 years ago

You are right, I see my confusion now, the Adapter pattern doesn't make sense for this proposed 'factory'; it's closer to the IReflectionElement instances themselves.

adamchester commented 10 years ago

What's the advantage over something like this? Obviously it's not optimised for speed or anything, but I think it illustrates what the proposed factory would look without the pipe/builder extensible factory mechanism being proposed?

public static class ReflectionElementFactory
{
    private class ElementMetadata
    {
        public Type ElementType { get; set; }
        public ConstructorInfo Ctor { get; set; }
        public Type ReflectionType { get; set; }

        public IReflectionElement Create(object source)
        {
            return (IReflectionElement)this.Ctor.Invoke(new[] { source });
        }
    }

    public static IReflectionElement AsReflectionElement(
        this object source,
        bool throwOnUnknownSourceType = false)
    {
        var reflectElementType = typeof(IReflectionElement);
        var dictionary = reflectElementType
            .Assembly
            .GetExportedTypes()
            .Where(t => t.IsAssignableFrom(reflectElementType))
            .Select(t => new ElementMetadata
            {
                ElementType = t,
                Ctor = t.GetConstructors()[0],
                ReflectionType = t.GetConstructors()[0].GetParameters()[0].ParameterType
            })
            .ToDictionary(em => em.ReflectionType, em => em);

        var sourceType = source.GetType();

        ElementMetadata metadata;
        bool matchingTypeFound = dictionary.TryGetValue(sourceType, out metadata);

        if (throwOnUnknownSourceType&& !matchingTypeFound)
        {
            throw new ArgumentOutOfRangeException(
                "source", "not a System.Reflection object instance");
        }
        return metadata == null ? new NullReflectionElement() : metadata.Create(source);
    }
}
ploeh commented 10 years ago

The problem with that approach is that it isn't extensible. The fact that there's already two scenarios makes me uneasy, because in my experience, there's rarely anything magical about the number 2. If there are two strategies, there are probably more - we just don't know what they are yet... That's why I think that extensibility is important in this case.

Also, I'd like to refer to the Flag Arguments code smell in Clean Code, and follow up with Replace Conditional with Polymorphism from Refactoring - those are the motivations for my current suggestion.

If we didn't care about extensibility, we could easily implement the method like this:

public static IReflectionElement AsReflectionElement(this object source)
{
    var a = source as Assembly;
    if (a != null)
        return new AssemblyElement(a);
    var t = source as Type;
    if (t != null)
        return new TypeElement(t);
    var mi = source as MethodInfo;
    if (mi != null)
        return new MethodInfoElement(mi);

    // etc.
}
ploeh commented 10 years ago

... however, I do accept that Builder probably isn't a good name, so I'm still open to suggestions :)

ploeh commented 10 years ago

Perhaps it is a Factory after all :)

but I like @adamchester 's Materializer suggestion as well...

moodmosaic commented 10 years ago

I like Materializer too :)

Wouldn't Metamophosis (a transformation that happens in some species) result to a scary-sounding API? :)

ploeh commented 10 years ago

OK, Materializer, then :)

ploeh commented 10 years ago

FYI, I just pushed AssemblyElementMaterializer<T> to master...

ploeh commented 10 years ago

FYI, I just pushed TypeElementMaterializer<T> to master...

adamchester commented 10 years ago

I was attempting to do LocalVariableInfoElementMaterializer<T> when I noticed you didn't attempt to test the OfType<X> (filtering) part of the sequence materializer. Was this on purpose?

I was going to do have a test like MaterializeAssembliesReturnsCorrectResult however using ClassData (because, I thought it was slightly more readable than using InlineData with Type[].

class MaterializeSources : IEnumerable<object[]>
{
    public IEnumerator<object[]> GetEnumerator()
    {
        // Test materializing a single NullReflectionElement
        yield return new object[] {new object()};

        // Tests materializing a single LocalVariableInfo
        yield return new object[]
        {
            new object[] { TypeWithLocalVariable.LocalVariable }
        };

        // Tests materializing a multiple LocalVariableInfo
        yield return new object[]
        {
            new object[]
            {
                TypeWithLocalVariable.LocalVariable,
                TypeWithLocalVariable.OtherLocalVariable
            }
        };

        // Tests materializing a NullReflectionElement between two LocalVariableInfo's
        yield return new object[]
        {
            new object[]
            {
                TypeWithLocalVariable.LocalVariable,
                null,
                TypeWithLocalVariable.OtherLocalVariable
            }
        };
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}
adamchester commented 10 years ago

I just noticed TypeElementMaterializerTests does actually test the OfType<T>, doing something like what I just proposed above.

ploeh commented 10 years ago

FYI, I just pushed ConstructorInfoElementMaterializer<T> to master...

ploeh commented 10 years ago

FYI, I just pushed EventInfoElementMaterializer<T> to master...

adamchester commented 10 years ago

Starting PropertyInfoElementMaterializer<T>... in case somebody was doing them in order, because ParameterInfoElement<T> was next in the alphabetical list ;)

ploeh commented 10 years ago

:+1:

adamchester commented 10 years ago

Well, nobody said stop, so I wont. ParameterInfoElement<T> coming...

moodmosaic commented 10 years ago

Nice! :)

ploeh commented 10 years ago

FYI, I just pushed FieldInfoElementMaterializer<T> to master...

adamchester commented 10 years ago

That's the last one right?

ploeh commented 10 years ago

Yes, I believe this is the last leaf materializer, but we still need to do the Composites...

ploeh commented 10 years ago

I think it would be beneficial to move all this stuff into a sub-namespace, for the following reasons:

This again triggers considerations regarding naming. An obvious name would be Ploeh.Albedo.Materialization, but I had a new thought:

What if we take a cue from the Albedo name and use wording or terminology related to (astronomical) Albedo. I was considering

If we do that, I think we should also rename IReflectionElementMaterializer<T> and its method accordingly.

You could argue that no-one knows what Absorb or Refract means (in a programming context), and I would agree. On the other hand, no-one really knows what Materialize means either... I don't mean this as a criticism of @adamchester's suggestion (I still think Materializer is the best of all previous candidates), but a Materializer is a bit like a Manager. As a programmer, you don't really understand what it does, and you'll have to refer to the documentation in any case.

With words like Absorption or Refraction, we would take words that currently holds no particular meaning in a programming context, and make them mean something in this specific API. Essentially, this is no different from what the Gang of Four did when they named many of their design patterns.

What do you think? Too obscure? Too cute? Or is it a good idea?

moodmosaic commented 10 years ago

It is a good idea, I think.

Also, moving the transformer classes to a separate namespace looks like a good idea.

A hypothetical façade would be just fine since the transformer classes are more likely to be used in advanced scenarios.

adamchester commented 10 years ago

Well, materialise does have a little meaning. For example, to 'materialise' something is to bring something into being, to 'make it real'. Whereas 'manager' could do just about anything IMO.

Having said that, I like your idea more. Besides, if it eventually gets too confusing, we'll know soon enough, so there's not much risk giving it a try :)

moodmosaic commented 10 years ago

With Semantic Versioning we won't be able to just go on and rename the classes though.. So, I think, we need to carefully consider the naming.. :)

adamchester commented 10 years ago

I think it's fair to be loose with semver in the early stages, right?

adamchester commented 10 years ago

It even says "

How do I know when to release 1.0.0?

If your software is being used in production, it should probably already be 1.0.0. If you have a stable API on which users have come to depend, you should be 1.0.0. If you're worrying a lot about backwards compatibility, you should probably already be 1.0.0.

ploeh commented 10 years ago

I agree with @adamchester that we're still allowed to change out minds, but once we release 1.0.0, we can't change the name just like that.

However, what we can still do after 1.0.0 is to add a new namespace with better names, if it turns out that the initial names are too confusing, and then deprecate the old names.

Absorption or Refraction?

moodmosaic commented 10 years ago

Refraction here. Mostly because the Re part matches the _Re_flection word.. (It tends to be more compatible as a word to me.)

moodmosaic commented 10 years ago

Oh, about versioning, agreed with @adamchester and @ploeh.

(I thought I submitted a comment earlier but.. my brain tends to be eventually consistent today..!) :)

adamchester commented 10 years ago

Absorption sounds like we're cleaning up instances of reflection elements by using hand towel from the kitchen.

Refraction sounds like the beam of light has had its path slightly altered, I like that.

moodmosaic commented 10 years ago

FWIW, here is another word: Antanaclasis - Idiomatically, it means reflection.

ploeh commented 10 years ago

OK, thanks for teaching me a new word today :)

According to Wikipedia (where I've just spent the last 10 minutes), Atanaclasis is related to rhetoric, which isn't so much related to astronomy :)

Let's go with Refraction - it sounds like all three of us like that, for various reasons.

moodmosaic commented 10 years ago

Agreed :)


In general, I think that in the field of programming, words like Antanaclasis, Metamorphosis, as well as many other Greek words with English derivatives could be used less wisely (otherwise we will run out of words eventually - or we will have to reuse the same words everywhere).

Other fields (e.g. medicine, physics, economics) already use less known (but meaningful) words :)

ploeh commented 10 years ago

FYI, I've now performed the rename to Refraction. I've searched the entire solution for 'materialize' and I believe that I've weeded out all occurrences of 'materialize', but if you find a remnant, please let me know (or deal with it).