Earlier this week, I was doing a training session with my current team, building a sample Org Chart application. One of the user stories we were working with, talked about assigning managers to department and had the following acceptance criteria:

• When assigning a manager to a department, ensure that they are an employee of that department, and remove them from their old department

The basic code that we came up with worked well and was encapsulated into a service object in the domain.

public class AssignmentService

{

    private IDepartmentRepository _repository;

    public AssignmentService(IDepartmentRepository repository)

    {

        _repository = repository;

    }

    public void AssignManagerToDepartment(Employee manager, Department assignedDepartment)

    {

        Department previouslyAssignedDepartment = _repository.GetDepartmentForEmployee(employee);

        if (previouslyAssignedDepartment != assignedDepartment)

        {

            previouslyAssignedDepartment.RemoveEmployee(manager);

            _repository.Save(previouslyAssignedDepartment);

        }

        assignedDepartment.AssignEmployee(manager);

        assignedDepartment.AssignManager(manager);

        _repository.Save(assignedDepartment);

    }

}

Problems

At first glance, this code seems simple enough, but we quickly ran into a problem when we started coding for another story and acceptance criteria.

• When creating a department, allow a manager to be assigned

Off-hand, this seems like a very simple situation. We'll just call the AssignmentService object after we create the Department. However, there's a large potential problem with this - the AssignmentService handles the entity persistence for both of the departments that were changed, when the manager is assigned. With that in mind, the developer working on the new requirement would have to ensure that the department is created with all of it's required fields and information before calling this service object. If the developer allows the manager to be assigned before the department's info has been completely filled in, we may accidentally allow an invalid state for a department object when calling the AssignmentService. Since the AssignmentService tries to save the department being assigned, we may also have created an semantic coupling between the department creation process and the AssignmentService - the developer needs to know that the AssignmentService will save the department. If they don't know this, they may try to save the same entities more than once, may try to save an invalid state, etc, etc, etc.

Unfortunately, we didn't have time to fix the problem during the training session - we had to stop the training almost immediately after we discovered this issue. I've got a few ideas running around in my head, on how to fix this, but I'm not 100% sure that I like any of them.

Possible Solutions

First and foremost, though, it's becoming rather apparent to me that we want to avoid entity persistence calls from within the domain service objects. This essentially means that the AssignmentService object would have to return the assigned department, and the 'previously assigned department' objects, so that the coordinating presenter could save them both. That seems a little odd, if you ask me.

One of my coworkers suggested that we allow the service to save the previously assigned department, since that is an self-contained operation in the method (the method loads the department, modifies it, and saves it) and then return the assigned department so that the presenter can save it. This option seems a bit better off-hand, but I'm still worried about the possible side effects of the AssignmentService saving the previously assigned department.

Questions

What are your thoughts on this situation... How would you solve the dilemmas that I'm pointing out? Is there a better design for the model that would eliminate this problem and make the department persistence more obvious, without having to 'know' when it is persisted?

Is the idea of not allowing entity persistence from a domain service object valid? Should we go with allowing the self-contained operations to persist the entity in that operation?

Or ???

During a TDD/BDD training session with my team, today, we wrote the following code:

public void SaveRequested()

{

    Employee employee = Department.CreateEmployee(firstName, lastName, email);

    if (employee == null)

        repository.Save(Department);

}

I ignored this when we first wrote it, as it was "technically correct", but later returned to this code as a point of refactoring for greater insight and expressiveness. When we came back to it, though, I started off by asking a series of questions:

• Why would that method return a null?
• Why would it not return a null?
• Why is it important to not call repository.Save if the employee is null?

The code is essentially a mystery to anyone that hasn't memorized the details of the Department.CreateEmployee method. Worse yet, I can make a lot of assumptions about this code - but every one of them is going to be wrong until I can prove that they are right by reading the code in Department.CreateEmployee.

