Code review: Fabric class

By Vladimir Khorikov

This is the second code review session where I showcase a real-world code example. If you want to learn more about this new format, check out this post. You can also request a review yourself. To do that, use the form on this page.

Code review: Fabric class

The full source code of this one is rather large for a single blog post, so I decided to pick the most interesting portions of it – those I think could be improved.
Even after the reduction, it remains to be too large to inline here in full. So instead, I will provide excerpts while discussing different parts of it. But you can go ahead and see the full version on GitHub.Gist.

The code sample belongs to an e-commerce application for a clothes business. The part of the domain model we’ll be focusing on consists of Fabric, FabricImage, and FabricTag classes. There’s a many-to-many relationship between Fabric and FabricImage, as well as between Fabric and FabricTag. Fabric also contains pre-order information: PreOrder and Due fields.

Here’s the part of the class we’ll start with:

First off, I want to start with the things I like. The use of Maybe is one of them. This public property here:

explicitly tells the reader that the due information might not exist in a fabric. To make this work with Entity Framework, the author applies a small trick. It is not the public property itself which is mapped to the database (such mapping would fail as EF knows nothing about Maybe) but the underlying string field. The public property here acts as a proxy which converts that string to and from a Maybe. This technique brings honesty to the class’s public API which is one of the key principles of functional programming.

I also like the guard clauses in the Create static method:

They explicitly denote the class’s preconditions, and it’s a good idea to do that even if you don’t practice Design by Contract much.

Alright, let’s move on to the “could be improved” part.

First of all, note that the fields PreOrder and Due are coupled to each other. They tend to change together:

and their preconditions depend on each other’s state:

This coupling is a strong sign that we are dealing with a hidden abstraction, and it’s a good idea to introduce a separate class for it. This abstraction doesn’t have its own identity, and so it can be represented as a Value Object. Here’s how it can look like:

I’m not a fan of the name (because of the “Info” suffix) but I’m not familiar with this domain, so can’t think of anything better.

Here’s how Fabric looks like after the extraction:

Note several things here. First, the two properties the class was dealing with previously are now substituted with a single one: PreOrderInfo. And as the new class takes care of its own invariants, we don’t need to verify them in the Create method anymore. All we need to do is use the constructor, the required checks reside inside of it:

We also don’t need to take special care of the two fields while updating the entity. Instead of this:

we are able to do just this:

And I’ve removed the SetPreOrder and HasArrived methods. If we ever need their functionality in the future, we can use these two helper methods in PreOrderInfo itself:

Overall, introducing new Value Objects helps simplify host Entities. They have fewer things to worry about and thus become much easier to maintain. The Value Objects themselves are also easier to deal with because of their immutability.

Note that you might have issues with this approach if you use Entity Framework. Its support of Value Objects is limited and it’s not always possible to marry the two. However, this particular Value Object should work (didn’t test it, though). Also, note that NHibernate doesn’t have such shortcomings.

Alright, let’s look at the part of Fabric that deals with image and tag collections:

First of all, when defining a public member, it is better to use IReadOnlyList as a return collection type instead of IEnumerable. Use IEnumerable only when you specifically want to enable lazy evaluation. I wrote more about it here: IEnumerable vs IReadOnlyList. And there’s also no need in calling Skip(0). You can just use ToList, it will create a copy of the underlying collection as well.

So these two properties:

Can be refactored into this:

Not a big deal, but something that we can change quickly, so why not. The more worrying piece here is the use of protected properties. Ideally, you want the backing mutable collection to be private. Otherwise, there’s a possibility for inheriting classes to mess up with it. Unfortunately, this is the best we can get when using EF, it’s another shortcoming of this ORM. Here I wrote more about it: EF vs NH (case #3). With NHibernate, it would be just that:

Next, there’s a room for improvement in how the class handles the default image. Instead of having an IsDefault flag in FabricImage, you could introduce a new DefaultImage property in Fabric itself. Changing the default image, then, would be reduced to just updating that property. Here’s how the whole class will look like after the refactoring:

Note that I’ve removed all methods related to the work with default images: UpdateDefaultImage, GetDefaultImage, and GetDefaultImageId. They are not required anymore after we introduced the DefaultImage property. Also, note that this refactoring would require you to change the database structure too.

Last thing could be improved is how the class updates the linked objects – tags and images. Here’s the code again:

The UpdateLinkedObjects method is from the base AggregateRootWithLinkedObjects class which you can find here: initial version. The problem is that the aggregate root works directly with persistence concerns. Specifically, it marks objects as deleted or unchanged so that EF has an idea what to do about them:

By default, EF treats any object you set to a property of a newly created object as also being new and tries to insert it into the database. This, of course, fails if that object already exists, and therefore you need to manually guide EF on which object it needs to insert and which not. This entails a bigger problem – coupling domain classes to concerns that have nothing to do with the domain. Unfortunately, just as with issues I mentioned previously, there’s no way around this problem if you use EF. So the only way to actually improve this part is by switching to NHibernate.

I think I saw a discussion on EF’s GitHub page where they were talking about changing this behavior, but can’t find it now and so don’t know its current status. In any case, this does seem like a small enhancement implementation wise, and NHibernate had it from the very beginning. It’s just a simple convention: if object’s Id is not the default value (zero in most cases), then attach that object to the context and not try to insert it. Only insert the object if its Id is zero. Not sure about possible breaking changes this feature might introduce, though.


Highlights from this code review:

  • Reveal hidden abstractions and introduce Value Objects for them.
  • Use IReadOnlyList as a return collection type in place of IEnumerable in public methods.
  • Don’t rely on IsDefault flag if you want to differentiate an object in a collection.

If you want me to review your own code, send it over using the submit form on this page.

Reference list: