Caffeine-Powered Life

DRYing Up Controller Actions With Responders

I read a few books over the long weekend. One of those books was Crafting Rails 4 Applications by José Valim. I’m going to say that this is a pretty advanced book on Rails, since it really digs into the Rails framework internals, especially when it comes to overriding the default functionality.

One of the topics is how to DRY up your controllers by writing custom responders. That got me thinking. Can we do the same thing in .NET?

The “Before”

I have two controllers in my Portfolio application: a TagsController and a TasksController. These controllers contain the following lines.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
// In TasksController
[HttpDelete]
public ActionResult Delete(DeleteTaskCommand command)
{
  var task = mediator.Send(command);
  var message = string.Format("Successfully deleted task: {0}", task.Title);
  this.Flash("success", message);
  return Json(new { success = true });
}

// In TagsController
[HttpDelete]
public ActionResult Delete(DeleteTagCommand command)
{
  var tag = mediator.Send(command);
  var message = string.Format("Successfully deleted tag: {0}", tag.Description));
  this.Flash("success", message);
  return Json(new { success = true });
}

That is some pretty similar code. In fact, that’s about as un-DRY as you can get. One might say that’s going to happen if you try to create some sort of standard in what your controllers and actions should look like.

Delete Is Easy

Notice that [HttpDelete] attribute on our Delete() methods? That means that the only way these methods are ever going to be called is via XHR. Currently, no browsers can create a DELETE request without the support of JavaScript. That means that our controller pattern has no reason to be complicated.

The DeleteResponder

It’s pretty obvious what we’re trying to achieve here, so let’s start by creating a branch and stubbing out some tests.

> git checkout -b responder-spike

Writing out your tests first is a good way to define a spec, so let’s do that. We can already tell what happens in our controller action, and we want our responder to mimic this behavior.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
public class DeleteResponderTests
{
  private DeleteResponder deleteResponder;

  [SetUp]
  public void Before_each_test()
  {

  }

  [Test]
  public void It_should_send_a_request_to_the_mediator()
  {
    Assert.Fail();
  }

  [Test]
  public void It_should_add_a_successful_flash_message()
  {
    Assert.Fail();
  }

  [Test]
  public void It_should_have_an_expected_JSON_result()
  {
    Assert.Fail();
  }
}

What is our responder going to look like? I have a good idea what I think it should look like up front.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
public class DeleteResponder
{
  private readonly ApplicationController controller;

  public DeleteResponder(ApplicationController controller)
  {
    if (controller == null) throw new ArgumentNullException("controller");
    this.controller = controller;
  }

  public ActionResult RespondWith<T>(IMediator mediator, ICommand<T> command)
  {
    throw new NotImplementedException();
  }
}

The easiest part is to send the command to the mediator. We can write that test fairly easily. We’re also going to need a fake controller.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
public class DeleteResponderTests
{
  private ICommand<object> command;
  private DeleteResponder deleteResponder;
  private Mock<IMediator> mockMediator;

  [SetUp]
  public void Before_each_test()
  {
    mockMediator = new Mock<IMediator> { DefaultValue = DefaultValue.Mock };
    deleteResponder = new DeleteResponder(new FakeController());
    command = new FakeCommand();
  }

  [Test]
  public void It_should_send_a_command_to_the_mediator()
  {
    deleteResponder.RespondWith(mockMediator.Object, command);
    mockMediator.Verify(x => x.Send(command));
  }

  class FakeCommand : ICommand<object> { }

  class FakeController : Controller { }
}

This command implementation is extremely simple.

1
2
3
4
5
public ActionResult RespondWith<T>(IMediator mediator, ICommand<T> command)
{
  mediator.Send(command);
  return null;
}

Next, we want to make sure that the JSON result is returned appropriately.

1
2
3
4
5
6
7
8
[Test]
public void It_should_have_an_expected_JSON_result()
{
  var result = deleteResponder.RespondWith(mockMediator.Object, command);
  var jsonResult = MvcTest.HasExpectedActionResult<JsonResult>(result);
  var objectDictionary = jsonResult.Data.ToDictionary();
  Assert.IsTrue(objectDictionary["success"]);
}

This implementation is also fairly simple.

1
2
3
4
5
6
7
8
9
10
public ActionResult RespondWith<T>(IMediator mediator, ICommand<T> command)
{
  mediator.Send(command);
  var jsonResult = new JsonResult();
  jsonResult.Data = new
  {
    success = true
  };
  return jsonResult;
}

The only remaining piece is how we add the flash message. I’m actually going to leave this piece alone. If you want to see how I did it, then check out my version of the DeleteResponder.

The “After”

Our TagsController and TasksController now look like the following:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
// TagsController
[HttpDelete]
public ActionResult Delete(DeleteTagCommand command)
{
  return new DeleteResponder(this)
      .RespondWith(mediator, command);
}

// TasksController
[HttpDelete]
public ActionResult Delete(DeleteTaskCommand command)
{
  return new DeleteResponder(this)
      .RespondWith(mediator, command);
}

Eight total lines of code become two total lines of code, and the common functionality is moved to a utility class.

