Billy McCafferty : C#

IT Samurai School coming soon...

Billy McCafferty — Mon, 04 May 2009 02:32:00 GMT

As if my life wasn't boring enough trying to get S#arp Architecture 1.0 out the door...

Code Quiz: How well do you understand ThreadStatic?

Billy McCafferty — Fri, 09 Jan 2009 23:51:00 GMT

I just ran into the most interesting bug in recent recollection. What's wrong with the following code?

... public override IEnumerable<PropertyInfo> SignatureProperties { get { IEnumerable<PropertyInfo> properties; if (signaturePropertiesDictionary.TryGetValue(GetType(), out properties)) return properties; ... } } [ThreadStatic] private static readonly Dictionary<Type, IEnumerable<PropertyInfo>> signaturePropertiesDictionary = new Dictionary<Type, IEnumerable<PropertyInfo>>(); ...

And you're not allowed answering if you get S#arp Architecture forum emails...and no peaking if you're not!! ;)

Billy McCafferty

The Mother of Exception Handling Anti-Patterns

Billy McCafferty — Fri, 30 Nov 2007 13:36:00 GMT

I know this has been discussed before elsewhere - but since I'm cranky about finding it after not-so-few-minutes of looking in some legacy code I'm working on - what follows is NOT an example of proper exception handling:

} catch (Exception ex) { AuditLogger.LogError(ex); }

This "swallow and forget" approach causes an immense amount of pain for all parties involved. I've been working on some third party code and was troubled by the fact that the Event Viewer had a number of errors in the log, but that I hadn't encountered any blatant errors while using the application. I only knew that every once and a while, the application didn't respond as I expected it to respond. From a developer's perspective, this is annoying and time consuming to pin down. From a business owner's perspective, this inconsistent behavior leads to lost business or worse.

Swallowing the exception is analogous to the "On Error Resume Next" which had many a developer (aka - me) throwing monitors out windows while debugging Active Server Pages way back when and which continues to blemish the VB.NET language. If there's an exception, it should be assumed that something is terribly wrong; otherwise, it wouldn't be called an exception. When exceptions occur, the application should fail and fail fast; and if that can't happen in your application (aka - a pacemaker), then careful attention should be made to ensure that exception recovery is complete, having put the application back into a state as if the exception had not occurred.

Failing fast doesn't have to mean failing ugly. Let's face it, we write bugs in our code. We shouldn't be embarrassed to politely let the user know that something went wrong (that it wasn't their fault), that we've been informed of the problem, and that we'll get back to them in a timely and courteous manner with a resolution. But what we shouldn't be doing is hiding bugs under the rug and keeping our fingers crossed that the application will somehow recover on its own. If you're lucky, the user won't notice that anything happened and will be able to continue. But in most cases, these swallowed exceptions confuse users with erratic behavior (that's erratic application behavior, not erratic users...although they can be problematic as well) and make it hellishly difficult for developers to figure out why the application is behaving as it is.

At bare minimum, use the following as a starting point for better exception handling:

} catch (Exception ex) { AuditLogger.LogError(ex); throw; }

Note that it does not say "throw ex;" The difference is subtle but important. "throw ex;" re-throws the exception and overwrites the stack trace. "throw;" simply allows the exception to pass through as if it had never been caught at all.

As I assume most of you already know this, please lean over and tap the nearest developer to you who doesn't read anything, and tell them you'll beat them with a dead chicken if you ever catch them swallowing exceptions! (Dead chicken optional for any PETA readers out there.)

Billy McCafferty

Back to Basics: XOR

Billy McCafferty — Sat, 24 Nov 2007 15:47:00 GMT

Conditionals are a common spot for the unintended creation of un-maintainable and un-readable code. To mitigate complicated conditionals, refactoring with Decompose Conditional always helps in conveying what you're trying to do. In other times, it's simply a matter of making the conditional logic itself less complicated. An oft-overlooked operator within C#, or within any language that supports it, is mutual exclusivity, or XOR. This is provided by the ^ character in C#. XOR states that a conditional will return true if one operand is true but not the other. The following demonstrates the differences between AND, OR and XOR.

AND

Operand	Operand	Result
False	False	False
False	True	False
True	False	False
True	True	True

Operand	Operand	Result
False	False	False
False	True	True
True	False	True
True	True	True

XOR

Operand	Operand	Result
False	False	False
False	True	True
True	False	True
True	True	False

Note again that XOR returns false if both are true or both are false, but returns true when only one operand is true.

When used appropriately, XOR can greatly simplify a conditional statement and ease of readability. Consider the following conditional: ((a > 0 && b <= 0) || (b > 0 && a <= 0)). Using the XOR operator, this conditional can be simplified to: (a > 0 ^ b > 0). Clean, simple, and easy to read. A review of other C# operators on the MSDN site may lead to the discovery of other coding simplifications that you were not aware of or had long forgotten...or more concisely: (not aware of ^ long forgotten). Sorry, had to do it.

Billy McCafferty

Using Equals/GetHashCode Effectively

Billy McCafferty — Wed, 25 Apr 2007 21:53:00 GMT

