.NET and me Coding dreams since 1998!

1Dec/072

Designing for testability – an valid architecture choice? (Part 1)

A back while ago, a whole bunch of people I respect : Ayende, Roy Osherove,Udi Dahan and Eli Lopian, started a debate regarding is designing for testability an overkill or a good practice.
(In case you really care about TDD, I'm sue you would enjoy reading some of this posts:Stop designing for testability, TypeMock is Freedom, Test Driven Design vs. YAGNI, Design and Testability - YAGNI, Design vs. Process, Tools vs. Design, Dependency Injection - Keep your privates to yourself, Testable Designs - Round 2: Tooling, Design Smells and Bad Analogies, The Production Value of Seams, Design & Testability – Sense & Sensibility)

Eli is author of the TypeMock mocking framework  which is based on profiler API and that enables TypeMock to mock anything without the need of designing the code on the way we usually  seen in TDD books and articles (design based on Service locator and dependency injection interface style of programing).
The downside of that "design for testability" is that components are exposing their internals which they wouldn't need to do if there won't be testing (constructor accepting interfaces etc).
Eli's point is that we shouldn't design for testability when we have the tools capable to mock dependencies without any required special design.

To me, Eli's point shakes the fundaments of the TDD philosophy, so I spent last couple of days thinking about it and came up with a conclusion that "design for testability" is still an valid architecture choice for me because:

  • TDD design enforces separation of concerns, which increases maintainability, reusability and coherence.
  • TDD design is loosely coupled design which allows easier development and evolution of the system (change is something certain for every system, application, framework... )
  • TDD design enforce test first which helps better understanding of functional requirements, easier project management by enabling realistic progress data

So there are a lot of upsides but what about downsides Eli points to? This series of blog post would try to present a way how to handle them on easy and efficient way. I would start with a code not designed for testability and redesign it in a series of steps without exposing internals or changing the usage of the methods. I hope by that I would prove that it is possible and easy to designing for testability without negative side effects..

Warning: This would be a couple of long blog post with a lot of code examples explaining ratios behind each one of the different phases in (re)design for testability. I'll try to make them as short as possible, but scrolling posts could look scary :)

Use case

Today's use case would be based on a transaction script type of static DAL class which I'm sure we all seen quite some time

For the sake of simplicity our manager class  would have only one method which would load users from aspnetdb database which name starts with a given string  properties and return the number of that users which were active in last 10 days

image

image

That method would be used from a console application which would print out the number of the active users to end user:

namespace Console
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            int activeUserCount=UserManager.NumberOfUsersActiveInLast10Days("M");
            System.Console.WriteLine(activeUserCount);
        }
    }
}

The task of today exercise is to redesign code so it would be designed for testability without exposing internals or complicating  ways of its usage

Initial state

Let's imagine that UserManager class would initially look like this

using System;
using System.Collections.Generic;
using System.Data.SqlClient;

namespace DAL
{
    public static class UserManager
    {
        private static readonly IDictionary<string, IList<User>> _userCache 
            = new Dictionary<string, IList<User>>();

        public static int NumberOfUsersActiveInLast10Days(string userName)
        {
            if (string.IsNullOrEmpty(userName))
                throw new ArgumentException("User id not sent");

            IList<User> userCollection;
            if (_userCache.ContainsKey(userName))
                userCollection = _userCache[userName];
            else
            {
                using (SqlConnection sqlConnection = new SqlConnection(
                    @"Server=.SQLEXPRESS;Database=aspnetdb;Trusted_Connection=yes;"))
                {
                    sqlConnection.Open();
                    const string query = 
                        @"Select * from User where UserName like '%@UserName%'";
                    using (SqlCommand sqlCommand 
                        = new SqlCommand(query, sqlConnection))
                    {
                        sqlCommand.Parameters.AddWithValue("@UserName", userName);
                        using (SqlDataReader sqlDataReader 
                            = sqlCommand.ExecuteReader())
                        {
                            if (!sqlDataReader.HasRows)
                                return 0;
                            userCollection = new List<User>();
                            while (sqlDataReader.HasRows)
                            {
                                userCollection.Add(
                                    new User(
                                        sqlDataReader["UserId"].ToString(),
                                        sqlDataReader["UserName"].ToString(),
                                        Convert.ToDateTime(
                                            sqlDataReader["LastActivityDate"]))
                                    );
                            }
                        }
                    }
                }
            }

            int result = 0;
            foreach (User user in userCollection)
            {
                if (user.LastActivity > DateTime.Now.AddDays(-10))
                {
                    result++;
                }
            }
            return result;
        }
    }
}

As we can see this class does quite lot things. Key points:

In line 14, method is performing argument checking and throwing exception in case of null or empty user name

In line 18, method is checking if there is cached instance of the already retrieved user collection satisfying given criteria

