My team's first swim lanes! From left to right: Backlog, In Dev, In Test, Docs, Done. sweeeeet.
In the last month or two, I have been hand coding a lot of mock and stub objects and it has become a nightmare to manage. My primary reason for doing this by hand was that Rhino Mocks 3.4 and older did not fit with the BDD style unit tests that I was writing. Yes, I made it work in a few places, but it was ugly and annoying. Fortunately, Ayende has cleaned it all up with the new syntax and made it very easy to assert that individual expectations were met, with v3.5. I finally got around to trying out the new syntax today, and I immediately fell in love with it. For example: [Concern("User Administration")]public class When_accessing_the_system_as_a_non_administrator : ContextSpecification { private IMainView view; private IMainView GetView() { IMainView mockView = MockRepository.GenerateMock<IMainView>(); mockView.Expect(v => v.EnableUserManagement()).Repeat.Never(); mockView.Expect(v => v.DisableUserManagement()).Repeat.Once(); return mockView; } protected override void Context() { User administrator = new User(); administrator.IsAdministrator = false; CurrentUser.User = administrator; view = GetView(); new MainPresenter(view); } [Observation] public void Should_not_display_the_User_Management_option() { view.AssertWasNotCalled(v => v.EnableUserManagement()); } [Observation] public void Should_hide_the_user_management_option() { view.AssertWasCalled(v => v.DisableUserManagement()); } }
In the "GetView" method, I am setting up two very distinct expectations on my mock object.
- Never Call the EnableUserManagement method.
- Call the DisableUserManagement method once.
With the new Rhino Mocks syntax, I can easily verify that each one of these expectations was called via my "should" observations.
The "AssertWasNotCalled" extension method verifies that an expectation of Repeat.Never was setup and that the method was not called.
mockView.AssertWasNotCalled(v => v.EnableUserManagement());
And the "AssertWasCalled" extension method verifies that the expectation was to call the method, and that the method actually was called.
mockView.AssertWasCalled(v => v.DisableUserManagement());
I like it. The new syntax has really simplified my Context Specification testing.
Scott Bellware (who finally started blogging again), is posting a series on Sustaining Capacity in Maturing Agile Software Teams. It's worth a read, and may be familiar territory to anyone who has done Agile and been reading about Lean. In the 3rd Post - Recognizing Entropy - he talks specifically about synchronizing the development testing and QA testing efforts: Development and QA Test Synchronization. Without greater synchronization between development and QA testing, valuable input often comes too late in a development or release cycle to be effective. Test design and test architecture are valuable inputs to development, and software design and architecture are valuable inputs to QA testing. Teams often loose unrealized capacity by not pursuing means to do more development and testing in parallel. This one really hit home for me, and is a shining beacon to the problems that my current team is going to face, soon. We had our first iteration planning and kickoff meeting yesterday, and the whole team was involved - except the testers from the QA department. For reasons that I have no control over (and honestly - they are legitimate business reasons), the QA department is not coming on to the project for another 2 months (at least). Today, the team leadership was discussing how the QA personnel would create their test scenarios, and I brought up the point that creating the test scenarios after-the-fact (after the iterations are done) is going to be very problematic. If a dev team is working from User Stories, and those User Stories are divided along various aspects of behavior, it is very likely that a single screen, workflow, or 'function' of a system is going to be divided into multiple user stories. When the QA team is involved with testing in parallel with the development team, this is not an issue because the QA team will be updating their test scenarios in each iteration. This allows them to keep the scenarios for a given screen, workflow, or 'function' of the system, growing over time. However, since our QA team is going to come into the project at least 2 months after the development team has started coding and testing, the QA team would have to sift through all of the current and previous iteration stories, try and aggregate them down into the screen, workflow, or 'function' of the system, and build test scenarios from there. If there is even a modest number of stories to sift through, this would be a daunting task for the QA team - they would be in a 'catch-up' mode from the beginning of their QA cycle, likely through the end of the project. As a result of the QA team being unable to join our project immediately, we are forced to modify our entire development process and account for the needs of the QA team, after the fact. Essentially, we are creating additional swim-lanes for our stories to travel through, during an iteration: - Backlog
- In Development
- Developer Testing
- Ad-Hoc Testing (BA's, Developers, etc. testing the app)
- QA Functional Spec Documentation
- Done
Swim-lane #4 and #5 are directly affected by the QA team not being able to enter the project immediately, and are causing the rest of the team to lower it's capacity. We (developers and BA's) are now required to do our own ad-hoc testing during the iterations, instead of letting the QA team take care of that; and we (the developers and BA's) are now required to create functional specifications, based on the user stories that are completed, to hand to the QA team once they are on the project. The functional specification documents will need to be updated for any given User Story, before the ticket is considered complete (the functional spec updates have become an acceptance criteria on every User Story). End result - because we cannot get the QA team on our project immediately, and ensure that they are testing in parallel to the developers, we as the development team are losing capacity and building entropy from the very first day of the very first iteration.
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 ???
Visual Studio 2008 installs .NET 3.5 SDK v6.0A, which is what NAnt 0.86 expects to run against – however, the .NET 3.5 SDK that is available for download, via the "Windows 2008 and .NET 3.5 SDK", is v6.1. It’s a small difference, but that difference will cause NAnt 0.86 to not be able to target .NET 3.5 if you are using the downloadable .NET 3.5 SDK. When you try to run NAnt 0.86 and have it target .NET 3.5 with the downloaded .NET SDK, you will get an “Object reference” error from NAnt and the build will fail immediately. Page Brooks has the details of how to fix NAnt to work against the v6.1 SDK. It is an easy fix, but it requires that every machine running NAnt have the v6.1 SDK installed (and if you want your devs to be able to run NAnt from their local machine, you’ll need all of the developers to install the updated version of the SDK). End Result: If you want to automate your VS2008 project builds with NAnt, you should probably have every developer on your team to install the new version of the SDK and have NAnt target that version.
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?
Anyone that has worked on a project with me is familiar with my usual "I just fixed … whatever" emails. For example - yesterday, I sent out this email to my team: FYI – I just pulled down the latest code from subversion, with the new solution and project reorganizations… there were several project reference errors in the {our project} solution – many of the projects were trying to reference the \Trunk\Code\Library\ folder for the compiled output of a project that was in the same solution. We need to make sure that projects within the same solution reference the project directly, not the compiled dll file. You can make sure this is correct by deleting everything out of the Library folder, than updating it from subversion (to make sure you have no {our project}.dll files in the Library) and then see if you get any missing file errors in visual studio, for the project references. I’ve been sending out similar email for several years now, and I’ve always felt that it was important to do so, so that the entire team can gain new insights, move forward with better techniques, creating better software, etc. Yesterday, I found An Article on the Lean Manufacturing concept of Jidoka and a few items stuck out in my mind and helped me to understand why I think that it’s important to call out these items. The core of jidoka goes well beyond the basic idea of "If it's broke, fix it now". It gets much more involved than just that. The article has a list of steps that should be taken whenever something is found to be 'broken' or 'abnormal': - Detect the abnormality
- Stop
- Fix or correct the immediate condition
- Investigate the root cause and install a countermeasure
There's a lot more to the real implementation of jidoka than just fixing the problem - finding the root cause and installing countermeasures to prevent it from happening again is by far the most important step in this process. If you fix the problem in isolation and no one else knows that the problem occurred, then it's likely that the problem will happen again for the same reasons. This gets straight to the core of why I send out the “I just fixed …” emails. I found this quote in the article that further illustrates the point: What I would not want to happen is to expect the team member who discovered the problem to try to fix it without telling anyone. But many times we expect them to do just that, and the rework becomes so deeply embedded into the routine that we can't even tell it is happening. It seems normal because it has never been flagged as abnormal. I don’t send out those emails to chastise anyone, point out that someone broke something, or anything negative like that. Honestly – I don’t care who broke something; I only care that it is broken, it gets fixed, and we find the cause and do what we can to eliminate that cause. I send out those emails because when we (as a team – any of us; developers, testers, subject matter experts, etc) find a situation that is not right, we need to fix it – but more importantly, we need to notify the rest of the team so that we can learn as a team and find ways to not make the same mistake in the future. I highly recommend that everyone go read that article and I also want to suggest that any time any of us runs into a situation where something is wrong – whether it’s code that is written or designed poorly, a references issue, the build being broken, or whatever it is – we not only fix the problem, but we inform the team of what the problem is/was and how to fix it and prevent it. We need to have this level of communication in the team, or we are not going to grow as a team or as individual developers, as fast as we could.
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.
My Previous Post talked about a sample Specification pattern implementation. Today, during a discussion with some team members, I had the revelation of how fluent interfaces are really built and how they operate under Closure of Operations. So I decided to expand my previous sample code with a more operators on the spec class, and I came up with a very powerful fluent interface for creating specifications. Here is the example I coded into my spike, to illustrate a fluent interface. Note that I changed nothing in my implementation of the ISpecification<t> or base Specification<t> from yesterday - other than to add the "Or" and "Not" specifications. The fluent interface was already there and available! //show the real power of Closure Of Operations, in creating a fluent interface! ISpecification<Foo> chainedSpec = equalSpec .And(passingSpec.And(trueSpec)) .Or(passingNotSpec) .Not(failingSpec.Or(falseSpec)) .And ( passingSpec.Or ( failingSpec.Not(falseSpec) ) );
I'll leave it to you, the reader, to implement the "Or" and "Not" specification classes. It should be pretty simple, based on the code I posted yesterday.
I can see how we could easily use an a "Closed" interface like this, to create a more fluent implementation of business rules and logic. I've often implemented multiple if-then statements in order to evaluate equality and equivalence, via multiple methods and a lot of ugly code. This specification implementation may be the answer to that ugliness.
And suddenly, Ayende's UnitOfWork and NHibernate's ICriteria make a lot more sense to me... I love those "AHA!" moments. 
Inspired by my recent readings in Domain Driven Design - specifically Chapter 10, "Supple Design" - and recent posts by David Laribee and Nigel Sampson, in combination with the recent pains I've been putting myself through, trying to test query generation code in a search screen, I decided to spike out a quick example of a reusable Specification implementation. Rather than repeat what's already been said, I'm just going to get straight to the code. using System; namespace Spec_Spike { class Program { static void Main() { Foo foo1 = new Foo(); foo1.Bar = "Test"; ISpecification<Foo> equalSpec = new Specification<Foo>(foo => foo.Bar == "Test"); ISpecification<Foo> notEqualSpec = new Specification<Foo>(foo => foo.Bar != "Not Equal To This Text"); ISpecification<Foo> falseSpec = new Specification<Foo>(foo => false); ISpecification<Foo> passingSpec = equalSpec.And(notEqualSpec); ISpecification<Foo> failingSpec = passingSpec.And(falseSpec); Console.WriteLine(equalSpec.IsSatisfiedBy(foo1)); Console.WriteLine(notEqualSpec.IsSatisfiedBy(foo1)); Console.WriteLine(passingSpec.IsSatisfiedBy(foo1)); Console.WriteLine(failingSpec.IsSatisfiedBy(foo1)); } } |