Tired of looking for errors in log files? Use OneTrueError - Automated exception management in .NET.

Protect your data!

The introduction of OR/M libraries and strongly typed views/view models has made a lot of programmers to switch to the dark side. Let’s fight this evil!

What evil am I talking about? Public setters in your domain/business model (or whatever you like to call them). Public setters allows anyone at anytime modify your domain model data. For instance, when you create an item it’s probably the repository and the controller (MVC) / VM (WPF) that modifies it. If you have a windows service who does background processing it will probably modify it. If you allow the user to edit the model through a GUI you have a third place which can modify the model. All those places get two responsibilities since the domain model itself have no control over it’s data it. They have to take care of their own responsibility and to make sure that the domain model is in a consistent state. imho that’s not a very bright idea. How could anyone else than the domain model itself know which properties should be specified and when?

Just look at this terrible domain model:

public class TodoTask
{
	public string Id { get; set; }
	public string Title { get; set; }
	public string Description { get; set; }

	public DateTime CreatedAt { get; set; }
	public User CreatedBy { get; set; }

	public DateTime UpdatedAt { get; set; }
	public User UpdatedBy { get; set; }

	public bool IsCompleted { get; set; }
	public bool CompletedAt { get; set; }
	
	public DateTime AssignedAt { get; set; }
	public User AssignedTo { get; set; }

	public List<UserHistoryEntry> UserHistory { get; set; }
}

public class UserHistoryEntry
{
	public DateTime StartedAt { get; set; }
	public DateTime EndedAt { get; set; }
	public User User { get; set; }
}

aawwww. That’s horrible. I can barely look at it. Give me a minute..

(One hour later)

Ok. So what’s wrong with that model? It’s not responsible of it’s own data. Any class that uses it can change it’s content without any kind of validation.

Here are a sample method to illustrate the problem:

public class TodoController : Controller
{
	// Transactional = action attribute by me: Commit to db if not exceptions & model is valid
	[HttpPost, Transactional] 
	public ActionResult Edit(TodoModel model)
	{
		var dbEntity = _repository.Get(model.Id) 
		
		if (dbEntity.IsCompleted != model.IsCompleted)
		{
			if (!model.IsCompleted)
			{
				// Changed from completed to not completed
				// error?
			}
			
			dbEntity.IsCompleted = true;
			dbEntity.CompletedAt = DateTime.Now;
			
			foreach (var user in dbEntity.UserHistory)
			{
				_emailService.Send(user.Email, "Item completed", dbEntity.AssignedTo + " has completed item " + dbEntity.title);
			}
		}
		
		if (model.AssignedTo != dbEntity.AssignedTo)
		{
			dbEntity.AssignedTo = model.AssignedTo;
			dbEntity.AssignedAt  = model.AssignedAt.

			foreach (var user in dbEntity.UserHistory)
			{
				_emailService.Send(user.Email, "Item (re)assigned", dbEntity.AssignedTo + " has been assigned to " + model.AssignedTo);
			}
		}
		
		_repository.Save(dbEntity);
	}
	
}

Take a moment and look at the code. We have wipped up a view used the tooling in Visual Studio (using a strongly typed view model, edit template). Since we allow the user to edit most fields we need to include different checks for different states and act accordingly. (It would be slightly better if that code existed in your service class instead of in the controller/VM.)

Let’s fast forward one year. You now have a problem with the todo items. For some reason the CompletedAt date is not saved when it should. You therefore add a new check each time a todo item is saved in the repository:

You check if CompletedAt is set if IsCompleted is set, if not you assign it.

That is a workaround. Those kind of workarounds is almost always present in applications that expose all model data to everything in the application. It do not matter if you have a TodoService class which should handle everything for the todo items, since all other code can “fix” the model info too. Hence you WILL get those workarounds sooner or later.

Protect your data

Let’s look at the cure. In fact, it’s very simple. Make all setters private:

public class TodoTask
{
	public string Id { get; private set; }
	public string Title { get; private set; }
	public string Description { get; private set; }