[Updated March 10, 2009: Important!! While the original content shown below is usable in most cases, there are scenarios wherein it breaks down. The results of GetHashCode should not also be used as the means for expressing object equality. Accordingly, this, and other issues have been resolved and may be viewed within S#arp Architecture's BaseObject.cs class found at http://sharp-architecture.googlecode.com/svn/trunk/src/SharpArch/SharpArch.Core/DomainModel/BaseObject.cs.]

[Updated June 23, 2008: A simplified and more systematic approach to this may be found within S#arp Architecture. While the complexities of the Equals/GetHashCode method are kept away in a base class, you simply need to add an [Identity] attribute over each property in a class which is part of the domain object's "signature." It also supports multiple attributes to be used in the same class for multi-property uniqueness - outside of the ID itself.]

The ability to override GetHashCode is available on every object but is seldom required for POCOs. The Equals method usually provides all of the comparison functionality we'll ever need. But when using an ORM such as NHibernate, GetHashCode takes a more prominent role as it helps NHibernate determine if an object already exists in a collection. Not overriding GetHashCode, or doing so inappropriately, may lead to duplicate objects showing up in collections and/or objects missing altogether. When needed, most people implement both methods and end up with similar code in both. So to ease the burden of managing both of these methods, there's exploitable overlap between Equals and GetHashCode to kill two birds with one stone...no offense to any PETA members out there. (A thanks goes out to Alan Northam whose approach made me aware of the code duplication I previously had in my own approach.)

In my project work, I consider the following to be true when comparing two objects:

If two objects have IDs, and have the same IDs, then they may be considered equal without examining them further. (I'm assuming ID to be an identity field, or equivalent, in the DB.)
If two objects have IDs, but have different IDs, than they may be considered not equal without examining them further. E.g., If customer A has an ID of 4 while customer B has an ID of 5, then they are not equal, QED.
If neither object has an ID or only one of them has an ID, but their "business value signatures" are identical, then they're equal. E.g., customer A has an ID of 4 and a social-security-number of 123-45-6789 while customer B has no ID but also has a social-security-number of 123-45-6789. In this case, customer A and customer B are equal. By "business signatures," I imply a combination of those properties which deem an entity unique, regardless of its ID. (As a side, see Eric Evans' Domain Driven Design for a stellar conversation of value and entity objects. A summary version is also available.)
If one of them is null, then they are not equal. (Whoo, that was easy.)

With the above considerations in mind, we'll want to write code to make the following unit test pass. Note that Customer takes a company name into its constructor. It also has a settable contact name. The combination of its company name and contact name give Customer its unique business signature.

[Test] public void CanCompareCustomers() { Customer customerA = new Customer("Acme"); Customer customerB = new Customer("Anvil"); Assert.AreNotEqual(customerA, null); Assert.AreNotEqual(customerA, customerB); customerA.SetIdTo(1); customerB.SetIdTo(1); // Even though the signatures are different, // the persistent IDs were the same. Call me // crazy, but I put that much trust into IDs. Assert.AreEqual(customerA, customerB); Customer customerC = new Customer("Acme"); // Since customerA has an ID but customerC // doesn't, their signatures will be compared Assert.AreEqual(customerA, customerC); customerC.ContactName = "Coyote"; // Signatures are now different Assert.AreNotEqual(customerA, customerC); // customerA.Equals(customerB) because they // have the same ID. // customerA.Equals(customerC) because they // have the same signature. // customerB.DoesNotEquals(customerC) because // we can't compare their IDs, since // customerC is transient, and their // signatures are different. Assert.AreNotEqual(customerB, customerC); }

Although some argue against a single object which all other persistable domain objects inherit from, I use one nonetheless and ingeniously call it "DomainObject." (Those are Dr. Evil quotes there.) "DomainObject," in its entirety, contains the following:

public abstract class DomainObject<IdT> { /// <summary> /// ID may be of type string, int, /// custom type, etc. /// </summary> public IdT ID { get { return id; } } public override sealed bool Equals(object obj) { DomainObject<IdT> compareTo = obj as DomainObject<IdT>; return (compareTo != null) && (HasSameNonDefaultIdAs(compareTo) || // Since the IDs aren't the same, either // of them must be transient to compare // business value signatures (((IsTransient()) || compareTo.IsTransient()) && HasSameBusinessSignatureAs(compareTo))); } /// <summary> /// Transient objects are not associated with an /// item already in storage. For instance, a /// Customer is transient if its ID is 0. /// </summary> public bool IsTransient() { return ID == null || ID.Equals(default(IdT)); } /// <summary> /// Must be implemented to compare two objects /// </summary> public abstract override int GetHashCode(); private bool HasSameBusinessSignatureAs(DomainObject<IdT> compareTo) { return GetHashCode().Equals(compareTo.GetHashCode()); } /// <summary> /// Returns true if self and the provided domain /// object have the same ID values and the IDs /// are not of the default ID value /// </summary> private bool HasSameNonDefaultIdAs(DomainObject<IdT> compareTo) { return (ID != null && ! ID.Equals(default(IdT))) && (compareTo.ID != null && ! compareTo.ID.Equals(default(IdT))) && ID.Equals(compareTo.ID); } /// <summary> /// Set to protected to allow unit tests to set /// this property via reflection and to allow /// domain objects more flexibility in setting /// this for those objects with assigned IDs. /// </summary> protected IdT id = default(IdT); }

Note that Equals is sealed and cannot be overridden by a DomainObject implementation. I suppose it could be unsealed, but since I put a lot of work into that method, I don't want anyone mucking it up!

Now assume that Customer implements DomainObject. As mentioned above, the combination of its company name and contact name give it its unique signature. So its GetHashCode would be as follows:

public override int GetHashCode() { return (GetType().FullName + "|" + CompanyName + "|" + ContactName).GetHashCode(); }

You'll notice that the start of the method includes the full name of the class type itself. With this in place, two different classes would never return the same signature. (You'll have to reconsider how GetHashCode is implemented to handle inheritance structures; e.g. a Customer and an Employee both inherit from a Person class but Customer and Employee may be equal in some instances...for this, I'd probably only add GetHashCode to the Person class.) Additionally, note that GetHashCode should only contain the "business signature" of the object and not include its ID. Including the ID in the signature would make it impossible to find equality between a transient object and a ! transient object. (Equality for all regardless of transience I say!)

Billy McCafferty

Varying a Property's get/set Accessibility

Billy McCafferty — Thu, 15 Feb 2007 23:55:00 GMT

It is a common occurrence wherein you may want to publicly expose a property's getter but internally restrict the setter. (The last refactoring challenge had a good use for this.) I regularly see people provide a publicly exposed property - with only a getter - and then add an internally available method for setting that property. Alternatively, as long as you're only "tightening" access, you can simply pre-append the appropriate access keyword in front of the get/set, accordingly. For example:

In the above code, the getter is publicly available while the setter is only internally available to the assembly in which it's defined. You can also use protected, private and logical combinations, if needed.

Billy

Inject DI Container into Domain Objects with NHibernate

Billy McCafferty — Tue, 06 Feb 2007 00:52:00 GMT

I've been asked a couple of times how to give a domain object access to a dependency injection (DI) container, such as Castle Windsor, if the domain object is loaded via an ORM, such as NHibernate. There are two apparent ways to do this: 1) you can manually give the domain object access to the DI via "setter injection" from your controller or service layer, or 2) you can use an NHibernate IInterceptor to preset the DI container when the object is loaded from the database. The latter is much cleaner and avoids having to pass service object references through middlemen classes.

Disclaimer: I feel I should add a disclaimer to this post that this technique should not be used as standard practice. Giving domain objects direct access to the DI container can be very powerful but also enables the domain layer to take on responsibilities that it should not have. Furthermore, having access to the DI container from the domain layer creates, at the very least, a virtual dependency on everything available via the DI container; consequently, additional maintenance issues may have to be considered. Finally, the DI container itself presents another layer of indirection which can be difficult to easily understand during the maintenance phase of a project. With that said...

Assume you want to show who a customer was referred by but can't set it up in a nice HBM mapping. The only way you can do it, for some reason or another, is by having the Customer instance make an explicit call to the database. Our goals are 1) to keep Customer only dependent on DAO interfaces and 2) to automatically inject the DAO dependency to Customer as soon as its loaded from the database even if it's loaded via an ORM. (Obviously, this example is a little strange to begin with but illustrates the technique, nonetheless. The technique is equally applicable for providing other services, such as an email utility.)

If you'd like to walk through modifying an application, already wired for Castle Windsor, to accommodate doing this, download the "enterprise" sample app found within the MVP with ASP.NET article on CodeProject.com and follow the steps below. (Alternatively, you can just download the sample at the end of this post to see the finished product.) You'll need to follow the steps found within the article to get it working with your SQL Server/Northwind database. The modification steps to the sample app are as follows:

1) First, since we always take a test-driven approach (right?), amend MvpSample.Tests.Data.CustomerDaoTests to reflect the following:

[TestFixture]
[Category("NHibernate Tests")]
public class CustomerDaoTests
{
    /// <summary>
    /// Initializes the NHibernate session bound to 
    /// HttpContext. This TestFixtureSetUp could be 
    /// moved to a NHibernateUnitTest class so you 
    /// don't have to 
    /// copy/past it into every unit test class.
    /// </summary>
    [TestFixtureSetUp]
    public void Setup() {
        NHibernateSessionManager.Instance
            .RegisterInterceptor(
            new NHibernateInterceptor());
 
        NHibernateSessionManager.Instance
            .BeginTransaction();
    }
 
    /// <summary>
    /// Tests successful dependency injection 
    /// from within the domain layer.
    /// </summary>
    [Test]
    public void TestGetReferrer() {
        IDaoFactory daoFactory = 
            new NHibernateDaoFactory();
        ICustomerDao customerDao = 
            daoFactory.GetCustomerDao();
 
        Customer foundCustomer = 
            customerDao.GetById("CHOPS", false);
        Customer referrer = 
            foundCustomer.GetReferrer();
 
        Assert.AreEqual("Folies gourmandes", 
                        referrer.CompanyName);
    }

... remainder of pre-existing tests...

In the above code, you can see that we're doing two new things. First, we've added the call to RegisterInterceptor to the Setup method. (Since you have to register interceptors before starting a transaction, the order is important here.) So, as the code implies, we can wire up container DI via a custom, NHibernate Interceptor. The second thing to note is the new test. We'll be adding the method GetReferrer, which will use the DI container, to the Customer class.

2) If you try to compile the above change, VS will initially complain about the missing NHibernate interceptor class. So add a new class called NHibernateInterceptor and at it to the MvpSample.Data package. Since it's specific to the data layer - and to NHibernate for that matter - the data layer is the appropriate place to put it. The class is defined as follows:

namespace MvpSample.Data
{
    public class NHibernateInterceptor : IInterceptor
    {
        public bool OnLoad(object entity, object id, 
            object[] state, string[] propertyNames, 
            IType[] types)
        {
            // Only use "setter injection" to give objects
            // that implement IUsesDependencyInjection a 
            // reference to the DI container.
            if (typeof(IUsesDependencyInjection)
                .IsInstanceOfType(entity)) {
                ((IUsesDependencyInjection) entity)
                    .DependencyInjectionContainer =
                    new WindsorContainer(new XmlInterpreter());
            }
 
            // Even though we gave the object it's DI 
            // container, return the fact that we 
            // didn't modify the persistent properties
            return false;
        }
 
        public bool OnFlushDirty(object entity, 
            object id, object[] currentState, 
            object[] previousState, string[] propertyNames, 
            IType[] types)
        {
            // Return the fact that we didn't 
            // modify the entity
            return false;
        }
 
        public bool OnSave(object entity, object id, 
            object[] state, string[] propertyNames, 
            IType[] types)
        {
            // Return the fact that we didn't 
            // modify the entity
            return false;
        }
 
        public void OnDelete(object entity, object id, 
            object[] state, string[] propertyNames, 
            IType[] types) {}
 
 
        public void PreFlush(ICollection entities) {}
 
        public void PostFlush(ICollection entities) {}
 
        public object IsUnsaved(object entity) {
            return null;
        }
 
        public int[] FindDirty(object entity, object id, 
            object[] currentState, object[] previousState, 
            string[] propertyNames, IType[] types) {
            return null;
        }
 
        public object Instantiate(Type type, object id) {
            return null;
        }
    }
}

The only bit of interesting code is found within the method OnLoad. This method first checks to see if the persistent object being loaded implements the interface IUsesDependencyInjection and then sets the publicly accessible property to give the object a reference to the Castle Windsor container. This is also called "setter injection."

3) The next logical step is to define the IUsesDependencyInjection interface. This will be implemented by our persistent business objects which should have a reference to the DI container when they're loaded from the database. The interface will be added to the MvpSample.Core project since the business objects will be implementing it. The simple interface is as follows:

namespace MvpSample.Core.DataInterfaces
{
    public interface IUsesDependencyInjection
    {
        IWindsorContainer DependencyInjectionContainer {
            set; 
            get;
        }
    }
}

4) Now modify MvpSample.Core.Domain.Customer to implement this interface. It'll immediatly complain that Custom needs to implement the getter/setter from the interface. You have a choice here; you can either implement the code in the Customer class itself, or, you can add it to the persistent object super class called DomainObject. (I.e. All persistent objects inherit from this class in the sample project.) I chose to put the implementation details into DomainObject for two reasons: A) it's now reusable without any duplicated code and exposes a very handy public setter for all persistent classes and B) you can place pre-condition code in the getter to ensure that the DI container has been set before it's used. The latter point will avoid any unexpected "object reference was null" exceptions. The downside to putting it into DomainObject is that it exposes a lot more power to a lot more classes. So this should be avoided if it's only going to be used by a couple of objects. (Regardless, DomainObject shouldn't inherit from IUsesDependencyInjection, only Customer should, at this time; otherwise, the interceptor will automatically give every persistent object a reference to the container...which might not be a terrible thing in some situations.) So in the DomainObject class, we add:

/// <summary>
/// Publicly accessible to allow the dependency 
/// injection container to be given to the object.  
/// If you have your domain object implement 
/// <see cref="IUsesDependencyInjection" />, 
/// it'll automatically be wired up with the DI 
/// container when loaded with the ORM. If 
/// someone  has switched out the ORM or 
/// disabled this, then the container can still 
/// be set via this property.
/// </summary>
public IWindsorContainer DependencyInjectionContainer {
    set {
        dependencyInjectionContainer = value;
    }
    get {
        if (dependencyInjectionContainer == null) 
            throw new ApplicationException(
                "dependencyInjectionContainer has not " +
                "yet been initialized");
 
        return dependencyInjectionContainer;
    }
}

5) The only remaining piece, to get the unit test working, is to add the GetReferrer method to Customer:

