3
\$\begingroup\$

Here is a class that exists in the Framework\ namespace in the xofz.Core library. It is used as a dependency of other Framework\ and Presentation\ classes, where the Framework namespace contains non-presentation or UI layer framework or infrastructure code.

I think it helps deal with ever-changing constructor dependencies in non-UI code. All that is done to use it is something along the lines of web.RegisterDependency(new ConcreteDependency(web)) and then consuming code may access that live object by any of its interfaces or its implementation (though that is usually hidden away from the implementation namespace).

namespace xofz.Framework
{
    using System;
    using System.Collections.Generic;
    using System.Linq;

    public class MethodWeb
    {
        public MethodWeb()
        {
            this.dependencies = new List<Tuple<object, string>>();
        }

        public virtual void RegisterDependency(
            object dependency, 
            string name = null)
        {
            this.dependencies.Add(
                Tuple.Create(dependency, name));
        }

        public virtual T Run<T>(
            Action<T> method = null,
            string dependencyName = null)
        {
            var dependency = this.dependencies
                .Where(tuple => tuple.Item1 is T)
                .FirstOrDefault(tuple => tuple.Item2 == dependencyName);
            if (dependency == null)
            {
                return default(T);
            }

            var t = (T)dependency.Item1;
            method?.Invoke(t);

            return t;
        }

        public virtual Tuple<T, U> Run<T, U>(
            Action<T, U> method = null,
            string dependency1Name = null,
            string dependency2Name = null)
        {
            var ds = this.dependencies;
            var dep1 = ds
                .Where(tuple => tuple.Item1 is T)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency1Name);
            var dep2 = ds
                .Where(tuple => tuple.Item1 is U)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency2Name);
            if (dep1 == null || dep2 == null)
            {
                return Tuple.Create(
                    default(T),
                    default(U));
            }

            var t = (T)dep1.Item1;
            var u = (U)dep2.Item1;
            method?.Invoke(t, u);

            return Tuple.Create(t, u);
        }

        public virtual Tuple<T, U, V> Run<T, U, V>(
            Action<T, U, V> method = null,
            string dependency1Name = null,
            string dependency2Name = null,
            string dependency3Name = null)
        {
            var ds = this.dependencies;
            var dep1 = ds
                .Where(tuple => tuple.Item1 is T)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency1Name);
            var dep2 = ds
                .Where(tuple => tuple.Item1 is U)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency2Name);
            var dep3 = ds
                .Where(tuple => tuple.Item1 is V)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency3Name);
            if (dep1 == null || dep2 == null || dep3 == null)
            {
                return Tuple.Create(
                    default(T),
                    default(U),
                    default(V));
            }

            var t = (T)dep1.Item1;
            var u = (U)dep2.Item1;
            var v = (V)dep3.Item1;
            method?.Invoke(t, u, v);

            return Tuple.Create(t, u, v);
        }

        public virtual Tuple<T, U, V, W> Run<T, U, V, W>(
            Action<T, U, V, W> method = null,
            string dependency1Name = null,
            string dependency2Name = null,
            string dependency3Name = null,
            string dependency4Name = null)
        {
            var ds = this.dependencies;
            var dep1 = ds
                .Where(tuple => tuple.Item1 is T)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency1Name);
            var dep2 = ds
                .Where(tuple => tuple.Item1 is U)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency2Name);
            var dep3 = ds
                .Where(tuple => tuple.Item1 is V)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency3Name);
            var dep4 = ds
                .Where(tuple => tuple.Item1 is W)
                .FirstOrDefault(
                    tuple => tuple.Item2 == dependency4Name);
            if (dep1 == null
                || dep2 == null
                || dep3 == null
                || dep4 == null)
            {
                return Tuple.Create(
                    default(T),
                    default(U),
                    default(V),
                    default(W));
            }

            var t = (T)dep1.Item1;
            var u = (U)dep2.Item1;
            var v = (V)dep3.Item1;
            var w = (W)dep4.Item1;
            method?.Invoke(t, u, v, w);

            return Tuple.Create(t, u, v, w);
        }

        public virtual U Run<T, U>(
            Func<T, U> method,
            string dependencyName = null)
        {
            var dependency = this.dependencies
                .Where(tuple => tuple.Item1 is T)
                .FirstOrDefault(tuple => tuple.Item2 == dependencyName);
            if (dependency == null)
            {
                return default(U);
            }

            return method((T)dependency.Item1);
        }

        public virtual void Subscribe<T>(
            string eventName, 
            Action eventHandler, 
            string dependencyName = null)
        {
            this.subscribeInternal<T>(
                eventName,
                eventHandler,
                dependencyName);
        }

        public virtual void Subscribe<T, U>(
            string eventName,
            Action<U> eventHandler,
            string dependencyName = null)
        {
            this.subscribeInternal<T>(
                eventName,
                eventHandler,
                dependencyName);
        }

        private void subscribeInternal<T>(
            string eventName,
            Delegate eventHandler,
            string dependencyName = null)
        {
            var dependency = this.dependencies
                .Where(tuple => tuple.Item1 is T)
                .FirstOrDefault(tuple => tuple.Item2 == dependencyName);
            if (dependency == null)
            {
                return;
            }

            var e = dependency.Item1
                .GetType()
                .GetEvent(eventName);
            e.AddEventHandler(
                dependency.Item1,
                eventHandler);
        }

        public virtual void Unsubscribe<T>(
            string eventName, 
            Action eventHandler,
            string dependencyName = null)
        {
            this.unsubscribeInternal<T>(
                eventName,
                eventHandler,
                dependencyName);
        }

        public virtual void Unsubscribe<T, U>(
            string eventName,
            Action<U> eventHandler,
            string dependencyName = null)
        {
            this.unsubscribeInternal<T>(
                eventName,
                eventHandler,
                dependencyName);
        }

        private void unsubscribeInternal<T>(
            string eventName,
            Delegate eventHandler,
            string dependencyName = null)
        {
            var dependency = this.dependencies
                .Where(tuple => tuple.Item1 is T)
                .FirstOrDefault(tuple => tuple.Item2 == dependencyName);
            if (dependency == null)
            {
                return;
            }

            var e = dependency.Item1
                .GetType()
                .GetEvent(eventName);
            e.RemoveEventHandler(
                dependency.Item1,
                eventHandler);
        }

        private readonly List<Tuple<object, string>> dependencies;
    }
}