	public DateTime CreatedAt { get; private set; }
	public User CreatedBy { get; private set; }

	public DateTime UpdatedAt { get; private set; }
	public User UpdatedBy { get; private set; }

	public bool IsCompleted { get; private set; }
	public bool CompletedAt { get; private set; }
	
	public DateTime AssignedAt { get; private set; }
	public User AssignedTo { get; private set; }

	public IEnumerable<UserHistoryEntry> UserHistory { get; private set; }
}

Notice that I’ve changed UserHistory to IEnumerable. That’s so that we don’t break Law of Demeter (and loose control over the history changes). The todo item data is now protected. But we can’t change anything now. And we can’t build a GUI. The solution is simple. We do not just want to change data. We want to take some action! Let’s add some methods so that we can do that.

public class TodoTask
{
	public TodoTask(User creator, string title, string description)
	{
		// insert argument exceptions here.
		
		CreatedBy = creator;
		CreatedAt = DateTime.Now;

		UpdatedBy = creator;
		UpdatedAt = DateTime.Now;

		Title = title;
		Description = description;
	}
	
	// [... all the properties ..]
		
	public void Assign(User user)
	{
		// insert argument exception here
		
		UserHistory.Add(new UserHistoryEntry(AssignedTo, AssignedAt, DateTime.Now));
		AssignedTo = user;
		AssignedAt = DateTime.Now;
		
		UpdatedAt = DateTime.Now;
		UpdatedBy = Thread.CurrentPrincipal.Identity;
		
		// hooked by the email dispatcher
		// the event architecture is provided by Griffin.Container
		DomainEvent.Dispatch(new ItemAssigned(this));
	}
	
	public void Complete()
	{
		if (IsCompleted)
			throw new InvalidOperationException("Item has already been completed");
			
		IsCompleted = true;
		CompletedAt = DateTime.Now;
		
		UpdatedAt = DateTime.Now;
		UpdatedBy = Thread.CurrentPrincipal.Identity;
			
		DomainEvent.Dispatch(new ItemCompleted(this));
	}
	
	public void Update(string title, string description)
	{
		// insert argument exceptions here
		
		UpdatedAt = DateTime.Now;
		UpdatedBy = Thread.CurrentPrincipal.Identity;
		Title = title;
		Description = description;
		
		DomainEvent.Dispatch(new ItemUpdated(this));
	}
	
	
}

Constructor

I use constructors to force mandatory data. By doing so I make sure that the item is never in an inconsistent state. It also makes it easy to spot what kind of information each model requires.

Methods

Next I added two methods for the two actions which are possible to do with our todo item. We have now protected the data and do not have to rely on that all dependent classes would have changed the properties correctly. Our todo code is therefore a lot less error prone than before.

Have it as a rule to create a method each time more than one field/property have to be changed to get a consistent state.

Exceptions

Notice that I also validate state in each method. For instance, it should not be possible to set an item as completed if it has already been completed. Those checks are more important than you think.

It can be hard to resist to remove checks like that. You probably think “It’s not so bad that an already completed item get’s set as completed again, what can possbible happen?”. That do not matter. If you start thinking like that, where do you end? It IS a bug. Better to have a zero tolerance policy. Fix all bugs, no matter how small or unimportant they are.

Summary

Since we got full control over our data, it’s quite easy to make sure that it stays that way. Unit tests becomes more straighforward as it’s clear what each method do and what it changes. I tend to always nag about unit tests: Classes which are hard to unit test are often a likely candidate for refactoring.

As we got our methods, it’s also easier to publish events for the actions (pub/sub, you can use Griffin.Container for that). Those events can be used to trigger other actions in the system, which in turn gives us a more loosely coupled application.

Encapsulation is important. That’s because it is very hard to find bugs which is created thanks to that the wrong combination of properties have been set. Those bugs tend to surface later on. It might be a week or a year later. That’s why most bug fixes for that is really workarounds (you can’t find the bug source, so you just “correct” the model information when you can).

Properly layered application (all layers may or may not exist in the same project, I prefer two projects from start, one UI and one for DL/BL):