Also, just because the delete is easy, that doesn’t mean that everything has to be difficult. Your controllers should be rather thin. They invoke a service. They do a query. They return an ActionResult. This responder pattern can be applied to any form post.

Learning Code Contracts

Taking the advice of Patrick Smacchia, I decided to start learning more about Code Contracts. What a better way to use them, than to work with them? So, I added them to my MVCFlashMessages project.

What Code Contracts Are

Code contracts are assertions in your code, much like unit tests are assertions. They provide both runtime and compile-time checking of conditions. Contracts are a debugging tool. They help direct you to finding (and eliminating) bugs. They are an elegant way of saying, “I expected this condition to be true. If it is, then go on. If not, I am going to throw an exception.”

Let’s look at an example from the FlashMessageCollection class.

1
2
3
4
5
6
7
8
9
10
11
12
public class FlashMessageCollection
{
  private readonly TempDataDictionary storage;

  public FlashMessageCollection(TempDataDictionary storage)
  {
    Contract.Requires<ArgumentNullException>(storage != null);
    this.storage = storage;
  }

  // snip
}

The .Requires<>() line is expecting that storage not be null. If the condition is failed, an ArgumentNullException will be thrown. It is functionally equivalent to this very common line of code.

1
2
if (storage == null)
  throw new ArgumentNullException("storage");

The fun part about code contracts is, of course, their integration into Visual Studio.The code contract can be statically checked; the if-then-throw exception cannot. This means that VS gives you hints that what you are about to do could possibly throw an exception. Neat!

There are three types of contracts you should be aware of.

  1. Preconditions
  2. Postconditions
  3. Assertions

Preconditions

Preconditions check state before a method starts. Usually, this is all the parameter checks or internal state validations that happen at the start of a method.

Here’s another example, this time requiring a valid value for the indexer.

1
2
3
4
5
6
7
8
9
public FlashMessage this[int index]
{
  get
  {
    Contract.Requires<ArgumentOutOfRangeException>(0 <= index && index < this.Count);
    EnsureFlashMessagesIsInitialized();
    return flashMessages[index];
  }
}

There’s nothing too fancy about .Requires(). As developers, we are pretty used to checking input parameters.

Postconditions

Postconditions are promises about the return values of methods. This is a newer concept for code contracts. While they appear at the top of a method, they are telling you information about the return clause.

1
2
3
4
5
6
7
8
9
public int Count
{
  get
  {
    Contract.Ensures(Contract.Result<int>() >= 0);
    EnsureFlashMessagesIsInitialized();
    return flashMessages.Count;
  }
}

The above line of code says the following.

I promise that the value I am about to return will be greater than or equal to 0. If not, I am going to throw an exception at runtime.

The only other place where I have used an .Ensures() clause is to state that a return value will never be null.

Thus, code contracts fulfill the design requirement that code should fail as early as possible.

Assertions

Assertions happen in the middle of your code and are neither a precondition or a postcondition. Again, you’re just checking that some condition is true. Unlike preconditions and postconditions, which can only appear at the beginning of a method block, assertions can appear anywhere in your code.

1
2
3
4
5
6
7
8
9
10
11
private void EnsureFlashMessagesIsInitialized()
{
  // If there are any flash messages already in the collection, do not do this
  // step again, or you will risk populating the collection twice.
  if (flashMessages.Any())
    return;

  IEnumerable<FlashMessage> objectFromStorage = storage.GetFlashMessages();
  Contract.Assert(objectFromStorage != null);
  flashMessages.AddRange(objectFromStorage);
}

We know that if we try to .AddRange(null), we will get an ArgumentNullException, so let’s add a contract to make sure that doesn’t happen.

This really isn’t so different from checking a null value early.

1
2
3
4
5
6
7
// 1. With code contracts, you will get a ContractException instead of a
//    NullReferenceException.
Contract.Assert(obj != null);

// 2. Or you can use a classic null check.
if (obj == null)
    throw new NullReferenceException("The obj is null!");

The above two examples do the exact same thing.

What Code Contracts Are Not

Code contracts are a debugging tool. They are not a replacement for unit tests. Good code contracts and good unit tests work together.

Have you ever said to yourself or your team something like the following?

What’s the point of unit tests? We can’t possibly cover every scenario that would ever happen!

That’s where code contracts can really come into play. Have you ever seen the bowling kata? It’s basically a way to teach TDD. Imagine how we would add contracts to a bowling game.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
public class Game
{
  private int currentFrame = 1;
  private int currentScore = 0;
  private List<int> rolls = new List<int>();

  public int CurrentFrame
  {
    get
    {
      Contract.Ensures(0 <= Contract.Returns<int>() && Contract.Returns<int>() <= 10);
      return currentFrame;
    }
  }

  public int CurrentScore
  {
    get
    {
      Contract.Ensures(0 <= Contract.Returns<int>() && Contract.Returns<int>() <= 300);
      return currentScore;
    }
  }

  public void Roll(int pins)
  {
    Contract.Requires<ArgumentOutOfRangeException>(0 <= pins && pins <= 10);
    rolls.Add(pins);
    // etc.
  }
}

We are still going to write tests. We are still going to follow all the rules of TDD. However, without knowing anything else about what a Game is going to look like, I absolutely know that the above contracts must always be true. A bowling frame of 11 or a score of 301 is never correct.