/// <summary>
/// This is an example of using DI from 
/// the domain layer itself.
/// </summary>
public Customer GetReferrer() {
    // Obviously, hard-coding "FOLIG" is 
    // unrealistic...but it'll work to illustrate
    // the use of a DI container from within the 
    // domain layer.
    return ((IDaoFactory) 
            DependencyInjectionContainer[typeof(IDaoFactory)])
        .GetCustomerDao().GetById("FOLIG", false);
}

6) The NUnit test that we wrote previously should now compile and run successfully. (Be sure to copy the web's Config folder into the MvpSample.Tests/bin/Debug so the unit test will find it. A post-compilation script would be good for doing this on future builds.)

7) Now that we have the unit test working successfully, all we have to do is modify the NHibernateSessionModule (an IHttpModule) to register the interceptor when the transaction-per-web-request is started:

/// <summary>
/// Opens a session within a transaction at the 
/// beginning of the HTTP request. This doesn't 
/// actually open a connection to the database 
/// until needed.
/// </summary>
private void BeginTransaction(object sender, EventArgs e) {
    // This needs to be called BEFORE begin 
    // transaction since an interceptor can't
    // be registered after a session has been 
    // created.
    NHibernateSessionManager.Instance
        .RegisterInterceptor(new NHibernateInterceptor());
 
    NHibernateSessionManager.Instance
        .BeginTransaction();
}

Obviously, you don't have to use a transaction for this to work, but it shows how an interceptor works when a transaction is being used.

8) To see it all in action, add a lblReferredBy label to the web project page found at /Views/EditCustomerView.ascx and set its text value to the referrer from within the set property of CustomerToUpdate, found within its code-behind page:

public Customer CustomerToUpdate {
    get { ... }
    set {
        ...
        lblReferredBy.Text = 
            value.GetReferrer().CompanyName;
        ...
    }
}

That's it in a nutshell. The process goes as follows:

Web request gets sent to server.
NHibernateSessionModule creates the interceptor, registers it with the Open-Session-in-View session and begins a transaction.
Page loads a persistent object from the database.
NHibernateInterceptor checks to see if the object being loaded implements IUsesDependencyInjection and sets its DI container, accordingly.
Other code calls the method GetReferrer, found on the Customer object. GetReferrer uses the DI container to load another Customer from the database via a data-access object. But before it's loaded, the getter method of the DI container ensures that the DI container is available for use and throws an exception if not.

This tutorial has shown how to give a domain object a reference to a DI container if that object was loaded from the database using NHibernate. The attached sample is a working version of all the steps mentioned above.

As always, please let me know if you have any comments, questions or suggestions.

Billy

Refactor It! Challenge 3 - The State of Things

Billy McCafferty — Tue, 19 Dec 2006 06:23:00 GMT

If you're not familiar with the Refactor It! challenges, please read this post. (Yes, this one is quite a bit overdue.) Now on to challenge 3...

Welcome back to another refactoring challenge! This challenge will pit your skills against the very useful State Design Pattern along with writing your own unit tests. It should take 1-2 hours for senior developers and a couple more for others. While the previous challenge made you focus only on the refactoring without worrying about writing unit tests, this challenge requires you to complete the refactoring along with writing your own unit tests. Here's the premise...

Background & Problem

This "refactoring to patterns" challenge surfaced during the past couple of weeks on an actual project. The real-world project is a project management tool that interfaces with a schedule management tool. The schedule management tool maintains a listing of project activities which include scheduled start/finish dates, actual start/finish dates (if they're available), and a remaining duration of the activity. (For the purposes of this challenge, many other activity properties have been dropped for simplification.) Since an API was not available to work with the scheduling tool's data, I had to communicate with its database directly. The problem was that quite a lot of business rules were executed when an activity changed from Not Started to Started, for example. ("Not Started" activities have scheduled start/finish dates but no actual dates; "Started" activities have an actual start date, a scheduled finish date but no other dates; and "Finished" activities have actual start/finish dates and no scheduled start/finish dates.) Taking a quick look at the class Challenge3.Domain.ProjectActivity shows how quickly this is getting out of hand.

Your Incentive

Up for grabs this week is the following book along with a CodeIt.Once Refactoring tool 3-User license pack! This seminal writing has become a required part of any serious developer's bookshelf.

Design Patterns: Elements of Reusable Object-Oriented Software
By Erich Gamma, Richard Helm, Ralph Johnson, John M. Vlissides.

Instructions to Enter

Make sure you read all the instructions as they're different from the previous challenge!

Make sure you have NUnit installed; available at http://www.nunit.org/.
Download Challenge3.zip, found below.
Open the solution with VS 2005 and compile it.
Open NUnit, go to File/Open, then open <solution directory>/Challenge3.Tests/bin/Debug/Challenge3.Tests.dll. Click run and make sure all the tests pass. There should be a few unit tests that are ignored; but none should be failing.
Complete the refactorings as described below. Be sure to follow the style of the code that is already in place: classes, methods and public properties in PascalCase, private members in camelCase, etc. Following the style that's already there is always a good rule of thumb when modifying legacy code.
When finished with the refactorings, make sure all the unit tests are still passing and that no test is being ignored, zip up your solution and send to challenge3@emccafferty.com. As you may only win once, only submit a solution if you'd like to be eligible for the prize or let me know that you don't want to be entered for the drawing.
Because of the upcoming holiday schedule, the due date for your submission is Thursday night at midnight, January 4, US mountain time.

Challenge 3 Download Overview

The VS 2005 solution contains two projects:

Challenge3.Core: This class library contains the business logic layer of the application. This class library is further broken down as follows:
- The Domain folder contains all the entities and value objects. It also contains a sub-folder, ActivityState, which will hold the state design-pattern classes.
- The Utils folder contains a design-by-contract utility class written by Kevin McFarlane. The utility class is for all the Check.Requires you'll see throughout the code.
Challenge3.Tests: This class library contains the unit tests of the application. As there is no defined presentation layer, the unit tests serve as a portal to how the application works.

Challenge3.Domain.ProjectActivity: This domain class represents a specific activity within a project's schedule. In the real-world app that this was inspired by, I was communicating with a third party database and had to respect the rules of the data communications being performed by the third party client app. (I was bypassing the client app altogether with my own code but needed to ensure continued data integrity.) For example, if an activity is currently Not Started and becomes Started, then the scheduled start date needs to be set to a null date and the actual start date then needs to be set, accordingly. In addition to this, the remaining duration of the activity needs to be calculated and set. A simple implementation of the state pattern could remedy the conditional logic in the UpdateActivityStartAndFinishWith method. But here's the kicker...you need to take into account not only the object's current state but what state it will be in next. For instance, going from Not Started to Finished is not the same as going from Started to Finished. In the real-world app that this was inspired by, there were unique actions which needed to take place for each of the nine possible transitions; e.g. Not Started to Started, Not Started to Not Started (this isn't necessarily a state change, but the dates can still be modified), Not Started to Finished, Started to Not Started, etc. Your tasks are as follows:
1. Leverage the state design-pattern to simplify the change in states currently being handled by the method UpdateActivityStartAndFinishWith. Any state-related classes should be placed into the folder Challenge3.Domain.ActivityState. There are a few comments in the mentioned method...be sure to pay attention to them!
2. While implementing the state design pattern, existing unit tests should be maintained and new unit tests should be written for any new code. You shouldn't have to remove any unit tests from Challenge3.Tests.Core.Domain.ProjectProjectActivityStateChangeTests; but you may want to add a new suite of unit tests for the state classes which will be created. Currently 97% of Challenge3.Core.Domain is being unit tested. Your submitted solution must have at least 95% of Challenge3.Core.Domain still under unit tests. You can use a tool such as NCover or Clover.NET to verify unit test coverage. (Don't worry about unit testing the Utils namespace; consider this to be a third party DLL.)

Good luck and let me know if you have any questions. And remember, the first submitter with a correct entry gets his/her name thrown into the hat twice!

Happy holidays!

Billy

12/19/06 9:32 AM Mtn Time - Updated project to use US culture dates within unit tests.

Refactor It! Challenge 2 Results

Billy McCafferty — Fri, 01 Dec 2006 01:07:00 GMT

Congratulations to Michaël Willemot who is the winner of Challenge 2! Michaël has won a copy of Applying Domain-Driven Design and Patterns: With Examples in C# and .NET by Jimmy Nilsson along with a 3-user license pack for CodeIt.Once Refactoring. (The winner was randomly selected from all correct entries received.) Geert Baeyaert had the first correct submission and, incidentally, the first correct bonus entry as well.

There were 19 entries altogether with 17 correct entries. 11 entries attempted the bonus but only 5 entries correctly completed the bonus with a mock logger. Here's a run down of the correct entrants: Adam Vandenberg (bonus as well), Andrzej Kuczynski, Avi A, Bill Arnette, Brian Sherwin, Franck Tetard, Geert Baeyaert (first entry and bonus as well), Harry Chou (bonus as well), John Penokie, Jonathan Roe, Joshua McKinney (bonus as well), Matt Payne, Michael Willemot, Navneet Gupta, Rasmus Faber-Espensen, Rune Rystad (last week's winner, so not eligible for prize, but got bonus as well), Vitaly Davidovich. Nice work all!

Interesting Finds

Below are a few items of particular interest that were found in the submissions:

A few entrants included an abstract "join spec" that acted as the parent for the AndSpec, NotSpec and OrSpec. This helped centralize functionality and reduce duplicated code. Furthermore, a couple of entrants also made the spec pattern completely generic so its structure could be easily reused with different domain objects. Great call on both approaches.
I forgot to mention, and I'm glad I did, to remove the two unit tests that were affected when you removed the methods FindByNotTypeAndAtLeastTotalCost and FindAtLeastQuantityOrAtLeastTotalCost. Quite a few people wondered whether they should be left alone, modified, or deleted. I'm glad it brought up this unintended conversation. Unit tests should be seen as a dynamic, living part of the code and are subject to the same level of refactoring as the "real" code. If a unit test is no longer pulling its weight or is no longer applicable, get rid of it. As Brian Sherwin noted, a great book to take a look at is Test-Driven Development by Kent Beck. This book demonstrates just how dynamic unit test development really is. With full-throttle TDD, the unit tests morph as much, if not more, as the production code itself.
The log4net bonus tripped up entrants mainly due to two reasons: 1) two separate logs needed to be generated, as was working in the original code, not just one, and 2) the singleton methods were tricky to mock. One solution to the first trip-point was to pass two loggers, one for the owner's log and one for the auditor's log to the nightly log process. The drawback to this approach is the maintenance headache with having to later on pass a third, forth, or nth logger to the class or to another class altogether. Alternatively, another approach is to supply a simplified Service Locator. This "logger locator" could be easily passed to anything and extended easily. I've attached a zip file, ExampleSolutions.zip, which contains four entries which used this type of approach (and contain a few variants of the spec pattern as well). The second trip-point was dealing with mocking the call to a method on a singleton. You can make a wrapper for the object, or, if you're in a real pickle, you can use a tool such as TypeMock which can mock just about anything, even sealed classes. The drawback to using a tool such as this is that it makes it easier to write more tightly coupled code. As mentioned in a previous post, Michael Feather's Working Effectively with Legacy Code has a number of great ideas for dealing with these types of situations without resorting to such a powerful tool. (You saw what the ring of power did to Frodo, didn't you?)
Someone brought up the question of what to do if you find yourself writing duplicated spec-creation code in different areas of the code. One solution is to create concrete "combination spec" classes for each of these variants. The problem with this is that it leads to the same type of combinatorial class explosion that we were running into before. Alternatively, for the duplicated spec combinations, you could create a CombinationSpecFactory class. This would have one or more creation methods that would return a Spec class; one method for each unique combination which is duplicated in more than one place. You might ask, what's better about having a factory with combination-spec-creation methods vs. having a number of concrete-combination spec classes? My preference of a factory over individual classes would be to keep the Spec class package clean; I want to keep my Spec class names to be as atomic as possible. I'd love to hear other thoughts on this issue!

Instead of describing the smell refactorings in detail, I'll let the code speak for itself. I've attached ExampleSolutions.zip below which contains four projects that have the bonus challenge included along with well-written log4net wrappers. Your comments are most welcome as to pros and cons of each approach.

I hope these refactoring challenges continue to be useful, offering plenty of food for thought. Stay tuned to the next challenge which will be rearing its ugly head sometime next week!

Billy

Refactor It! Challenge 2

Billy McCafferty — Thu, 23 Nov 2006 08:46:00 GMT

If you're not familiar with the Refactor It! challenges, please read this post. (Yes, this one is a little late in being published...but it's worth it!) Now on to challenge 2...