My only thoughts currently are that the Subscribe and Unsubscribe methods are kind of extraneous, due to being able to subscribe in an Action delegate passed in to Run<T>().

\$\endgroup\$
  • 1
    \$\begingroup\$ Sorry, what's the usage scenario? Kind of DI for methods? Can you please provide an use-case? \$\endgroup\$ – Adriano Repetti Jan 10 '18 at 9:51
  • \$\begingroup\$ You are aware that there is like of bajillion DI containers that pretty much exist for the sole purpose of managing dependencies in larger projects, right? \$\endgroup\$ – Nikita B Jan 10 '18 at 13:52
1
\$\begingroup\$

It wasn't immediately obvious to me what your class does or what problems it tries to solve, so here's a quick summary of what can be done with it:

// You can register objects, which are identified by their type and an optional name:
mw.RegisterDependency(store, "main-store");
// mainStore = store;

// You can add and remove event handlers on any registered object:
mw.Subscribe<Store>(nameof(Store.ItemSold), Store_ItemSold, "main-store");
mw.Unsubscribe<Store>(nameof(Store.ItemSold), Store_ItemSold, "main-store");
// mainStore.ItemSold += Store_ItemSold;

// And you can invoke methods, while letting the MethodWeb resolve their parameters:
mw.Run<Store>(UpdateStore, "main-store");
// UpdateStore(mainStore);

It looks like the core functionality here is the ability to resolve objects. Adding/removing event handlers or using objects as method parameters is something that can easily be done outside this class: mw.Get<Store>("main-store").ItemSold += Store_ItemSold and UpdateStore(mw.Get<Store>("main-store")).

Event subscription

  • Problems like incorrect event handler signatures or non-existing event name are normally detectable at compile-time. Here, they are only detected at run-time, which decreases the code's reliability.
  • Specifying a non-existing event name results in a non-descriptive NullReferenceException. You'll also have to use the nameof operator if you want to keep things somewhat refactor-friendly.
  • Subscribe and Unsubscribe don't report any error if the specified dependency does not exist. This means that things will silently fail if you subscribe before registering. If that is intentional then I'd recommend clearly documenting it.
  • (object sender, EventArgs e) or some variation on that is a common 'shape' for events, but only event handlers that take 0 or 1 arguments are allowed.

