Please take a look at this code snippet. What would you say about it?

    public class TreatmentDataProviderFactory
    {
        private IDataAccess _dataAccess;
        private TreatmentDataProvider _treatmentDataProviderHost;
        private TreatmentDataProviderField _treatmentDataProviderField;

        public TreatmentDataProviderFactory(IDataAccess dataAccess)
        {
            _dataAccess = dataAccess;
        }

        public ITreatmentDataProvider Provider
        {
            get
            {
                if (SomeInteractionSingleton.PluginHost.GetWorkMode() == WorkMode.ConnectedMode)
                {
                    if (_treatmentDataProviderHost == null)
                    {
                        _treatmentDataProviderHost = new TreatmentDataProvider(_dataAccess);
                    }
                    return _treatmentDataProviderHost;
                }
                else
                {
                    if (_treatmentDataProviderField == null)
                    {
                        _treatmentDataProviderField = new TreatmentDataProviderField(_dataAccess);
                    }
                    return _treatmentDataProviderField;
                }
            }
        }
    }

Doesn’t it smell bad a lot?

1) Let’s start with obvious: properties in C# are not intended to be 10-20 lines long and having complex logic. Anyway I won’t wrote post if this was a problem.

2) Now the worst mistake here: how am I supposed to test this code if it uses SomeInteractionSingleton in the if condition. Why should this code be so coupled to SomeInteractionSingleton? I wrote this post because developer didn’t run tests. If he ran the tests, he would see they fail.

3) Now second bad mistake: this code keeps two instances of data providers. We can suppose that this is storage for two providers or something? :) I think GC is designed for something and lifecycle of objects shouldn’t be treated as this. I thought IoC is invented for something like this. At least developers should not keep code that much coupled and deliver creation logic.

4) Another mistake: Is this a Factory Method design pattern? – Almost. At least it has such name and looks like it (a bit). But it doesn’t meet either its original definition or either Parameterized Factory Method definition.

I redesigned this class. See:

    public class TreatmentDataProvider
    {
        protected TreatmentDataProviderHost TreatmentDataProviderHost { get; private set; }
        protected TreatmentDataProviderField TreatmentDataProviderField { get; private set; }
        protected IPluginHost PluginHost { get; private set; }

        public TreatmentDataProvider(IPluginHost pluginHost, TreatmentDataProviderHost treatmentDataProviderHost, TreatmentDataProviderField treatmentDataProviderField)
        {
            PluginHost = pluginHost;
            TreatmentDataProviderHost = treatmentDataProviderHost;
            TreatmentDataProviderField = treatmentDataProviderField;
        }

        public ITreatmentDataProvider Provider
        {
            get
            {
                if (PluginHost.GetWorkMode() == WorkMode.ConnectedMode)
                {
                    return TreatmentDataProviderHost;
                }
                else
                {
                    return TreatmentDataProviderField;
                }
            }
        }
    }

Code above does the same logic, but it delivered control of creation of instances to other parties (IoC), also it now doesn’t have dependency on static methods, so code is less coupled. I didn’t remove this class as we have to keep verification for WorkMode at runtime. I know you might complain about these protected properties, but I like to have it that way for more flexibility when testing.

What are your thoughts? [Sentence removed 10/18/2011]

[Added 10/18/2011]

5) Yet another big mistake: Code reviewer. Guess who he was. When I’ve been reviewing this code by request of developer I just thought “it works, then it should be ok”. Why didn’t I ask about unit tests and why didn’t I took reviewing more scrupulously. I have to be more accurate when reviewing others code. Bad code reviewer.

6) Yet another mistake: writing this blog post. I understand that my criticism might be taken to close, especially if this was read. I also don’t like criticism. No one likes. Man, if you reading this you have to know I didn’t mean to abuse you or something, I just was upset at night about failing tests.