Welcome back to another refactoring challenge! I'm excited about this week's challenge and am sure it'll be interesting and challenging for all skill levels. It should take about 2 hours for senior developers and a couple more for intermediate developers. While last week's challenge served as a good introduction to refactoring in general, this week's challenge focuses on design pattern refactorings. Here's the premise...

Background & Problem

This application continues from the theme started in last week's challenge and includes a logging system for the local tiki bar. Each day, very early in the morning, a logging process kicks off which creates two logs summarizing orders placed on the previous day. An order has a customer, a date, a specific tiki drink, and a quantity of that drink. If the customer wants different types of tiki drinks, they need to place an order for each type. It's not your tiki bar, so that's just the way it is! The first log created each night is for the bar owner and includes all orders which had a total cost of at least $20 or had a quantity of at least 5. The second log is for the auditor and includes all tiki drink orders which had a total cost of at least $10 and were not for the drink called the "London Fog Cutter." (Don't ask me why the auditor doesn't want to know about these drinks...I just work here!)

The problem, in a nutshell, is two-fold. The first problem is that the filtering mechanism for figuring out which orders should be logged is getting quite complicated. The specification design pattern, a specialized version of the interpreter design pattern, needs to be employed to make the filtering easier to maintain. The second problem is that a bi-directional dependency exists between the data layer and the business logic layer. Consequently, it's very difficult to unit test the business logic layer without actually hitting the database. We'll need to introduce a separated interface, also known as the dependency inversion principle, to decouple the business logic layer from the data access layer. We'll then need to use a mock object so that we can predictably test the business logic layer without needing to worry about having a database around. (Tools exist to automate mock object creation, but let's we'll use one developed by hand.)

Your Incentive

Up for grabs this week is the following book along with a CodeIt.Once Refactoring tool 3-User license pack! This is a great book and will be a well-earned prize.

Applying Domain-Driven Design and Patterns: With Examples in C# and .NET
By Jimmy Nilsson.

Instructions to Enter

Make sure you read all the instructions as they're different from last week's challenge!

Make sure you have NUnit installed; available at http://www.nunit.org/.
Download Challenge2.zip found below.
Open the solution with VS 2005 and compile it.
Open NUnit, go to File/Open, then open <solution directory>/Challenge2.Tests/bin/Debug/Challenge2.Tests.dll. Click run and make sure all the tests pass. There should be a few unit tests that are ignored; but none should be failing.
After running the tests, confirm that two log were generated in <solution directory>/Challenge2.Tests/bin/Debug/. The log files are named Auditor.log and Owner.log and contain a number of order summaries pulled from the "database."
Complete the refactorings as described below. Be sure to follow the style of the code that is already in place: classes, methods and public properties in PascalCase, private members in camelCase, etc. Following the style that's already there is always a good rule of thumb when modifying legacy code.
When finished with the refactorings, make sure all the unit tests are still passing and that no test is being ignored, zip up your solution and send to challenge2@emccafferty.com. As you may only win once, only submit a solution if you'd like to be eligible for the prize or let me know that you don't want to be entered for the drawing.
Because of the lateness of this post and this week's holiday, the due date for your submission is Wednesday night at midnight, Nov. 29, US mountain time. It's not an easy one, so I wouldn't wait until Wednesday to get started!

Challenge 2 Download Overview

The VS 2005 solution contains two projects (at the moment):

Challenge2.Core: This class library contains the data access and business logic layers of the application. This class library is further broken down as follows:
- The Data folder contains all the data-access code. The only class within this folder, OrderDao, simulates talking to a real database by generating zero or more sample orders. The unpredictability of the results serves to simulate a real-life database.
- The Domain folder contains all the entities and value objects. It also contains a sub-folder, OrderFinder, albeit still in the same namespace, which holds the order-filtering logic.
- The Services folder contains the nightly logger process.
- The Utils folder contains a design-by-contract utility class written by Kevin McFarlane. The utility class is for all the Check.Requires you'll see throughout the code.
Challenge2.Core: This class library contains the unit tests of the application. As there is no defined presentation layer, the unit tests serve as a portal to how the application works.
Challenge2.Data: This isn't in the download, but you'll be adding this class library soon enough.

Now Refactor It!

Below is a listing of smells that need to be fixed within the project. Each point indicates the file that contains the smell, a description of the problem and any guidelines for completing the refactoring. There may be other smells in the code, but these are the only ones that should be addressed.

Smell: Combinatorial Explosion
Refactoring: Replace Implicit Language with Interpreter

I have yet to write an application that doesn't include a bit of filtering capabilities. Furthermore, it's often the case that the filtering capabilities need to be extended throughout the life of the application. If only a couple of filters are required, then hard-coding the filter logic isn't that big of a deal. But if the number of filters grow or the logic becomes more complicated than just "AND"s, then using the interpreter design pattern may be an appropriate fit. But there's also the question as to whether filtering should be done by the database or by the code. As I prefer to keep database logic to the simplest CRUD (create/read/update/delete) functionality, I avoid putting complicated filtering logic into the database unless performance warrants otherwise. A good rule of thumb is that if you have a couple hundred items or less to filter, and you have more than a few filters that change regularly, then putting the filtering into the business logic layer is a good way to go; otherwise, the database should be doing the basic filtering. Alternatively, ORMs such as NHibernate provide their own implementation of the interpreter design pattern. So onto the smell...

Challenge2.Core.OrderFinder: This order-filtering class exposes the verbosely named methods FindByNotTypeAndAtLeastTotalCost and FindAtLeastQuantityOrAtLeastTotalCost. Besides taking a while to read, these methods are difficult to extend, likely to change and are far from flexible. Without removing these methods (yet), refactor to the specification design pattern to make the filtering capabilities much more flexible. To assist, uncomment the unit tests within Challenge2.Tests.Core.Domain.OrderFinderTests one at a time, following the order suggested. Get each unit test working by implementing the implied code. Put all your "Spec" classes into the folder Challenge2.Core.Domain.OrderFinder but keep the namespace of those classes to be Challenge2.Core.Domain.
Challenge2.Core.Services.NightlyOrderLogger: This class creates writes two logs, one for the tiki bar owner and one for the auditor. To get the appropriate orders to log, it makes a call to the database to get all of yesterday's orders, and then leverages the OrderFinder class to filter, accordingly. Refactor this class to use the specification classes that you just created and delete the methods FindByNotTypeAndAtLeastTotalCost and FindAtLeastQuantityOrAtLeastTotalCost from Challenge2.Core.OrderFinder.

Smell: Dependency on Concrete Service
Refactorings: Extract Interface and Introduce Separated Interface

If you're wondering why you've never heard of the smell listed above, you probably won't be alone. (And if I ever write a book, I'll be sure to change that!) One of the most frequent issues I find with legacy code is direct dependencies on concrete classes that provide a service. A service, in this case, may be a data-access object which communicates with a database, an emailing utility class, an external web service, etc. A service is usually resource intensive, time consuming, or something that shouldn't be executed outside of a production environment. Having dependencies on concrete service classes does not present a problem if the code won't ever be unit tested, but neither will standing in the middle of a highway as long as there's no traffic...but I digress.