So how do we refactor this code for better insight and more expressiveness? How do we change this code so that the assumptions a reader creates in their own mind are more likely to be correct - or better yet, are irrelevant because you don't need to even care about the details of the Department.CreateEmployee method? First and foremost, you need to understand what this code is supposed to be expressing - what it's supposed to be doing and why. If you don't know, you need to ask the person that wrote it. If they don't know (or aren't around), you need to find someone that can help you determine the intended behavior. Ultimately, you may need to dig into the Department.CreateEmployee method to figure it out for yourself.

In this situation, we were trying to express some business rules from a set of acceptance criteria on a user story:

• When creating an employee, the employee must belong to a department
• When creating an employee, require a first name, last name, and email address

This code is a factory method on the Department object that is used to ensure these rules are satisfied. In this case, if CreateEmployee returns a null, one of these rules was violated and we should not attempt to save the Department (with the associated employee, or lack there-of). If CreateEmployee returned an employee, we satisfied the rules and we can save the Department and it's associated employees.

The code that we have in place doesn't express any of that intent, though. What we really want is code that tells us that the employee creation failed - and probably why it failed. Fortunately, the changes can be extraordinarily simple.

public void SaveRequested()

{

    EmploymentResult employmentResult = Department.Employ(firstName, lastName, email);

    if (employmentResult.Success)

    {

        repository.Save(Department);

    }

    else

    {

        view.DisplayError(employmentResult.FailureReason);

    }

}

UPDATE: Thanks to Martin Linkhorst for the comments, and making this code even more expressive (changing from Department.CreateEmployee to Department.Employ)

By creating an AddEmployeeResult object and supplying a few additional bits of information in it, our code became much more expressive of it's intent. We can now read the SaveRequested method and have a much better chance of knowing what it is really doing - try to create an employee and if it successfully created, then save it. Otherwise, display the failure reason.

What assumptions does your code create in the minds of the readers? Are these assumptions correct, incorrect, or unintelligible? And finally, what can you do to make your code assumption-free and/or assumption-irrelevant?

Over the weekend, I spent some time refactoring some business rules in my current project, to use the ISpecification<t> framework that I previously created.

One of my coworkers had asked about logical groupings - making sure that we can handle complex logic such as, "(this and this) or this". Since my project actually made use of logical groupings I wanted to share with the world and show how easy it is.

In this example, I have a Date Range that is being selected – a From date and a To date. The rules are as follows for validating the range:

• If a From date and a To date are provided, the From date must be equal to, or before, the To date
• Or, you can specify a From date without To date
• Or, you can specify a To date without From date

The following is the actual code that I’m using to define these business rules, via the ISpecification<t> framework. 

PredicateSpec<ExtractSearchCriteria> fromDateProvided = new PredicateSpec<ExtractSearchCriteria>(crit => crit.FromDate > DateTime.MinValue);

PredicateSpec<ExtractSearchCriteria> toDateProvided = new PredicateSpec<ExtractSearchCriteria>(crit => crit.ToDate > DateTime.MinValue);

PredicateSpec<ExtractSearchCriteria> fromDateBeforeToDate = new PredicateSpec<ExtractSearchCriteria>(crit => crit.FromDate <= crit.ToDate);  

ISpecification<ExtractSearchCriteria> validDateRangeSpec =

    (

        (fromDateProvided.And(toDateProvided))

        .And(fromDateBeforeToDate)

    )

    .Or(

        fromDateProvided.Not(toDateProvided)

    )

    .Or(

        toDateProvided.Not(fromDateProvided)

    );

Here, I am defining the core of the business rules – to check if the From date was provided, to check if the To date was provided, and to check if the From date is before or equal to the To date. From these core specifications, I can build the aggregate spec that handles all of the stated rules for validating the date range.

There are three logical groupings using parenthesis to represent each of the business rules that can comprise a valid date range. The first logical grouping checks to see if both a From and To date are provided, and checks if the From date is before or equal to the To date, if both are provided.

(

    (fromDateProvided.And(toDateProvided))

    .And(fromDateBeforeToDate)

) 

The second group is specified with an Or, and checks to see if we provided a From date but not a To date.

.Or(

    fromDateProvided.Not(toDateProvided)

)

And the third group also uses an Or, and checks to see if we provided a To date but not a From date.

.Or(

    toDateProvided.Not(fromDateProvided)

)

The end result is that we have one of three possible ways for this date range to be valid – all logically grouped and relatively easy to read (compared to a bunch of if-then statements, at least). Even better – we re-used two out of three simple ISpecification<t> objects to create all of the rules for this validation check. This helps to create a validation that is not only easier to read, but easier to maintain and enhance in the future.

Myself and 10 other developers in my company went through a day of BDD / TDD training, with Scott Bellware, yesterday. It was a lot of fun, very challenging at times, and covered a lot of topics including an overview of Agile and Behavior Driven Development, all the way down to writing Specification Tests, doing Test Driven Development  and refactoring the model to improve readability, maintainability, flexibility, etc. I took notes via index cards (love that cool-aid) and wanted to share. I don't expect these notes to make sense to everyone. Hopefully it will spark some dialog in someone's mind and cause them to dig further.

First off - the quote of the day.

"Can I be honest with you and say that I've been wanting to touch your keyboard, all day?"

Now for my notes.

User Stories

[Role], [Goal], [Motiviation]

• As a [role], I want to [goal], so that [motivation]
• Example: As a nurse, I want to record a patients vital signs, so that I can determine their medication and care needs
• Motivation is critical - it determines how the development team understands and implements the story. It determines the user experience, how things are integrated, how the software is designed, etc.

Acceptance Criteria

• Acceptance Criteria is used to drive code, not the story, directly
• may change at any point, up to implementation
• is used to drive code design, test design, implementations, etc.
• should be spoken in domain language
• may include non-functional, technical details such as database tables, infrastructure, performance, etc
• All acceptance criteria must be met and tested / verified before a story is considered done

Specification Tests

• Test Fixture per Class is an anti-pattern (on a personal note, this problem bothered me for months before I discovered BDD)
• Context Specification or Behavior Specification testing
• When [verb] then [verb]
  • "When [verb]" is the context
  • "Then [verb]" is an observation of the behavior
• Based on Acceptance Criteria, but not "code-gen'd" from acceptance criteria

Story Estimation

• Agile Poker: uses generalized Fibonacci sequence as order of complexity
  • "?", 0, 1/2, 1, 2, 3, 5, 8, 13, 20, 40, 100, infinite
• everyone throws their estimate at same time
• if estimates have significant outliers, discussion occurs to understand why, get more detail, etc. and re-throw may happen

Entity Data vs. Aggregate Data

• Entities should never contain aggregate data
• Aggregate data is for reporting and other aggregate needs
• If you need aggregate data to process something, write an SQL query, stored proc, etc. - don't use an ORM like NHibernate
• We don't want a "Customer" entity to need 10,000 "Order" entities, to aggregate data for processing; write a query to aggregate instead
• We don't want to persist data that can be calculated / aggregated, generally (performance issues may override this)

Domain Services

• Can have dependencies on external systems
• are part of domain logic, therefore are in domain model / assembly
• are "Doers" of process that don't fit into entity and entity logic, directly
• coordination of entity logic
• can include calls to data access, logging, etc.

Continuous Integration

• Not just continuous compilation of code
• Full end to end integration of all code, components, databases, services, etc
• Full suite of integration testing including database testing
• Do not allow commits if build is currently broken
• do not allow defects to live - fix immediately, to fix build
• "Defect" is broken software, "Bug" is functional but wrong

Daily Scrum

• 3 Questions everyone answers:
  • What did I do yesterday?
  • What am I doing today?
  • What issues am I having?
• Each person should answer quickly - 1 or 2 minutes, max
• further discussion happens outside of the Scrum meeting

Productivity of Dev Team

• RAD and other non-review, non-iterative based management causes problems and loss of productivity
• we need constant review of the design to ensure good design
• shorten the feedback loop and get constant review of the design, to always improve the design, via pair programming, work cells, retrospectives, etc.
• good design will cause productivity gains in the development team beyond the capabilities of any tools

Whiteboard Diagramming vs. Details Specs

• White board diagramming and human interaction is always better than detailed documents and specs in UML
• Human interaction leads to knowledge crunching and learning, not just reading a repeating
• Take pictures, don't re-draw in UML; don't waste time with it
• Video the entire conversation is even better, so others can learn from the knowledge crunching that occurs; capture the human interaction, body language, etc.

For the last few months, I've had a very small base class that abstracts out the NHibernate configuration, session creation, etc. It works very well, but is very limited in what it can do. basically, every method in my actual repository implementation would have to open a new session, execute a criteria and close the session. A typical implementation would look like this:

public ICollection<Invoice> GetAll()

{

    ICollection<Invoice> invoices = null;

    try

    {

        OpenSession();

        invoices = Session.CreateCriteria(typeof(Invoice)).List<Invoice>(); 

    }

    finally

    {

        CloseSession();

    }

    return invoices;

}

That certainly is easy and keeps the code fairly clean, removes a lot of duplication, etc.

However, there is a significant limitation - I can't have any code re-use for multi-query scenarios, without duplicating code. In other words, if I want to load that list of invoices and then load some other collection from another repository, I have to use two different Sessions across two different repository implementations. This really becomes an issue when dealing with transactions - I want my entire change set to pass or fail in a single transaction. In my current abstraction, this can't be done.

Fortunately, NHibernate has the solution to my dilemma built right in - all I need to do is create my criteria objects without a session, and then I can execute any / all of them from any session that I want.

public ICollection<Invoice> GetAll()

{

    ICollection<Invoice> invoices = null;

    try

    {

        DetachedCriteria criteria = DetachedCriteria.For<Invoice>();

        OpenSession();

        invoices = criteria.GetExecutableCriteria(Session).List<Invoice>(); 

    }

    finally

    {

        CloseSession();

    }

    return invoices;

}

I don't have a complete abstraction of the separate execution, yet. However, a very basic implementation could look like this (idea stolen from Ray Houston):

protected void Do(Action unitOfWork)

{

    try

    {

        OpenSession();

        unitOfWork();

    }

    finally

    {

        CloseSession();

    }

}

public ICollection<Invoice> GetAll()

{

    ICollection<Invoice> invoices = null;

    DetachedCriteria criteria = DetachedCriteria.For<Invoice>();

    Do(() =>{

        invoices = criteria.GetExecutableCriteria(Session).List<Invoice>();

    });

    return invoices;

}

This simple abstraction provides a lot of benefit for us.

• Eliminates duplicate code (not calling OpenSession / Close Session from all repository methods)
• Removes ugly Try / Catch blocks from Repository methods
• Allows multiple Criteria to be executed from a single Session / Transaction

And most importantly - this gives us the ability to create a better abstraction of NHibernate, to support business level transactions, not just repository level transactions. Obviously, this simple example is not going to provide business level transactions. It does get us down the path, though.

A coworker and I ran into a problem yesterday - we were trying to re-use an assembly from a WinForms app, in a WebService and ran into this code:

Directory.SetCurrentDirectory(Application.StartupPath);

The problem with this, is that it uses the Application static class, which is part of System.Windows.Forms - and we're in a web service now, not a WinForms app. So, after some headache and thought, we tried to use this:

Assembly.GetExecutingAssembly().Location

That doesn't work well, either, because in the web, it gives you the ShadowCopy location of the assembly, not the original location of the assemblies. A little more thought, and a few hours later, we finally came up with this:

private static string GetBinFolder()

{

    AppDomain appDomain = AppDomain.CurrentDomain;

    string binFolder;

    if (appDomain.RelativeSearchPath != null && appDomain.RelativeSearchPath != string.Empty )

        binFolder = Path.Combine(appDomain.BaseDirectory, appDomain.RelativeSearchPath);

    else

        binFolder = appDomain.BaseDirectory;

    return binFolder;

}

The AppDomain.BaseDirectory will give you the root folder that the application is being run from - no matter what type of app you are in; windows or web. This is perfect for Windows because it alone gives us the folder that the code is running from and lets us find the assembly we need. The RelativeSearchPath is important for the web - it gives us the "bin" folder where our assemblies live. So a simple check to see if there is a relative search path (it returns null in a standard WinForms app) and combine the two if there are, otherwise just get the base directory, and we now have our folder that the assemblies are located in, so we can call:

Directory.SetCurrentDirectory(GetBinFolder());

...

Of course, this problem could have been avoided if there was proper Inversion of Control in the code... don't have time to introduce it right now, but at least we removed some code duplication by creating a single GetBinFolder() method.

As a kid, I was never part of the boy-scouts or anything; but my family and I went camping a lot, and I went camping with my youth group on several occasions. I remember hearing my parents and the various youth leaders talking about we should always leave the camp site cleaner than we found it. I always thought this was annoying - why should I clean up someone else's mess? If I clean up my own mess, isn't that good enough?

Yesterday, while helping a coworker fix some bugs in an application that I wrote around a year ago, I was suggesting ways to improve various parts of the code; move this property to a parameter of that method, make this method private and only call it from here in the owning class, and items as simple as making an if-then statement easier to read.

After a few of these suggestions, he asked me if I always clean up the code that I'm working with, even if the bug is not directly related to the code that we are cleaning up. My answer was emphatically, "yes". If I'm reading code, trying to find a bug and I'm having a hard time understand what's going on with the code, then it becomes much more difficult to find the actual bug. Even if this code does not end up being part of the bug I was looking for, by cleaning up the code I am making it more likely that I will be able to understand what this code is doing the next time I have to look at it.

Here's my basic perspective that drives all of this: if you have a hard time reading the code and seeing what it is doing, chances are, you or someone you know will have to debug that code at some point. I don't want to debug hard to read code - that's annoying, at best. I want to debug code that is easy to read and easy to understand. And I certainly don't want to make any of my coworkers debug hard to read code. I try not to torture coworkers like that. So, if my motivation is to not debug hard to read code, then doesn't it make sense that I would want to clean up that ugly code? It makes sense to me...

What does this really come down to, then? Two things:

1. Leave the code cleaner than when you arrived, by
2. Micro-refactoring - make that one line of code easier to read

(By the way - there's no such thing as "micro-refactoring". Refactoring, by definition, is exactly what I described above. Stop trying to change the architecture and learn to change that one line of ugly code. By doing this, you'll find that the architecture does change, because you clean up more than you realize and change become natural.)