In lines 22 - 51, method contains standard ADO NET type of code code which reads some data from database,pack that data into the collection of User objects

in lines 54 - 62, method is iterating through collection of user objects and for each user active in last 10 days, increments the result which is returned there (this represents business logic)

What is wrong with this code?

This code has very low coherence and it does a lot of different things so if we would look at this initial solution we would see that it contains three major parts:

  1. Cross cutting concerns: argument validation and cache checking
  2. Database access related code
  3. Method "business logic" - counting active users

Mixing of those 3 things in one method  is:

  • decreasing maintainability
  • preventing potential reuse of database code
    for e.g. there could be a need for the same method without caching for a web page calling that method which would use HttpContext based cache. With this design in place we would need to add a bool cacheOn parameter here (and end with spaghetti code) or we would make another method without caching (and end with redundant pieces of code)

Redesign step 1 - UserProvider

The simplest solution would be to follow separation of concerns principle and create a separate internal provider class which would get all the ADO code while "business logic" would stay in manager level

So the new internal static provider class would look like this:

image 

 

using System;
using System.Collections.Generic;
using System.Data.SqlClient;

namespace DAL
{
    internal static class UserProvider
    {
        public static IList<User> GetUserCollection(string userName)
        {
            IList<User> userCollection = new List<User>();

            using (SqlConnection sqlConnection = new SqlConnection(
                @"Server=.SQLEXPRESS;Database=aspnetdb;Trusted_Connection=yes;"))
            {
                sqlConnection.Open();
                const string query 
                    = @"Select * from User where UserName like '@UserName%'";
                using (SqlCommand sqlCommand 
                    = new SqlCommand(query, sqlConnection))
                {
                    sqlCommand.Parameters
                        .AddWithValue("@UserName", userName);
                    using (SqlDataReader sqlDataReader 
                        = sqlCommand.ExecuteReader())
                    {
                        if (sqlDataReader.HasRows)
                        {
                            while (sqlDataReader.HasRows)
                            {
                                userCollection.Add(
                                    new User(
                                        sqlDataReader["UserId"].ToString(),
                                        sqlDataReader["UserName"].ToString(),
                                        Convert.ToDateTime
                                            (sqlDataReader["LastActivityDate"])
                                        )
                                    );
                            }
                        }
                    }
                }
            }
            return userCollection;
        }
    }
}

As we can see, the provider method returns collection of users which name starts with user name. That method can be called by some other method which could have different approach to cross cutting concerns

The manager class would now look like this

using System;
using System.Collections.Generic;

namespace DAL
{
    public static class UserManager
    {
        private static readonly IDictionary<string, IList<User>> 
            _userCache = new Dictionary<string, IList<User>>();

        public static int NumberOfUsersActiveInLast10Days(string userName)
        {
            if (string.IsNullOrEmpty(userName))
                throw new ArgumentException("User id not sent");

            IList<User> userCollection;
            if (_userCache.ContainsKey(userName))
                userCollection = _userCache[userName];
            else
            {
                userCollection = UserProvider.GetUserCollection(userName);
            }

            int result = 0;
            foreach (User user in userCollection)
            {
                if (user.LastActivity > DateTime.Now.AddDays(-10))
                {
                    result++;
                }
            }
            return result;
        }
    }
}

In line 21, UserManager is calling the UserProvider internal class and geting collection of the users which is then processed on the same like in initial implementation

From usage point of how console application is using the code nothing changed in discoverability of the DAL component because intelisense is not showing the provider class (it is internal class)

What is wrong with this code?

If we look at user manager code now, to test that only active users are counted we would need to insert to database one user with active date earlier then 10 days and one with active date older then 10 days so we could check the results. Problem with that is that we test how business logic works (lines 21-27) ; not how database code in UserProvider class works. That's why we have to further redesign the code so we could remove this obstacle.

(To be continued)

 

Comments (2) Trackbacks (0)
  1. Hi Nikola! Sorry for non-related question, but could you please explain

    1) How did you create those "copy to clipboard" links in your examples? I saw simple JavaScript code using  window.clipboardData.setData() on http://www.superexpert.com/…/default.aspx, but that only work with IE. Your code works in Firefox as well.

    2) How did you implement those &lt;pre class="c-sharp"&gt; blocks with source code? Please!

    You could communicate directly using my address vkelman at gmail dor com, if you’d like so. Thank you!

  2. Greetings,

    even though I risk being pedantic, but isn’t the separation incomplete?

    At least to me, the caching would belong to the provider, and the buisness-logic should not need to care about caching or not caching — all it wants is a list of users. (Moving that into the user-provider would also remove the danger of having data in the user-cache that might modify test results based on the tests run earlier)


Leave a comment

No trackbacks yet.