A solution for removing a dependency on a concrete service is to introduce, instead, a dependency on an interface. (I'll discuss more of the benefits of this in a moment.) But if the service class is still within the same physical assembly as the business logic layer, then there's nothing to stop a developer from bypassing the interface and going back to the concrete service class directly. To enforce a policy of services via interfaces, a separated interface may be introduced.

Challenge2.Core.Services.NightlyOrderLogger: Notice that this class creates a new instance of OrderDao to retrieve relevant orders from the database. This presents two problems. The first is that the process is time consuming from a unit test point of view. If you have a couple hundred unit tests and many of them require communications with a database, the unit testing process can begin to take 10 minutes or more. Once this happens, developers stop running unit tests altogether. (I experienced this on a previous project.) Furthermore, unit tests expect predictable results to verify code is working properly. Unless a database is reinitialized at the beginning of each test run, the results may vary and the tests will begin to break. This is another motivating factor for developers to stop running unit tests. (This was another unpleasant experience from a previous project.) One action you can take to mitigate this is to label any test that depends on a database with a "Database Test" categorization. With this in place, it's easy to "ignore" these tests within NUnit. But how can we test the business logic if we're ignoring much of it?

You can alleviate these problems by leveraging a mock object that simulates the service in an efficient and predictable manner. Take a look at Challenge2.Tests.MockObjects.Data.MockOrderDao. This class simulates the behavior of the real OrderDao class but returns the exact same results each time. Consequently, it's possible to use this mock object in place of OrderDao to verify that the business logic is working properly. Using mock objects in place of production data-access objects, you won't need to reinitialize the database on each test run. Carry out the following steps to introduce a Separated Interface and inject mock objects into your business logic layer for testing purposes.
1. Use the refactoring technique Extract Interface on the class OrderDao to create an interface called IOrderDao. Put the extracted interface into Challenges2.Core.DataInterfaces and have OrderDao implement it. (The interface should have just one method called GetOrderPlacedOn.)
2. Compile and run the unit tests to make sure everything is still working.
3. A mock object already exists, to simulate this data-access object, found at Challenge2.Tests.MockObjects.Data.MockOrderDao. Have this class also implement the IOrderDao interface.
4. Within Challenge2.Tests.Core.Services.NightlyOrderLoggerTests, there's a method called CanExecuteNightlyOrderLogger. Within this method, create an instance of MockOrderDao and pass it as a parameter to the method ExecuteNightlyLogging. You'll get a compilation error at this point. This is OK as we want the unit test to drive our code changes.
5. Add the following two unit test assertion to the same CanExecuteNightlyOrderLogger method: (You'll still be getting compilation errors from the previous step.)
```
Assert.AreEqual(3, summary.AuditorsLogEntryCount);
Assert.AreEqual(4, summary.OwnersLogEntryCount);
```
6. Modify the ExecuteNightlyLogging method of Challenge2.Core.Services.NightlyOrderLogger to accept an IOrderDao object. Be sure to use Check.Require to make sure it's not null. The name of the parameter should simply be orderDao. This will conflict with the new instantiation of OrderDao a few lines down. That's expected. Delete the line OrderDao orderDao = new OrderDao();. The NightlyOrderLogger is no longer dependent on a concrete service class; e.g. OrderDao. Instead, it only depends on the IOrderDao interface. But developers are still free to use OrderDao directly if they so wish. Now we'll introduce the separated interface.
7. Before proceeding, make sure everything compiles and the unit tests are passing.
8. Add a new project to the solution called Challenge2.Data. This project will contain classes which communicate with the database.
9. Move Challenge2.Core.Data.OrderDao to this new project and delete the Data folder from Chanllenge2.Core. (You'll need to add a reference to Challenge2.core from the Challenge2.Data project. While you're add it, also add a reference to Challenge2.Data from the Challenge2.Tests project.)
10. Finally, repair the namespace reference problems found within Challenge2.Core and Challenge2.Data, compile and confirm that all the unit tests are still passing.
As an optional, bonus step (and to get your name thrown into the hat twice), pass a mock log4net logger to NightlyOorderLogger in addition to the mocked OrderDao so that log files aren't necessarily generated during unit testing.

So what did we just accomplish? There are a few benefits to what we just did: 1) the unit tests are blazing fast as they don't depend on database communications, 2) the data-access and business logic layers are now loosely coupled, 3) the data-access layer can be easily mocked or even, albeit not likely, replaced with a completely different database communications layer, and 4) developers can no longer introduce new, bi-directional dependencies between the data-access and business logic layers. The last point goes a long way to keeping an application clean, testable and maintainable.

This refactoring challenge is quite a few steps beyond the simple, introductory refactorings we saw last week. But I hope I've illustrated that refactoring techniques can not only make a method cleaner, they can also provide direction for refactoring application architecture as a whole.

Good luck on the challenge and let me know if you have any questions!

Billy

Refactor It! Challenge 1 Results

Billy McCafferty — Mon, 20 Nov 2006 23:13:00 GMT

Congratulations to Rune Rystad who is the winner of Challenge 1! Rune has won a copy of Framework Design Guidelines by Krzysztof Cwalina and Brad Abrams along with a 3-user license pack for CodeIt.Once Refactoring. (The winner was randomly selected from all correct entries received.) For the record, Jeff Perrin had the first correct submission...so he got his name thrown into the hat twice and gets a nice pat on the back. ;)

Wow, what a response! There were 21 correct entries out of a total of 25 submissions received before the Sunday night deadline. Due to the number of entrants, if I haven't responded with feedback, you can assume your submission was correct. I was glad to see that people enjoyed working through the challenge and hope you all got something out of it.

Before digging into the results and the solution itself, I wanted to bring up a couple of notes about refactoring and the challenge itself. There's an important aspect of refactoring that the challenge doesn't illustrate very well: a tenant of refactoring is keeping the code in a working state as much as possible and to only have one unit test failing at a time. Consequently, refactoring should be seen as a process, and not just a before and after state. I'll give an example of this when discussing the example solution. On another note, a couple of entrants brought up the point that there were other smells in the code not mentioned in the challenge description...heck yeah there were! If you've worked with legacy code, and we all have, you'll see smells all over the place. At times, the smells are so bad that it's difficult to make any forward progress with refactoring without feeling that you have to gut the whole thing. In times like these, Michael Feather's Working Effectively with Legacy Code can be a great resource. These comments have inspired me to include one such example in the the next challenge.

Interesting Finds

Below are a few items of particular interest that were found in the submissions:

Some created a new class called EmailAddress to handle checking the validity of an email address. Although not required for this particular challenge, doing so consolidates the business logic, makes it more reusable and treats an email address as a first class citizen; and it usually should be.
To fix the "magic number" smell in the Customers class, Adam Vandenberg included a clever usage of iterators to get the list of all customers of drinking age:
/// <summary> /// Enumerate only <see cref="Customer"/>s who are of legal drinking age in the USA. /// </summary> private IEnumerable<Customer> USADrinkingAgeCustomers { get { foreach (Customer customer in this) if (customer.CanDrinkInUSA) yield return customer; } }
Some created a configuration-settings, reader class that centralized the mining of the config file into a single class. This is an example of what Martin Fowler calls a Gateway. I thought it was a great idea and have included it in the example solution. (Another good time to use a gateway is when working with data stored in Session.) The major benefit of using this approach is that if the config (or session) keys change, then you only need to change them in one spot outside of the config file.

Smell Refactorings

Now let's take a look at one possible solution to the refactorings. There are many different approaches to the challenge, and many of them are equally valid.

Smell: Comments

Challenge1.Core.Domain.Customer: There were two comments at the top of this class dictating business rules for "age" and "emailAddress." As mentioned before, some entries included an EmailAddress class to encapsulate the logic for verifying the validity of an email address. This particular refactoring, unsurprisingly enough, is called Move Method. Detailing how this is done, without breaking existing code, you would take the following refactoring steps:
1. Introduce a new method in the Customer class called IsEmailAddressValid. This method would accept an email address and return whether or not it's valid. This refactoring is called Extract Method. This method could then be used to enforce a valid email address in the constructor of Customer. Now we can get rid of the comment concerning passing a valid email. But should the Customer class really know how to check for the validity of an email address? We'll address this concern after we make sure we haven't broken anything.
2. Run the unit tests to make sure everything still works as expected.
3. Create a unit test class, called EmailAddressTests for a yet, non-existent EmailAddress class. (This class is in the solution as Challenge1.Tests.Core.Domain.EmailAddressTests.)
4. Add a unit test called CheckValidEmailAddress which creates an instance of an EmailAddress, passing in a valid email address, as a string, to its constructor. In the unit test, assert that the IsValid method of the object returns true for a valid email address. (You're correct if you're asking yourself, what IsValid method? This is called test driven development...we'll let the unit test tell us what code to write.
5. Copy the code in Customer.IsEmailAddressValid to a new method within EmailAddress called IsValid. Don't remove the duplicate code from Customer yet...we want to fix the currently breaking unit test before moving on to anything else. (Incidentally, some entries checked for the email address, validity check in the constructor of EmailAddress while others left it to other classes to ask EmailAddress if it was valid. Either way may be correct depending on the business requirements and both approaches were considered correct for this challenge.)
6. Compile and verify that the CheckValidEmailAddress unit test is passing.
7. Add another unit test called CheckInvalidEmailAddress, passing in an invalid email address this time, and assert that IsValid return false.
8. Compile and verify that the CheckInvalidEmailAddress unit test is passing.
9. Now, have Customer talk to the EmailAddress object directly to determine email address validity instead of the private method IsEmailAddressValid.
10. Compile and run the unit tests again to make sure everything still works.
11. Delete the IsEmailAddressValid method from the Customer class and run the tests again.
The preceding steps included the refactorings Introduce Assertion, Extract Method, Move Method, and Decompose Conditional. No wonder it took so many steps! Although the details of additional refactoring will not be broken down to such detail, I wanted to show how a refactoring is carried out at a lower level. (And if I haven't promoted it enough already, I highly recommend reading Martin Fowler's Refactoring to see each of these refactorings examined in more depth.) The above seems quite tedious, but the entire cycle takes just a couple of minutes and gives you confidently, working code (aka - peace of mind).

If you've looked at the solution code, you may have noticed calls to Check.Require to implement the Introduce Assertion refactoring. This code calls a wonderful design-by-contract class originally written by Kevin McFarlane. You can read more about it, and design-by-contract in general, in a previous post. Although it seems overkill and unnecessary, sprinkling Check.Require all over your code will save you hours upon hours in the debugger. (Just give it a whirl for a couple of days and you'll be convinced.)
Challenge1.Core.Services.EmailService: If I write anything about the comment-smell in this class, I'd create a "duplicated blog content" smell. I.e. see the details of the first point for all the nitty-gritty details.