Code Contracts Are Cancerous

If you turn on the feature “Fail build on warnings,” be prepared to spend some time working through the issue of Contracts Cascading Errors. Here’s an example.

See that purple squiggly line? My FlashMessageCollection constructor has a code contract on it that says the TempDataDictionary can never be null. The IDE is giving me a warning. I can eliminate that warning by adding an additional contract.

1
2
3
4
5
Contract.Requires<ArgumentNullException>(controller != null);
Contract.Requires<ArgumentException>(controller.TempData != null);
FlashMessageCollection flashMessageCollection = new FlashMessageCollection(controller.TempData);
FlashMessage flashMessage = new FlashMessage(key, message);
flashMessageCollection.Add(flashMessage);

The only problem with this is that my FlashMessage constructor also has contracts on the key and message parameters. Since there’s no guarantee on public methods, I need to cover these contracts as well. To eliminate all the build warnings, this is what my method finally looks like.

1
2
3
4
5
6
7
8
9
10
public static void Flash(this Controller controller, string key, string message)
{
  Contract.Requires<ArgumentNullException>(controller != null);
  Contract.Requires<NullReferenceException>(controller.TempData != null);
  Contract.Requires<ArgumentNullException>(key != null);
  Contract.Requires<ArgumentNullException>(message != null);
  FlashMessageCollection flashMessageCollection = new FlashMessageCollection(controller.TempData);
  FlashMessage flashMessage = new FlashMessage(key, message);
  flashMessageCollection.Add(flashMessage);
}

Thus, the “cancer” of contracts. Much like calling something dynamic or using async/await, once you put contracts in one part of your code, the contracts will start to spread. In this example, I have more contracts than lines of production code. This isn’t a bad thing. Remember that contracts enforce the design rule that you should fail as early as possible. It is something you should be aware of if you treat build warnings as errors.

Happy coding!

Why the Repository?

The great news is that we have myriad ways to communicate, and the word is getting out there! As time marches on, developers are asking questions. Those questions are getting answered on blogs and StackOverflow. The problem, though, is that these wonderful communication tools are shortcuts.

I am finishing up work on a series of ASP.NET MVC projects. The work I’m seeing looks something like this…

1
2
3
4
5
6
7
public ActionResult NewItem(ItemViewModel form)
{
  var itemsRepository = new ItemsRepository();
  var item = ConvertViewModelToEntity(form);
  itemsRepository.Insert(item);
  return RedirectToAction("ListItems");
}

Mostly, it’s not so terrible. I think we can all agree that we’ve see a lot worse.

The problem here is that this type of code doesn’t really provide us anything. When we look into the ItemsRepository, we see something like this.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
public class ItemsRepository
{
  public Item[] GetAllItems()
  {
    using (SqlConnection connection = OpenConnection())
    {
      // etc.
    }
  }

  public Item GetItem(int id)
  {
    using (SqlConnection connection = OpenConnection())
    {
      // etc.
    }
  }

  public int Insert(Item item)
  {
    using (SqlConnection connection = OpenConnection())
    {
      // etc.
    }
  }

  public void Update(Item item)
  {
    using (SqlConnection connection = OpenConnection())
    {
      // etc.
    }
  }

  private static SqlConnection OpenConnection()
  {
    var connection  = new SqlConnection(ConfigurationManager.ConnectionStrings[0].ConnectionString);
    connection.Open();
    return connection;
  }
}

Pay no attention to the fact that it is late 2013 and we are writing .NET 1.1 style data access.

This developer has pushed everything down one layer, but hasn’t made anything easier to test or decoupled anyclass from another. Furthermore, the way the data access layer is written, we cannot even code against an IDbConnection interface. We are using some pieces dependent upon the SqlConnection class.

Why do we write interfaces? Why do we inject dependencies? Why do we isolate classes? Why do we write small methods? Testing! These tools, these SOLID principles, we use these practices for a reason. Ultimately, they exist to improve code quality. If you’re not writing testable code, then what’s the point?

Related Posts

Data Access Still Matters

This blog post was inspired by Bill Karwin’s book SQL Antipatterns:Avoiding the Pitfalls of Database Programming. I read this over Thanksgiving weekend. If you are a developer, and you do database work, this is on the must-read list.

I agreed with just about everything in his book except a few little details. In Chapter 4, “ID Required,” Mr. Karwin lays out two antipatterns.

  1. The primary key is always an autoincrementing column named id.
  2. The primary key is not necessary when there is another unique restriction present naturally in the data.

Regarding the First

The first condition is something like this.

1
2
3
4
5
6
7
8
CREATE TABLE employees (
  id INT NOT NULL IDENTITY(1, 1),
  last_name VARCHAR(64) NOT NULL,
  first_name VARCHAR(64) NOT NULL,
  department_id INT NOT NULL,
  -- ...
  CONSTRAINT [pk_employees] PRIMARY KEY CLUSTERED ([id] ASC)
);

I agree that not every table requires a primary key named id. There are lots of situations where not using id or not having autoincrementing values is helpful. However, understand that with technologies like NHibernate with fluent mapping or Rails’ ActiveRecord, it is terribly damn convenient for the developer.

