How can I avoid code duplication of a static method in multiple classes

Question

I have multiple classes containing duplicated code, especially members and most important a static method that will create a new instance of the class and returning this instance: either a previously created instance registered in a dictionary or a new instance by calling the constructor.

An interface is no option, because I have the static method. I tried to solve the problem by introducing a base class that implements this static method, but I can not find a way to create and return a spefific child class properly.

Below is a code example of the current situation with class A and class B showing duplicated code.

public class A
{
    private static readonly IDictionary<string, A> Registry = new Dictionary<string, A>();
    public string Name { get; set; }

    public A(string name)
    {
        this.Name = name;
    }

    public static A GetA(string instanceName)
    {
        lock (Registry)
        {
            if (!Registry.TryGetValue(instanceName, out var newInstance))
            {
                newInstance = new A(instanceName);
            }
            return newInstance;
        }
    }
}

And then in class B again there is a member Name and the GetX() Method.

public class B
{
    private static readonly IDictionary<string, B> Registry = new Dictionary<string, B>();
    public string Name { get; set; }

    public B(string name)
    {
        this.Name = name;
    }

    public static B GetB(string instanceName)
    {
        lock (Registry)
        {
            if (!Registry.TryGetValue(instanceName, out var newInstance))
            {
                newInstance = new B(instanceName);
            }
            return newInstance;
        }
    }
}

Is there a possibility to avoid this kind of code duplication by introducing a base class or any other way?

Mabe make the base method, accepting a generic type and then create instance from that. see https://stackoverflow.com/questions/731452/create-instance-of-generic-type — Malior, Jan 03 '19 at 10:28
But if dictionary will be static, then you will share the same dictionary over between `A` and `B`, which may be not what you want. — Michał Turczyn, Jan 03 '19 at 10:32
Why do you want to limit the number of instance based on `instanceName` ? Is it allowed that both an instance of `A` and an instance of `B` with the same `instanceName` ? — Spotted, Jan 03 '19 at 11:06
`Name` can be a part of base class/interface. As for `GetX` you can't avoid declaration of it, unless you can make it generic, then *base class* is where it should normally be. You can still declare type specific static method to invoke that generic method for the comfort of those type users. — Sinatr, Jan 03 '19 at 11:11
@Spotted Actually I thought it needs to be public. Using the Activator.CreateInstance suggestion, an exception occurs with a private constructor. — plm36021, Jan 04 '19 at 10:23

John Babb · Accepted Answer · 2019-01-03T10:41:34.763

2

This might be a little cleaner:

public class B: RegistryInstance<B>
{
    public string Name { get; set; }

    public B(string name)
    {
        this.Name = name;
    }
}

public class A : RegistryInstance<A>
{
    public string Name { get; set; }

    public A(string name)
    {
        this.Name = name;
    }
}

public abstract class RegistryInstance<T> where T:class
{
    protected static readonly IDictionary<string, T> Registry = new Dictionary<string, T>();

    public static T GetInstance(string instanceName)
    {
        lock (Registry)
        {
            if (!Registry.TryGetValue(instanceName, out var newInstance))
            {
                newInstance = (T)Activator.CreateInstance(typeof(T), new object[] { instanceName });
                Registry.Add(instanceName, newInstance);
            }
            return newInstance;
        }
    }
}

edited Jan 03 '19 at 10:41

answered Jan 03 '19 at 10:38

John Babb

931
10
19

You forgot to add to the `Registry` when you were creating a new instance. That way, it will never hit the dictionary. – Anderson Pimentel Jan 03 '19 at 10:40
1

@AndersonPimentel, noticed as soon as I posted it. Thanks for noticing. Great answer btw. – John Babb Jan 03 '19 at 10:47
Thanks @JohnBabb for that helpful approach. – plm36021 Jan 03 '19 at 12:03

score 1 · Answer 2 · answered Jan 03 '19 at 10:31

Are you looking for a generic base class?

public abstract class BaseRegistryGetter<T>
{
    private static readonly IDictionary<string, T> Registry = new Dictionary<string, T>();
    public string Name { get; set; }