Smell: Magic Numbers

Challenge1.Core.Services.EmailService: There was a hard-coded SMTP server name and email address in the code. The correct solution was to put these values in App.config so that they could easily be changed between development and production environments. Taking it a step further, the example solution includes a new class, Challenge1.Core.ConfigSettings, which provides centralized access to the config settings. A couple of entries suggested this approach.
Challenge1.Core.Domain.Customers: This class also had a hard-coded email address; the previous point's fix also applies to this one. Additionally, this class had a hard-coded value of "21" to represent the legal drinking age in the United States. The refactoring Replace Magic Number with Symbolic Constant has been used in the example solution to give the number more meaning. I chose to put this magic number into a constant instead of the config file because it is highly unlikely, in spite of how much we tried before we were 21, that this age will change. But the constant's name serves to make the code more readable. Finally, this class had a couple of strings, representing the spam's subject and body, which never changed. Although not implicated before, I've also placed these strings into constants as well. In a real-world situation, these probably would be pulled from a database or flat-file...neither of which I want to get into now! If requirements dictated that either of these be changed in the future, that would be the time to refactor the code to a more maintainable solution.

Smell: Long Method and Complicated Conditionals

Challenge1.Core.Domain.Customers: The method SendBeerSpamToCustomersOfDrinkingAge has been sliced and diced, using the refactorings Extract Method, Decompose Conditional, and Replace Magic Number with Symbolic Constant to make it, arguably, easier to understand. The method name ConditionallyTrackAndSendSpamTo may seem long, but it fully describes that the method sends spam, conditionally, and tracks any emails that are sent. You should never shorten a method name with acronyms or leave out a responsibility from the name. If you follow this rule of thumb, then long method names may be a sign that a method is trying to do too much. Perhaps we should Extract Method again to enable us to make the method name shorter and more concise?

In carrying out the refactoring Decompose Conditional, for this class, I introduced the method IsHomebrewOrderPurchasedInPastSixMonths to the order class. Now there's a method name! This is an example of a decomposed conditional taken to the extreme. It perfectly summarizes the conditional in a readable fashion; but it's easy to see that this could get out of hand if we need to add other methods such as IsMoonshineOrderPurchasedInPastEightMonths. We could parameterize both the type and the timeframe, but what if we add another variable to the mix...er...mess? If that never happens, then this method may be adequate, but if that very probable situation does occur, it may be time to refactor to the interpreter design pattern. But again, if there's never a need to expand the filtering capabilities for orders, then we needn't invest the time, nor added complexity, of introducing this design pattern. (Down pattern-itus, down! Good dog.)

As I've mentioned, there are many, many ways to refactor this solution and what I've described is just one of them...with that, I open the floodgates to constructive criticism! ;) To make a preemptive strike, there could be a few more unit tests and the Customers class is taking on a few too many responsibilities; but since I've spent way too much time on this post already, I'll leave the remaining smells as food for thought!

Stay tuned for tomorrow's challenge!

Billy

Refactor It! Challenge 1

Billy McCafferty — Tue, 14 Nov 2006 21:24:00 GMT

For an overview of this contest, read this opening introduction. Now, onto this week's refactoring challenge...

As Fowler's subtitle describes, refactoring is about "improving the design of existing code"; an introduction to the subject can be found at refactoring.com. This first challenge includes a number of very common smells and straight-forward refactorings to remove those smells. For those of you who are new to refactoring, this particular challenge provides the perfect starting place. For those of who are refactoring guru's, I've got your can of worms coming soon!

Instructions to Enter

Make sure you have NUnit installed; available at http://www.nunit.org.
Download Challenge1.zip found below.
Open the solution with VS 2005 and compile it.
Open NUnit, go to File/Open, then open <solution directory>/Challenge1.Tests/bin/Debug/Challenge1.Tests.dll. Click run and make sure all the tests pass.
Complete the refactorings as described below. Be sure to follow the style of the code that is already in place: classes, methods and public properties in PascalCase, private members in camelCase, etc. Following the style that's already there is always a good rule of thumb when modifying legacy code.
When finished, make sure all the unit tests are still passing after you've completed the refactorings, zip up your solution and send to challenge1@emccafferty.com. As you may only win once, only submit a solution if you'd like to be eligible for the prize.

Up for grabs this week is the following book along with a CodeIt.Once Refactoring tool 3-User license pack!

Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries
By Krzysztof Cwalina, Brad Abrams.

Challenge1.Core.Domain.Customer: There are two comments concerning the allowable parameters. These comments don't seem to be enforcing much. Enforce these comments and get rid of them to make the code speak for itself. Don't worry about checking to see if the email address is valid. For a hint in the right direction, take a look at the constructor of Challenge1.Core.Domain.Order. (Guideline: Include the refactorings "Extract Method" and "Introduce Assertion.")
Challenge1.Core.Services.EmailService: There's a comment on line 10. How can we get rid of this comment and make the "if" statement which follows a little more understandable? (Guideline: Include the refactorings "Extract Method," "Decompose Conditional" and "Introduce Assertion.")

Smell: Magic Numbers

A magic number is a constant which appears in the code itself. There are a few examples of magic numbers in the code that need to be fixed:

Challenge1.Core.Services.EmailService: There's a hard-coded email address and a hard-coded SMTP server within the method SendEmailTo. (Although not a number, this would still be classified as the Magic Number smell.) If the email address changes or the SMTP server changes, you'll have to make a modification and recompile the entire project. How should this be fixed to make it easier to change these items and to deploy it to various servers?
Challenge1.Core.Domain.Customers: The same type of problem is found at the bottom of the method SendBeerSpamToCustomersOfDrinkingAge. Doh...who programmed this piece of ...!
Challenge1.Core.Domain.Customers: This class contains the magic number "21." How should it be fixed? Is it appropriate to fix it in the same way as the EmailService fix, or do we really need to worry about changes to this number very often? Regardless, this is a magic number which isn't very explicit.

Smell: Long Method and Complicated Conditionals

A long method is any method that has more than one responsibility or is too long to understand easily. As a rule of thumb, I try to make sure no method has more than about ten functional lines of code. In practice, I usually find that most of my methods are between three and ten lines long. (As a side note... Taking this to the extreme, is it incorrect to have a method with just one line of code? Not if it helps make your code more explicit and understandable.) In addition to improving readability, short methods make it much easier to spot duplicated code. For the challenge at hand, fix the following smell:

