Design

Why am I angry at developers? Is it factory? Is it testable?

October 17, 2011 Design 4 comments

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.


4 comments


Few notes about Designing System

March 14, 2010 Book Reviews, Design No comments

Designing is “dry”, non-deterministic and heuristic process. 

I just read one chapter from S. McConnell’s “Clean Code” book, and want make some review and summary without taking a look back into book. This should help me reinforce memories of what I’ve learnt.

When you are designing you always should keep in mind that you will not be able to make design good for all situations of life. Thus overthinking on problems could lead to overengineering, so you will get complex system with lot of unneeded tails. Future developers will not be able to work with your design. And project could even fail at some point of time.

To avoid this you should keep building system on easy to understand interfaces, that define how system behaves on different levels and that provide loose coupling.

Building your application like one solid system will lead to situation when you have everything coupled. I’m familiar with system, which is done like single exe file with more than 50 Mb size. You could not even imagine how everything there is coupled – you can get access to any part of the system from any other part. I understand that it is a legacy system and some things where done cause of ‘old school’ and so on and I understand that developers take this into account, but how on earth new guy will know how to work in that system? But how could it be sweet when you have system divided into subsystems, which of them has own responsibilities and depends only on one-two other subsystems, but at the same time provides functionality to good portion of other subsystems.

So, here we talk about low coupling, high cohesion and subsystems ordering. Here is how I do understand this terms:

Low coupling means that parts of the system should interact with each other by clearly defined interfaces, and you should work on decreasing number of interaction. Few characteristics that lead to low coupling are: Volume of connections should be low, Visibility of how two parts connects to each other should be high, Flexibility of changing connection should be also high.

High cohesion means that inside of one part of the system everything should be done compactly, part should have one responsibility well encapsulated inside.

Subsystems ordering means that once you have A and B, and B uses A, then A should not use B. If you need to grow your system you should decrease number of subsystems that you depend on and you should increase number of subsystems that depends on current subsystem. So you are getting tree.

When designing use Design Patterns. Why? Because they are found heuristically and if you think that you have a better decision to the same problem that known design pattern solves it is very possible that your solution will fail.

When designing ask Questions. Why? Designing is iterational  process, you always can improve your solution. Just ask yourself if your current design is enough good and if so how could you prove that. Ask if there are other solutions to problem and why did you select your. Ask a lot. Invite a friend let he ask you.

You can design top-bottom and bottom-top ways. Use both. Why and how? Because they have advantages and disadvantages – you can get stuck with top-bottom if you will not have tech solution to some problem, that was defined at the top or solution is very cumbersome, also you can lost vision of what you do with bottom-top and will lost how to clue everything. We are all now far from waterfall so I think that nowadays it is possible to combine those two ways to design.


No comments