UI design

If you follow me on twitter you have probably read a somewhat cryptic message a couple of weeks ago. It said something like “make all setters private and see what it do to your UI”. The point was that if you make all setters private, it forces you to use methods. It also effectivly prevents you from creating CRUD applications (ooohh, I really REALLY hate those).

Your users might think that they want a simple CRUD app. But really. They don’t. My current client are using a web based application to be able to follow ISO2000 (a small part of it is a case management system). The edit case screen for it looks like this:


(yes, the image is from a real production system.)

The point is that approximatly 20% of the fields are used when creating the case. 10% or less is used for each state change (the case can have 10 different states). There are research which shows that presenting too much information makes it harder for the user to decide what to do. Which in turn makes the user subconsciously hostile to using the application (my own subjective conclusion based on experience). Have you ever heard “Less is more”? It’s very true when it comes to UI design.

If you are creating a native application you should not have a problem with only displaying relevant information. If you are coding web applications, use Ajax/JSON to load partial information when you need it. A great way to design an UI is to stop focusing on the information, but on the actions instead. Don’t ask the client what kind of information he want’s to handle. A great starting question would be “When you start the application, what want you be able to do on the start page?”. You wont get an answer that says “I want to be able to specify title, description, created at and user who created the item”. But “I want to be able to create a todo item”. Don’t be tempted to ask which information the user want’s to specify, but ask what the minimal requirements are. Then take it from there.

Why am I ranting about this? Because changing to private setters actually help you to create a better UI and therefore increase the user experience. If you are using ASP.NET MVC3, try to create an edit view using your domain model. How many fields do the wizard add for you? The answer is ZERO. Isn’t that great? =)

What we should do is to create a view which corresponds to each method in the model. Only specify the fields which the method takes as it’s arguments. Voila, you have now got a lot more user friendly UI.

Let’s look at the original todo item edit page:


(again, it’s a simple model)

We can’t build our UI in the same way anymore, since our methods effectivly prevents us from having a lot of edit fields. Hence we have to scrap the old edit page and refactor our UI a bit. The details page will now get some buttons instead.

And when we press for instance the assign button we get:

Isn’t that more straight forward than before?

ORM

How do you handle the ORM them? First of all: The OR/Ms responsibility is persistance only. The information stored in the database doesn’t have to have a 1-1 relation with your domain model. You can for instance have normalized or denormalized the information so that it’s either stored more effeciently or to make it easier to query the data source. The point is that the entity that your OR/M uses isn’t always an exact copy of your model.

I do in 99% cases use repostiory classes even if I’m using an OR/M. It’s so that I can refactor the data source (for instance denormalize it) or refactor the models without having affect depending code. (And to be able to unit test all code that loads/store data to/in the data source). If you truly want to create layered applications, you have to make sure that the abstraction between the layers are intact. It will force you to write more code. That’s no way around that. But the code is more losely coupled, which means that it’s easier to maintain and extend it in the future.

However, nhibernate is quite flexible when it comes to the mapping. It can handle properties with private setters. Which means that I can in many cases use the model as the database entity too. Since I use repositores it doesn’t really matter. I might start with a 1-1 mapping but later switch to db entities if the code diverge later on.

Summary

Going back to one of the fundemantals (encapsulation) of object oriented programming doesn’t just make our code less error prone. It also forces us to think again about how we design the applications and the UI. Don’t just be a code monkey using the tools provided. Think about what those tools do to your application. Their end result might not always be a good thing.