Method invocation

  • Run does not call the given method if any dependency cannot be resolved, and returns a tuple of default values. That makes it difficult for the caller to determine which dependency is missing.
  • Why is there only one Func<> overload? And what about async methods? This seems more restrictive than first resolving dependencies and then calling a method directly.
  • Is there a specific reason why the Run methods are so tightly coupled to dependency resolution? They do need to resolve things, but they don't need to know how those things are resolved, so they might as well be separate utility methods.

Dependency resolution

  • Objects are identified by their type and by an optional name, but adding multiple objects with the same type and name is allowed - they just won't be resolved due to registration order. You may want to disallow that. You may also want to document how exactly object resolution works.
  • If you register two objects of different type, that implement the same interface, and you use the interface to resolve an object, which one do you get? Is that behavior intentional?
  • The dependency resolution code is duplicated all over the place. Granted, it's only a few lines, but what if you need to change the behavior? You'll have to modify each place, and it's easy to overlook a few, which may cause (subtle) problems. I'd write a method for it.
  • Roughly how many dependencies do you expect to have? Is a list a suitable choice for that, or would a dictionary make more sense?
  • What about ownership, object lifetime and disposal? Is there a reason for not providing an Unregister method?

Other notes

  • This looks like a piece of low-level 'plumbing' code that lots of other code will rely on. With that sort of code I always try to give more attention to documentation and error handling.
  • Why are almost all methods virtual?
  • Consider using T1, T2, T3, T4 and TResult as type names for the Run methods. This makes their purpose more clear (it also matches the naming of Action<> and Func<>) and the numbers match with the dependency name parameters. For type parameters, I prefer to use T as a prefix, followed by a short but meaningful name.
\$\endgroup\$
1
\$\begingroup\$

I feel like too many things are missing from your implementation API-wise to consider this code useful in general case (as public part of a framework that other people will use). Pieter already made a nice list of what is wrong. I can only add that:

1) There is no Unregister method. If I register a disposable object, how can I safely dispose it, if a reference to it is kept for a lifetime of MethodWeb?

2) There is a lot of code duplication. For example this code:

        var dep1 = ds
            .Where(tuple => tuple.Item1 is T)
            .FirstOrDefault(
                tuple => tuple.Item2 == dependency1Name);

is repeated over and over for every generic parameter.

3) In above code snippet, you are using a linear search, which is O(n). It will become a huge overhead for larger lists of registered objects. To the point that it will take more time to find parameters, than to actually execute a method. Consider using hash-based lookups or more complex adaptive algorithms.

4) Lazy creation of registered objects is not supported. So I have to always instantiate every dependency regardless of which methods are actually called afterwards. Seems wasteful.

P.S. I can't shake off the feeling, that if you are injecting parameters to some method using predefined list of dependencies, then it might make more sense to just inject those dependencies straight into component's constructor. I mean what is the point of having method with parameters, if those parameters do not change? But I do not fully understand the use-case, so I might be missing the point here.

\$\endgroup\$
1
\$\begingroup\$

I think it helps deal with ever-changing constructor dependencies in non-UI code.

I don't know why the constructor dependencies are ever-changing. Aside from that, if the class's dependencies change the constructor dependencies should change. The constructor should tell you what the class depends on.

That's one of the problems with a service locator. Your class never really depends on it. Instead, your class depends on what it will get from it. And that could be anything.

So even if you're not using strings to specify dependency names, you still run a greater risk of runtime errors, because your class can call the service locator and ask for a dependency like this:

var thingIneed = _serviceLocator.GiveMe<Thing>

when _serviceLocator doesn't actually know how to resolve a Thing. Granted, that can still happen with a DI container, but with a DI container you can package your registrations into a class and then test that it can resolve an entire object with all of its nested dependencies.

But the other huge problem is that a class that gets its dependencies from a service locator is harder to test. Now instead of mocking the dependency, you have to a) figure out what the class is getting from the service locator, b) create a mock (or test double) for the resolved dependency, and somehow mock your service locator to return your mock. It's not fun, and it makes unit tests confusing to set up and hard to read.

And, every time a mock returns a mock a child's kitten turns into an insect.

Some poor child's kitten has turned into an insect because a mock returned a mock.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.