NHibernate with explicit mapping won’t much matter, since you must define the exact purpose of each and every column. It’s only when you’re using some of the implicit mapping magic that this comes into play.

Also, if you’re using PostgreSQL, there is some syntactic sugar you can use when joining tables.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
-- Using id
SELECT e.id AS employee_id, e.first_name, e.last_name, SUM(te.total_hours) AS sum_total_hours
FROM employees e
  INNER JOIN time_entries te ON e.id = te.employee_id
WHERE te.entry_date = N'2013-06-16'
GROUP BY e.id, e.first_name, e.last_name

-- Using table-specific columns
SELECT e.employee_id
SELECT e.id AS employee_id, e.first_name, e.last_name, SUM(te.total_hours) AS sum_total_hours
FROM employees e
  INNER JOIN time_entries te USING (employee_id)
WHERE te.entry_date = N'2013-06-16'
GROUP BY e.id, e.first_name, e.last_name

In the above example, you can use the USING keyword to join two tables on columns when the names match. If you’re explicit about calling the Employee ID employee_id instead of id, you can take advantage of syntax.

Consider your data access. If you think you’ll be handcrafting a lot of SQL, then maybe this syntactic sugar is worth it. Are you using a database where this even matters? Are you using a data access tool (NHibernate with fluent mapping, ActiveRecord) that takes advantages of the primary key being called id? There’s a lot to consider here beyond, “Don’t do it.”

Regarding the Second

Mr. Karwin also describes the following as an antipattern.

1
2
3
4
5
6
CREATE TABLE users (
  user_id INT NOT NULL IDENTITY(1, 1) PRIMARY KEY,
  username VARCHAR(64),
  -- ...
);
CREATE UNIQUE INDEX [unique_username] ON [users] ([username]);

He says that this table has two unique identifiers, since both user_id and username unique identify a row. So I tried this in my Portfolio project. I have the ability to create tags and apply these tags to events. The slug for these tags are each unique. So I tried dropping the id column in favor of just using the slug column as the primary key. I made sure to turn on cascading updates every place a tag was referenced. Imagine my surprise when I saw this error message when I tried to edit a tag.

OK, I wasn’t surprised. I knew this would happen. But only because I’ve been burned by it before.

NHibernate won’t let you change the primary key of a loaded record. As it turns out, neither will Entity Framework or ActiveRecord. So I would offer up just one little addendum to the second snafu. It is fine to use a real-valued column if that value isn’t going to change, depending on what kind of data access technology you are using in your application. Are usernames allowed to change? Are tag slugs going to be modified? Are you sure employees never get the social security numbers wrong? If so, I wouldn’t recommend using those as primary keys — with or without cascading.

Fixing allowDefinition=MachineToApplication Build Error

Just a quick fix for this build error.

It is an error to use a section registered as allowDefinition=‘MachineToApplication’ beyond application level. This error can be caused by a virtual directory not being configured as an application in IIS.

If you look in your project’s obj folder, you will probably see multiple outputs. If you haven’t made any changes to the default build configurations, then you will have both Debug and Release folders. This is the cause of this error.

The fix is simple, we just need to get rid of these folders. We’re going to add a pre-build event that will get rid of all obj folders except the one we are building. Paste the following into your Pre-build event command line.

rmdir /s /q $(ProjectDir)\obj
mkdir $(ProjectDir)\obj
mkdir $(ProjectDir)\obj\$(ConfigurationName)

Like so…

I need this on every project I work on. Time to put it out here instead of looking it up from scratch each time.

Using Modernizr to Detect HTML5 Support

Just a little something I wrote yesterday. I had an issue of adding a datepicker to a UI. Being the developer I am, my first response was, “Get a better browser. Chrome and Firefox both native support <input type="date"/> tags.” I guess that didn’t fly. So, this is what I wrote instead.

I use the Modernizr library. I also take advantage of the following lines doing the exact same thing.

1
2
3
var now = new Date();
now.getFullYear();      // => returns the current year
now["getFullYear"]();   // => returns the current year

That’s all!

Anemic Domain Models: Blame Complexity

This is a reply to Steve Wilkes’ post on I See Anemic Domain Models. I was going to write this as a reply, but then it got way too long.

I also dislike the Anemic Domain Model antipattern. I love the idea of nouns and verbs. Our models, or classes, are nouns. The actions those models can take, or verbs, become methods. You end up with a wonderful programming grammar.

Keep it simple.

Let’s start with a really simple domain model. We have a task. We can complete that task.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
public class Task
{
  public virtual int Id { get; set; }
  public virtual string Description { get; set; }
  public virtual DateTime CreatedAt { get; set; }
  public virtual DateTime? CompletedAt { get; set; }
  public virtual bool IsCompleted { get; set; }

  public virtual void Complete()
  {
    if (!IsCompleted)
    {
      IsCompleted = true;
      CompletedAt = DateTime.UtcNow;
    }
  }
}

The above it truly some beautiful code. I wish everything I worked on looked like that.

To use the above method, we will probably do something like this.