    public BaseRegistryGetter(string name)
    {
        this.Name = name;
    }

    public static T GetValue (string instanceName, Func<string, T> creator) {
        lock (Registry)
        {
            if (!Registry.TryGetValue(instanceName, out var newInstance))
            {
                newInstance = creator(instanceName);
            }
            return newInstance;
        }
    }
}

And then use it like this:

public class A : BaseRegistryGetter<A>
{
    public A(string name) : base(name)
    {
    }

    public static A GetA(string instanceName)
    {
        return BaseRegistryGetter<A>.GetValue(instanceName, s => new A(s));
    }
}

The source for the awkward approach to make sure there is a string-constructor for A can be found here.

score 0 · Answer 3 · answered Jan 03 '19 at 10:37

I think this should work. You can adapt it to fit your needs. Also, there was a bug in your code: you forgot to add to the Registry when you were creating a new instance.

class Program
{
    static void Main(string[] args)
    {
        A a1 = A.GetInstance("a");
        A a2 = A.GetInstance("aa");
        A a3 = A.GetInstance("a");

        B b1 = B.GetInstance("a");
        B b2 = B.GetInstance("aa");
        B b3 = B.GetInstance("a");

        Console.WriteLine(a1 == a2); //false
        Console.WriteLine(a1 == a3); //true

        Console.WriteLine(b1 == b2); //false
        Console.WriteLine(b1 == b3); //true

        Console.ReadKey();
    }
}

public class A : Generic<A>
{
    public A(string name)
        : base(name)
    {
    }
}

public class B : Generic<B>
{
    public B(string name)
        : base(name)
    {
    }
}

public abstract class Generic<T> where T : Generic<T>
{
    private static readonly IDictionary<string, T> Registry = new Dictionary<string, T>();
    public string Name { get; set; }

    public Generic(string name)
    {
        this.Name = name;
    }

    public static T GetInstance(string instanceName)
    {
        lock (Registry)
        {
            if (!Registry.TryGetValue(instanceName, out var newInstance))
            {
                newInstance = (T)Activator.CreateInstance(typeof(T), instanceName);
                Registry.Add(instanceName, newInstance);
            }
            return newInstance;
        }
    }
}

InBetween · Answer 4 · 2019-01-03T11:40:25.520

All the other answers try to solve this with generics, but it might be the case you wouldn't want to do this. First, it could be an unnecessary restriction further along that could end up causing variance issues. Second, it only solves one level of inheritance, if there is more, you are stuck again with the same problem:

 class Base<T> { ... }
 class A: Base<A> { ... }
 class B: A { //How does the generic base class help? }

There are general solutions without the use generics that entails just a little code duplication. One could be the following:

public class Base
{
    static readonly IDictionary<string, Base> Registry = 
        new Dictionary<string, Base>();

    protected static Base GetBase(string instanceName,
                                  Func<Base> creator)
    {
        lock (Registry)
        {
            if (!Registry.TryGetValue(instanceName, out var newInstance))
            {
                newInstance = creator();
            }   

            return newInstance;
        }
    }

    //...
}

And now yor derived types can impement a strongly typed delegated method:

public class A: Base
{
    public A(string instanceName)
        :base(instanceName)
    {
    }
    public static A GetA(string instanceName)
        => GetBase(instanceName, () => new A(instanceName)) as A;
}

public class B: Base
{
    public B(string instanceName)
        :base(instanceName)
    {
    }
    public static B GetB(string instanceName)
        => GetBase(instanceName, () => new B(instanceName)) as B;
}

Did you forget to add to `Registry` in your first code sample? Could `Registry` instead be a `ConcurrentDictionary` (perhaps where the value is `Lazy`)? — mjwills, Jan 03 '19 at 11:08
@sinatr Sorry about the mess, I really need to go to bed. This is more or less what I had in mind. And yes, another option is making `Register` generic and calling `Activator.CreateInstance` but that relies on reflection... — InBetween, Jan 03 '19 at 11:45

How can I avoid code duplication of a static method in multiple classes

4 Answers4