Challenge1.Core.Domain.Customers: The method SendBeerSpamToAppropriateCustomers is an example of a long method. It has two obvious responsibilities, but it also has a couple of other, subtle responsibilities buried into the conditionals. The comment above the conditional could also be dropped if the conditional were made more explicit. Use the refactorings "Extract Method" and "Decompose Conditional" as many times as appropriate to improve readability and consolidate responsibilities. You needn't introduce any new classes to refactor this method.

To find help or explanations for a particular refactoring, "Extract Method" for example, try googling "extract method refactoring." Good luck and let me know if you have any questions!

Billy

Refactor It! The Weekly Book Giveaway

Billy McCafferty — Tue, 14 Nov 2006 04:55:00 GMT

Each week, refactor my smelly project and win a free book
along with a CodeIt.Once Refactoring tool 3-User license pack!

Here's how it'll work...

Each week, beginning tomorrow, Nov. 13, a VS 2005 project will be provided which contains one or more specified "smells." The project may be a Windows App, a Console App, an ASP.NET Web App or even a simple Class Library; but it will always be in C# 2.0. Refactorings may range from simply introducing new methods to applying design patterns. The project will have one or more unit tests, verified with NUnit 2.0, to verify that the project is in working order.
You have until the specified time to submit your zipped, refactored solution. Instructions for submitting your solution will be presented when the contest begins. To be considered, the solution must be compilable with all unit tests passing.
The winner will be randomly selected from all "correct" submissions received. (I'll use my 2-year old to pick the name instead of Math.Random to make sure it's truly random!) As there are 1,000 ways to skin a cat - no offense to any PETA readers - "correct" entries will be subjectively determined by yours truly, Billy McCafferty. Along with an announcement of the week's winner, a results summary and refactoring analysis will be presented. "Correct" can generally be defined as the solution which presents simple, concise, and maintainable refactorings for the specified smells. Other "correctness" guidelines may be provided at the beginning of each challenge.
To help your chances, the first "correct" solution received will have two names thrown into the hat instead of one.
Only one entry may be submitted for each challenge and, unless specified at the opening of a contest, each participant may only win once.
The weekly contest will continue until I get refactoring-itus or I run out of books, whichever comes sooner.

Why refactoring...

Refactoring has been widely adopted as a vital technique for producing high quality software. It is important that we each embrace this practice into our development work. This contest will serve to present an overview of common smells found within code and discuss techniques to correct those smells for creating better software.

Resources to help...

The essentials are Refactoring by Martin Fowler and Refactoring to Patterns by Joshua Kerievsky. Other resources that may be helpful include Working Effectively with Legacy Code by Michael Feathers and Patterns of Enterprise Application Architecture by Martin Fowler.

Books up for grabs...

Design Patterns: Elements of Reusable Object-Oriented Software
By Erich Gamma, Richard Helm, Ralph Johnson, John M. Vlissides.

Essential Windows Workflow Foundation
By Dharma Shukla, Bob Schmidt.

Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries
By Krzysztof Cwalina, Brad Abrams.

Applying Domain-Driven Design and Patterns: With Examples in C# and .NET
By Jimmy Nilsson.

Essential ASP.NET With Examples in C#
By Fritz Onion.

Essential ASP.NET 2.0
By Fritz Onion, Keith Brown.

Stay tuned to devlicio.us and good luck!

Billy

Show User-Friendly enum Translations

Billy McCafferty — Mon, 16 Oct 2006 20:39:00 GMT

It's common to populate a drop-down box with enum values or, similarly, to display an enum setting to the user. The problem is that enum values usually aren't very pretty to look at. (Memories of hoping QA wouldn't spot "OrderShipped" come to mind.) In a previous post by Joe Niland, Joe suggested that attributes be used to maintain the user-friendly version of enum values. What follows is a solution for doing just that.

What we need is a class that will satisfy the following unit-test:

[TestFixture] public class EnumDescriptionTests { [Test] public void CanRetrieveEnumTextAndDescription() { // Simply uses the enum value itself as the text representation by default Assert.That( EnumDescription.TextRepresentation.GetTextRepresentationOf(OrderStatus.Delivered), Is.EqualTo("Delivered")); Assert.That( EnumDescription.TextRepresentation.GetDescriptionOf(OrderStatus.Delivered), Is.EqualTo(String.Empty)); Assert.That( EnumDescription.TextRepresentation.GetTextRepresentationOf(OrderStatus.InventoryDepleted), Is.EqualTo("Out of Stock")); Assert.That( EnumDescription.TextRepresentation.GetDescriptionOf(OrderStatus.InventoryDepleted), Is.EqualTo(String.Empty)); Assert.That( EnumDescription.TextRepresentation.GetTextRepresentationOf(OrderStatus.InventoryComing), Is.EqualTo("More in Stock Soon")); Assert.That( EnumDescription.TextRepresentation.GetDescriptionOf(OrderStatus.InventoryComing), Is.EqualTo("An order for more items has been placed with the supplier")); } private enum OrderStatus { Delivered, [EnumDescription.TextRepresentation("Out of Stock")] InventoryDepleted, [EnumDescription.TextRepresentation("More in Stock Soon", "An order for more items has been placed with the supplier")] InventoryComing } }

When a description is provided for an enum value, via the "Description" attribute, then the provided description is returned. When a description isn't available, the ToString version of the enum value, with spaces put before each capital letter, is returned, instead.

So then to satisfy the unit test:

public class EnumDescription { public class TextRepresentationAttribute : Attribute { public readonly string Text; public readonly string Description = ""; public TextRepresentationAttribute(string text) { Text = text; Description = ""; } public TextRepresentationAttribute(string text, string description) { Text = text; Description = description; } } public class TextRepresentation { public static string GetTextRepresentationOf(Enum enumType) { TextRepresentationAttribute textRepresentation = GetTextRepresentationAttributeFor(enumType); return textRepresentation != null ? textRepresentation.Text : enumType.ToString(); } public static string GetDescriptionOf(Enum enumType) { TextRepresentationAttribute textRepresentation = GetTextRepresentationAttributeFor(enumType); return textRepresentation != null ? textRepresentation.Description : String.Empty; } private static TextRepresentationAttribute GetTextRepresentationAttributeFor(Enum enumType) { MemberInfo[] memberInfo = enumType.GetType().GetMember(enumType.ToString()); if (memberInfo != null && memberInfo.Length == 1) { object[] customAttributes = memberInfo[0].GetCustomAttributes(typeof(TextRepresentationAttribute), false); if (customAttributes.Length == 1) { return (TextRepresentationAttribute) customAttributes[0]; } } return null; } } }

This should make a useful addition to any utility library.

Billy

Examine XML of Web Service Response

Billy McCafferty — Mon, 09 Oct 2006 22:29:00 GMT

While working on winning Netflix' million, I came across a need to examine the raw XML returned by a third party web service. After a couple desk-pounding hours, I finally found a simple solution which works equally well in both Web and Windows apps.

Install Web Services Enhancements (WSE) 3.0
Add a project reference to Microsoft.Web.Services3
Modify the web service proxy to inherit from Microsoft.Web.Services3.WebServicesClientProtocol instead of System.Web.Services.Protocols.SoapHttpClientProtocol. (This augments the proxy's capabilities.)
The response's raw XML may then be retrieved immediately after the call to Invoke as ResponseSoapContext.Envelope.InnerXml

Optionally, log4net may be employed so as not to interrupt the application while it goes about its business...

Add a project reference to log4net
Add log4net configuration to app.config or web.config. For example:



<log4net threshold="ON">

    <appender name="LogAllToFile"
        type="log4net.Appender.FileAppender">

        <file value="MyLog.log"/>

        <appendToFile value="false"/>

        <layout type="log4net.Layout.PatternLayout">

            <conversionPattern
                value="%d [%t] %-5l - %m%n%n"/>

        </layout>

    </appender>

    <root>

        

        <priority value="ALL"/>

        <appender-ref ref="LogAllToFile"/>

    </root>

</log4net>
Initialize log4net when the application starts with a call to XmlConfigurator.Configure();
Add the following code immediately after the call to Invoke in the proxy web service:

log4net.ILog logger = log4net.LogManager.GetLogger(typeof(MyProxyClassName));

logger.Info(ResponseSoapContext.Envelope.InnerXml);

With log4net turned on, the raw XML within the web service response will get written out to the log file and the application will not be interrupted.

Billy