1
2
3
4
5
6
7
8
9
using (var session = sessionFactory.OpenSession())
{
  using (var txn = session.BeginTransaction())
  {
    task = session.Load<Task>(id);
    task.Complete();
    txn.Commit();
  }
}

Wow! That was so easy!

It doesn’t always get better.

When an object is simple and well-defined, this is great. So let’s make it a bit more complicated. What if instead of a simple IsCompleted flag, we now have user-defined statuses? Not only do we want to trak the current status of a task, we want to maintain a status history.

1
2
3
4
5
6
7
8
9
create table task_statuses (
  task_status_id int not null identity(1, 1),
  description varchar(256) not null,
  is_default bit not null,
  is_completed bit not null,
  trigger_emails bit not null,
  trigger_chart_regeneration bit not null,
  constraint [pk_task_statuses] primary key clustered ([task_status_id] asc)
);

We should also check that we’re not setting a status twice. There’s nothing completely awful if this happens, so there’s no need for an exception, probably a race condition in the UI or something like that. However, we do want to know about it.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
public class Task
{
  public virtual TaskStatus CurrentStatus { get; set; }

  public virtual IList<StatusHistory> StatusHistories { get; set; }

  public virtual void SetStatus(TaskStatus newStatus)
  {
    if (this.CurrentStatus == newStatus)
    {
      log.Warn(string.Format("Attempting to set task {0} from status {1}:{2} to status {3}:{4}. Someone should really write a better UI to prevent this scenario from happening.", Id, CurrentStatus.Id, CurrentStatus.Description, newStatus.Id, newStatus.Description));
      return;
    }

    var statusHistory = new StatusHistory
    {
      Status = newStatus,
      AssignedTo = newStatus.AssignedTo,
      Task = this,
      CreatedAt = DateTime.UtcNow
    };
    this.StatusHistorys.Add(statusHistory);
    this.CurrentStatus = newStatus;
    this.AssignedTo = newStatus.AssignedTo;
    this.IsCompleted = newStatus.IsCompleted;
  }
}

While we’re adding complexity, let’s introduce a task workflow. You’re only allowed to pass from certain statuses to other statuses given conditions stored in a database table. The simple version of this table would look something like the following.

1
2
3
4
5
6
create table status_workflows (
  status_workflow_id int not null identity(1, 1),
  from_status_id int not null,
  to_status_id int not null
  constraint [pk_status_workflows] primary key clustered ([status_workflow_id] asc)
);

Our method signature would need to change to include these workflows. The

1
2
3
4
5
6
7
8
9
10
11
12
13
public virtual void SetStatus(Status newStatus, IEnumerable<StatusWorkflow> statusWorkflows)
{
  bool isAllowedWorkflow = statusWorkflows
     .Any(sw => sw.FromStatus == this.CurrentStatus && sw.ToStatus == newStatus);

  if (!isAllowedWorkflow)
  {
    // We've written a custom ApplicationException to handle this scenario.
    throw new InvalidWorkflowTransitionException(CurrentStatus, newStatus);
  }

  // etc.
}

This method is public, so we should probably take care of some null checks. FxCop is going to warn about that.

1
2
3
4
5
6
7
public virtual void SetStatus(Status newStatus, IEnumerable<StatusWorkflow> statusWorkflows)
{
  if (newStatus == null) throw new ArgumentNullException("newStatus");
  if (statusWorkflows == null) throw new ArgumentNullException("statusWorkflows");

  // etc.
}

What if we also need to trigger emails? We know better than to create a new SmtpClient instance in our class because that’s not going to be unit-testable. That means that we need to create a wrapper class for SmtpClient and pass in the adapter via method injection.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
public virtual void SetStatus(
    Status newStatus,
    IEnumerable<StatusWorkflow> statusWorkfows,
    ISmtpAdapter smtpAdapter)
{
  // etc.

  // Fetch the correct email template from ???
  Template mailTemplate = ...;

  // Set the task for mail template.
  mailTemplate.Task = this;

  // Create a new email message and send it.
  string mailBody = template.TransformText();
  MailMessage mailMessage = ...;
  smtpAdapter.Send(mailMessage);
}

Oh, we also have burndown chart regeneration. It turns out, I need to know a lot more info from the database to regenerate that burndown chart. Now I’m passing around my database session.

1
2
3
4
5
6
7
8
9
public virtual void SetStatus(
    Status newStatus,
    IEnumerable<StatusWorkflow> statusWorkfows,
    ISmtpAdapter smtpAdapter,
    IBurndownChartGenerator burndownChartGenerator,
    ISession session)
{
  // Really?
}

As you see, as our application complexity grows, our method continues to add more and more parameters, and our method grows longer.

This is why double dispatch exists.

Screw it. This is getting way too complicated for a domain model. Let’s just go with what we learned from Eric Evans in Domain Driven Design instead. We’ll just use the double dispatch pattern.

1
2
3
4
public virtual void SetStatus(Status newStatus, IStatusSetter statusSetter)
{
  statusSetter.SetNewStatus(this, newStatus);
}

Wow! We have finally cleaned up that code! It looks a lot better now. And I didn’t even get into assigning a task to a user. (Is the user active? Is the user on this project? Can the user be assigned tasks with this task type? Are you doing any type of capacity planning?)