This entry was posted in Architecture and tagged . Bookmark the permalink.
  • http://blog.bergdaniel.se Daniel Berg

    Awesome article, Jonas.

    Setting all set-accessors to private might leave you with an anti-pattern though if you set your default constructor as private and use a constructor that should contain all the required parameters.

    A in idea would be to use either the essence- or the fluent interface pattern.

    Keep up the great posts!

    • http://www.gauffin.org jgauffin

      The blog entry only targets the domain/business model. Consider it a guidance that should try to follow, and not a law that you should blindly follow ;)

      The only time you would want to be able to specify all data is when you persist the model from the data source. And a mapper can help you there. Most mappers can copy information to the model without the model having public setters.

      It’s nothing that you should do everywhere, but imho perfectly OK at the layer boundaries (as long as you are sure that the copied data is in a valid state).

  • http://www.rogerharford.com Roger Harford

    I’m not sure why it would be a problem to take in a model with public setters in your controller if you use data annotations and check if the model state is valid. If it isn’t valid you can just re-show the page.

    Also I never accept data objects as a controller parameter, I always do that work myself after validation passes on my “ViewModel” objects, so this doesn’t seem like an issue either.

    As long as you make sure that you only have one central place that can access the setters on the actual data objects then I wouldn’t care what type of access was granted on the model objects that get passed around between layers because they are only for the benefit of the consumer (model binding, the view, services, etc).

    • http://www.gauffin.org jgauffin

      It might not look like it matters if you just think of assigning property to property in one single method.

      Most models requires that you set a combination of properties. Not setting one of the properties may result in an inconsistent state. With that in mind, having public setters requires that all actions in all of your controllers set the correct combination of properties.

      Using a method in the model (and protected/private setters) moves the control to a single location: The method in the model.

      You can of course put that control in another single point. But in my experience that works only for a while (since all setters are public). When the application grows you’ll start to edit the model in other places too (but not be intentional, can be a small quick fix etc etc).

  • http://www.cuttingedge.it/blogs/steven Steven

    What I don’t get is why you use some sort of Service Locator (the DomainEvent class) to publish events? Mark Seemann writes about this: http://blog.ploeh.dk/2011/09/19/MessageDispatchingWithoutServiceLocation.aspx.

    • http://www.gauffin.org jgauffin

      I don’t think that Service Location is an anti-pattern for everything. It do have it’s usages. imho the DomainEvent class is infrastructure, not a dependency. The classes work fine without it.

      And the service location pattern allow me to reduce the complexity of the publisher.

      Why do you think that it’s wrong in this case?

  • http://www.cuttingedge.it/blogs/steven Steven

    It hinders testability. How do you abstract the DomainEvent class when testing? For instance when using a test runner that runs tests in parallel (such as MSTest)? You’ll need some sort of thread-static implementation under the covers for testing and need to reset or reinitialize it for each test. This complicates unit testing.

    • http://www.gauffin.org jgauffin

      Have you looked at the source code or are you just generalizing? (hint: Look at the Assign method in the class)

      • http://www.cuttingedge.it/blogs/steven Steven

        Could you show a unit test for the ToDoTask.Assign method? I’m interested to see how you abstract this DomainEvent.

        • http://www.gauffin.org jgauffin

          Not tested, just coded:

          public class TestableDomainEvents : DomainEvent
          {
          	[ThreadStatic]
          	private static List<object> _events = new List<Object>();
          	
          	private static TestableDomainEvents _instance;
          	
          	public static TestableDomainEvents Current 
          	{ 
          		get 
          		{ 
          			if (_instance == null)
          			{
          				_instance = new TestableDomainEvents();
          				DomainEvent.Assign(_instance);
          			}
          			
          			return _instance;
          		}
          	}
          	
          	protected override void PublishEvent<T>(T domainEvent) where T : class
          	{
          		_events.Add(domainEvent);
          	}
          	
          	public IEnumerable<object> PublishedEvents { get {return _events; } }
          	
          	public void Clear()
          	{
          		_events.Clear();
          	}
          }
          
          public class TestTodoTask
          {
          	public void Assign()
          	{
          		TestableDomainEvents.Current.Clear();
          		var task = new TodoTask();
          		
          		task.Assign(blabla);
          		
          		Assert.True(TestableDomainEvents.Current.PublishEvents.Any(x=> x is ItemAssigned));
          	}
          }
          
          • http://www.cuttingedge.it/blogs/steven Steven

            I must admit that this test code looks quite clean (and if this is the only time in the application where you’re using this pattern, it isn’t that bad), but still you have to do ‘nasty’ thread-static stuff and special test setup (in each test) to ensure tests not influencing each other.

          • http://www.gauffin.org jgauffin

            Why do you think that [ThreadStatic] is nasty? It’s easy to understand what it does in this case.

  • HoyaBaptiste

    What you describe here makes sense. Do you have a sample project illustrating this on Codeplex, GitHub or Google Code? Can you share it? Thanks in advance.

    Carlos Baptiste
    @HoyaBaptiste

    • http://www.gauffin.org jgauffin

      No examples yet. There will probably be one though. Can’t say when. Follow me on twitter @jgauffin to find out.

      • HoyaBaptiste

        Thanks, Jonas. I will.

  • Patrick

    Nice article, it makes a lof sence. I wonder though: are you always against public setters, even when truely unrelated? For example Car.NumDoors and Car.NumWheels? Thanks.

    • http://www.gauffin.org jgauffin

      Really good question. The little anal me want’s to scream YES! Since changing it later on will force you to refactor every place which uses it (violating Open/Closed principle).

      But the more realistic me says no. It’s ok. Refactoring is a part of the process. Change it later into a private setter if you find that a related property have to be changed too. However, public setters are as some big dirty rats. Once you have let a few in the’ll start to multiple like horny rabbits. Before you know it, they’re all over the place.

      Consensus: Try to use methods first. If it’s not possible, sneak a property in.

  • Patrick

    Okay i understand. So you basically assume that at one time a new property gets introduced, let’s say Car.Weight, which depends on the number of wheels? (Just as an example).

    • Patrick

      Ah, i should have ‘replied’. sorry for messing up the spacing :)

      • http://www.gauffin.org jgauffin

        No problem, the “Leave a reply” text box and the bottom fools me to.

    • http://www.gauffin.org jgauffin

      I’m saying that it could happen, and they should then be grouped with a method if it do. Which would require you to refactor the application.

      Properties tend to either force you to violate Open/Closed principle or to violate encapsulation.

  • Patrick

    Rethinking this post, for simple pairs of properties, you could mark one of them as ‘master’ and make the other private, as you suggest. For example:

    obj.Log = true;
    obj.LogFile = @”c:logslog.txt”;

    If we mark LogFile as the master, we could change its property to:

    // uses old-style member variable
    public string LogFile
    {
    _logFile = value;
    // call private setter
    Log = (!string.IsNullOrEmpty(_logFile));
    get { return _logFile; }
    }

    Thus don’t call Log ourselves anymore.

    Would that make sense?

    • http://www.gauffin.org jgauffin

      Yes. That’s how I would do it. The state depends on that the logger is correctly configured. Hence the state should only be set by the logger itself.

      You could otherwise have a correctly configured logger which is marked as not configured since the caller did not follow the rules.

  • Michael Starke

    How do you reconcile private setters with two-way databinding in Silverlight/WPF which requires accessible setters?

    • http://www.gauffin.org jgauffin

      Your view models are not your domain model. It’s fully possible to write task based applications with WPF too.

      • Alireza

        Brilliant answer!

  • Pingback: Why Domain Driven Design (DDD) is so great « jgauffin's coding den

  • Roger

    I would argue that you are 50% “correct”. Yes, (public) setters are evil, but so are getters.
    Most of your (valid) points are applicable to getters as well. You mention that you’re using IEnumerable to follow Law of Dementer but the same problem you have with all your non primitive getters, eg todoTask.AssignedTo.Name.FirstName…..

    Encapsulation isn’t only about protecting how the data is set but how the data is accessed generally (inluding reads). Even though many .net and Java programmers seems to believe that encapsulation means wrapping the data in accessors/properties for reads and writes, this is IMHO not true. It’s about not exposing it’s internal state, neither for writes nor for reads.

    I don’t say it’s easy or even good to always avoid getters, but trying to avoid them as well is a good thing.

    • http://www.gauffin.org jgauffin

      Absolutely. But as you say it’s a lot harder than the setters since you most often need to get the information at some point.