But now what’s the point of the SetStatus() method? All the work is now being done by our IStatusSetter logical service. Let’s look at the next two pieces of code that do the exact same thing.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
// Version 1:
IStatusSetter statusSetter = ServiceLocator.Instance.GetService<IStatusSetter>();
ISession session = ServiceLocator.Instance.GetService<ISession>();
using (var txn = session.OpenTransaction())
{
  task = session.Load<Task>(id);
  nextStatus = session.Load<TaskStatus>(nextStatusId);
  task.SetStatus(nextStatus, statusSetter);
  txn.Commit();
}

// Version 2:
IStatusSetter statusSetter = ServiceLocator.Instance.GetService<IStatusSetter>();
ISession session = ServiceLocator.Instance.GetService<ISession>();
using (var txn = session.OpenTransaction())
{
  task = session.Load<Task>(id);
  nextStatus = session.Load<TaskStatus>(nextStatusId);
  // We skip the double dispatch and just call SetNewStatus directly.
  statusSetter.SetNewStatus(task, nextStatus);
  txn.Commit();
}

I write services because our models are complicated. I never get the simple music library, blog, or to-do list apps to work on any longer. This leads to the Single Responsibility Principle. If any class is handling both setting statuses and assigning tasks and sending emails and updating burndown charts, I’m instantly going to ask, “Who threw up all over this code?” That is plainly too much going on inside one class.

Honestly, service methods are just better ways of handling this kind of stuff. What would you rather see in your Task object? All of that stuff up there or none of it?

ActiveRecord is too busy by default.

If you’re used to using Rail’s ActiveRecord, I’m going to stake this claim right away: ActiveRecord has enough going on already. Please, do not add business logic to your ActiveRecord models. Between the finders and validators, these classes already have enough going on. Adding any business logic to them just makes them even more complicated.

This is an ActiveRecord object that has only minimal logic for validations and a few custom finders. I even tried to keep this class small by using helpers and concerns where I could.

episode.rb
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
require 'textacular/searchable'

class Episode < ActiveRecord::Base

  include ActiveModel::ForbiddenAttributesProtection
  include EpisodeValidations
  include NewEpisodeBehavior
  include MarkdownBehavior

  # Add support for PG full text search
  extend Searchable(:title, :summary_markdown, :content_markdown)

  # Active record relations.
  has_and_belongs_to_many :tags

  # Named scopes.
  scope :free_to_view, lambda { with_tag('free') }
  scope :published, lambda { where('status = ? and published_on <= ?', 'published', Date.today) }
  scope :order_by_published_on, lambda { order('published_on desc') }
  scope :with_tag, lambda { |slug| joins(:tags).where('tags.slug = ?', slug) }

  # Callbacks
  before_validation :ensure_video_path_leading_slash, :render_html

  def free_to_view?
    self.tags.collect(&:slug).include?('free')
  end

  def published?
    self.published_on <= Date.today && self.status == 'published'
  end

  def to_param
    self.slug
  end

  private

    def ensure_video_path_leading_slash
      if self.video_path
        self.video_path = '/' + self.video_path unless self.video_path[0] == '/'
      end
    end

    def render_html
      self.content_html = markdown(self.content_markdown)
      self.summary_html = markdown(self.summary_markdown)
    end

end

We still end up with 50 lines of code, and this class doesn’t even perform any real business logic.

Let’s start drawing some lines.

Not all software is easy. But that doesn’t mean it’s going to be difficult.

Your service classes should NOT be static.

There’s a big difference in testability between the following lines of code…

1
2
ITaskCompletionService taskCompletionService = ...; // get the service
taskCompletionService.CompleteTask(123);

…and this…

1
TaskCompletionService.CompleteTask(123);

The latter version is nearly impossible to test the myriad scenarios that you may encounter. Also, it means that everything that is used by TaskCompletionService is also going to be a static reference. That means that those are going to be nearly impossible to test, as well.

Your data model is not your domain model is not your view model.

It doesn’t take very much complication before we run into a scenario where what we store in the database is not the same as the object that we manipulate in code. Frequently, this business object isn’t what I want to show the user.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
public class EmployeeDataRow
{
  // This is what the employee record looks like when serialized
  // in our database.

  public int EmployeeId { get; set;}
  public string SSN { get; set; }
  public string FirstName { get; set; }
  public string LastName { get; set; }
  public IDictionary<string, string> Properties { get; set; }
}

public class Employee
{
  // This is our domain model that we work with in our
  // code. We have created an actual SSN type.

  private readonly EmployeeDataRow data;

  public Employee()
    : this(new EmployeeDataRow())
  {
  }

  public Employee(EmployeeDataRow data)
  {
    this.data = data;
  }

  public int EmployeeId { get; set; }
  public SSN SSN
  {
    get { return new SSN(data.SSN); }
    set { data.SSN = value.ToString(); }
  }
  public string FirstName { get; set; }
  public string LastName { get; set; }
  public MailAddress WorkEmail
  {
    get { return GetPropertyValue<MailAddress>("work_email"); }
    set { SetPropertyValue("work_email", value); }
  }
  public DateTime DateOfHire
  {
    get { return GetPropertyValue<DateTime>("date_of_hire"); }
    set { SetPropertyValue("date_of_hire", value); }
  }

  // etc. with more properties...
}

public class EmployeeViewModel
{
  // This is what we'd actually display to users, which probably
  // won't be the same as what we store in the database or the
  // domain model that we actually work with in code.
  //
  // SSN will be a string masked as XXX-XX-1234.
  //
  // Date of hire will be a string formatted as dd-mmm-yyyy.
  //
  // Work email will only show the part before the '@'.
  //
  // Salary estimate needs to be computed for hourly employees, based on
  // 40 hrs per week, 48 weeks per year.
}

Yes, these object are all very similar. However, what we store, what we work with, and what we display to users is not the same object.

We also need to realize that the objects that we work with (our domain models) can have a lot more layers to them than our data objects will. For example when a user shows up to your web site that user is a Guest. Once they log in, that person is a User. You might even distinguish between a FreeUser and a Subscriber. Yes, all of these objects will be serialized to a users table. However, it should be clear that different types of users would have different methods available to them.

Conclusion.

There are reasons that domain models end up anemic. Our rules are complex. Our classes are too big. If I store everything that a Task can do, that will be a plain and clear violation of the Single Resonsibility Principle. Writing software is about making choices. I would much rather have clear domain service that does one thing than have one Task class with too many built-in features.

Unit Testing a Controller Action

This is one of those sore subjects that never seems to get any easier. There’s a lot more MVC stuff out there, and most of it is junk. Let’s review a few simple rules.

What Should a Controller Action Do?

  1. Do any web-specific interactions. This may mean handling Session or Request variables, as necessary.
  2. Call into a service layer, as necessary.
  3. Return an ActionResult.

That’s it. If your controller is doing more than that, then your controller is doing too much stuff.

What About That Service Layer?

I see a lot of code that looks something like this. There’s nothing technically wrong with code that looks like this.

EventsController.cs
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
public class EventsController : ApplicationController
{
  private readonly IEventsService eventsService;

  public EventsController(IEventsService eventsService)
  {
    this.eventsService = eventsService;
  }

  public ActionResult Index()
  {
    var e = eventsService.GetEvents();
    return View("Index", e);
  }

  public ActionResult Show(int id)
  {
    var e = eventsService.GetEventById(id);
    return View("Show", e);
  }

  [HttpGet]
  public ActionResult New()
  {
    var model = new Event();
    return View("New", model);
  }

  [HttpPost]
  public ActionResult New(Event e)
  {
    eventsService.CreateNewEvent(e);
    return RedirectToAction("Show", new { id = e.Id });
  }

  [HttpGet]
  public ActionResult Edit(int id)
  {
    var e = eventsService.GetEventById(id);
    return View("Edit", e);
  }

  [HttpPost]
  public ActionResult Edit(Event e)
  {
    eventsService.UpdateEvent(e);
    return RedirectToAction("Show", new { id = e.Id });
  }

  [HttpDelete]
  public ActionResult Delete(int id)
  {
    eventsService.DeleteEvent(id);
    return Json(new { success = true });
  }
}

Honestly, if every MVC application I ever looked at was structed like that from now on, I would be mostly satisfied. My one gripe with this is that I can tell, without even looking, that your IEventsService implementation is going to be just as ugly as your EventsController would have been. You’ve just moved the ugly down one layer. Sweeping dirt under the rug doesn’t get rid of the dirt!

Still, this is testable now.

Writing The Test

Since we’ve made our controllers so thin, writing the test is actually really easy now.

EventsControllerTests.cs
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
[TestFixture]
public class EventsControllerTests
{
  public class EventsControllerTestContext
  {
    public void InitializeContext()
    {
      Controller = new EventsController();
      Event = new Event();
      MockService = new Mock<IEventsService>()
      {
        DefaultValue = DefaultValue.Mock
      }
    }

    public EventsController Controller { get; set; }
    public Event Event { get; set; }
    public Mock<IEventsService> MockService { get; set; }
  }

  public class GetIndex : EventsControllerTestContext
  {
    [SetUp]
    public void Before_each_test()
    {
      InitializeContext();
    }

    [Test]
    public void It_fetches_events()
    {
      Controller.Index();
      MockService.Verify(x => x.GetEvents(), Times.Once());
    }

    [Test]
    public void It_returns_a_view()
    {
      var actionResult = Controller.Index();
      Assert.IsInstanceOf<ViewResult>(actionResult);
    }

    [Test]
    public void It_has_the_expected_model()
    {
      var actionResult = Controller.Index();
      object model = ((ViewResult)actionResult).Model;
      Assert.IsInstanceOf<IEnumerable<Event>>(model);
    }
  }
}

What is there to test?

  1. Was the correct service called?
  2. Is the expected output returned?
  3. Does the model match?

That’s it. Repeat as necessary.

I Still Love/Hate CoffeeScript

The Love

  1. It’s a lot less code and syntax noise.
  2. It creates really pretty JavaScript and does all the “nasty stuff” for you.

Examples are fun, so let’s look at something I wrote this week.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
class MultiSelect

  constructor: (options) ->
    defaults =
      checkboxSelector: ".multi-select-checkbox",
      onSelector: ".multi-select-on",
      selectAllSelector: "#multi-select-all"
    @options = $.extend(defaults, options)
    console.log("MultiSelect: constructor finished")

  hide: ->
    console.log("MultiSelect: hide()")
    $(@options.onSelector).hide()

  initialize: ->
    @hide()
    @listenForCheckedEvent()

  listenForCheckedEvent: ->
    # Be sure to call .bind() on the callback, otherwise the callback
    # will have the context of the checkbox, not the class definition.
    $(@options.selectAllSelector).on("click", (->
      isChecked = $(@options.selectAllSelector).is(":checked")
      console.log("MultiSelect: select all was clicked. isChecked: #{isChecked}")
      @toggleChecked(isChecked)
    ).bind(@))

  setChecked: (value) ->
    console.log("MultiSelect: setChecked(#{value})")
    $(@options.checkboxSelector).prop("checked", value)

  show: ->
    console.log("MultiSelect: show()")
    $(@options.onSelector).show()

  toggleChecked: (isChecked) ->
    if (isChecked)
      @setChecked(true)
    else
      @setChecked(false)

There’s nothing too difficult. All I want to do is have a “Select All” checkbox on a web site that selects everything when checked. I also need to the ability to show/hide these checkboxes. 40 lines, including whitespace, comments and console logging.

Here’s the generated output of that class.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
window.MultiSelect = (function() {
  function MultiSelect(options) {
    var defaults;
    defaults = {
      checkboxSelector: ".multi-select-checkbox",
      onSelector: ".multi-select-on",
      selectAllSelector: "#multi-select-all"
    };
    this.options = $.extend(defaults, options);
    console.log("MultiSelect: constructor finished");
  }

  MultiSelect.prototype.hide = function() {
    console.log("MultiSelect: hide()");
    return $(this.options.onSelector).hide();
  };

  MultiSelect.prototype.initialize = function() {
    this.hide();
    return this.listenForCheckedEvent();
  };

  MultiSelect.prototype.listenForCheckedEvent = function() {
    return $(this.options.selectAllSelector).on("click", (function() {
      var isChecked;
      isChecked = $(this.options.selectAllSelector).is(":checked");
      console.log("MultiSelect: select all was clicked. isChecked: " + isChecked);
      return this.toggleChecked(isChecked);
    }).bind(this));
  };

  MultiSelect.prototype.setChecked = function(value) {
    console.log("MultiSelect: setChecked(" + value + ")");
    return $(this.options.checkboxSelector).prop("checked", value);
  };

  MultiSelect.prototype.show = function() {
    console.log("MultiSelect: show()");
    return $(this.options.onSelector).show();
  };

  MultiSelect.prototype.toggleChecked = function(isChecked) {
    if (isChecked) {
      return this.setChecked(true);
    } else {
      return this.setChecked(false);
    }
  };

  return MultiSelect;
})();

We’re up to 51 lines, mostly for a lot of closing } and junk like that. Also, since CoffeeScript generates prototypical classes, there’s a whole lot of MultiSelect.prototype.* going on. Look at all those times you have to type out the work function. That’s fun in a functional language. Finally, my friendly comments were stripped out of the compiled CoffeeScript. Hey! I put those comments there for a reason!

The Hate

The hate boils down to two simple pieces.

  1. Just write better JavaScript.
  2. Debugging is a bitch.

As mentioned above, CoffeeScript just generates JavaScript. CoffeeScript doesn’t run anywhere. Debugging goes something like this:

  1. Get a JavaScript error on line ##.
  2. Look at the compiled JavaScript.
  3. Find that line in CoffeeScript that generated line ##, since you’re all but guaranteed that it won’t be the same.

The Conclusion

I write better JavaScript because I know CoffeeScript. Seeing and reading good JavaScript output has been incredibly beneficial for me. There are plenty of bad JavaScript examples out there. I long for languages like CoffeeScript, where conciseness and clarity are first-class citizens.

Solving the Problem Isn’t Enough

I found myself saying the following way too often in the past three weeks. So, I’m putting it on my blog. Maybe someone will read it, and I won’t have to have this conversation in the future.

  1. How do I solve the problem?

  2. How do test my solution?

  3. Is my solution written in such a way that allows for isolated and automated testing?

If you know me and my projects, it’s been a rough few weeks. I’m coming in on the tail end of a project that is late, not meeting client expectations, has a very poor quality, little quality, and no technical leadership. We’re doing the best we can in a short time to rectify the problem, but the damage is done.

Do you remember high school math class, when your teacher told you to, “Check your work?” That’s what testing is all about. The answer shouldn’t be, “We’ll run the application in production and see if we can get the right answer.” That’s unacceptable in any business environment. The good news is that we do have shared development and staging environments, but the data in these environments is constantly changing as other developers and testers work. No, the right answer must include test isolation. How do you verify your solution is correct is a sustainable, repeatable way, where the work of other developers won’t affect your ability to produce a desired result?

Solving the problem is not enough.

Manually testing the solution to the problem is not enough.

Your solution must allow for isolated and automated testing. If it doesn’t, go back to the drawing board. You’re not done working the problem.