Finding Design Smells In Non-Design Related Places

In my last post, I talked about the idea of encapsulation and using it to ensure that our business rules were enforced correctly. What I didn’t talk about, though, was the second half of the conversation that my coworker and I had, concerning the patent -> consultation relationship. It turns out that we had the relationship wrong. That’s not to say that patients don’t have consultations, but that the logical model we were traveling with had an incorrect perspective and was causing us to create some very ugly workarounds in various parts of the system. What really stuck out in my mind, though, was not the idea that we had the model wrong, but how we came to the conclusion of the model being wrong. It has become apparent to me, upon reflection of the conversations and situation as a whole, that design smells are not always evidenced by design related activities, if ever.

A Persistent Problem – Duplicate Redundancy

After some initial coding of the patient -> consultation relationship, we start working on the persistence model via NHibernate. What we have is a patient with a collection of consultations – this is easy to map with NHibernate’s one-to-many capabilities. We also have a CurrentConsultation property which needs to be mapped. This property is mapped to the same Consultation table, but only pull one specific consultation based on the business rules that state the current consultation is chronologically the most recent and has not ending date set.

After some thought, we found that there were a few possibilities for handling the CurrentConsultation property in our current model:

  1. Create a “CurrentConsultation” object that is mapped to the Consultation table and use a “where” class attribute in the NHibernate mapping that would limit the returned result
  2. Create a “CurrentConsultation” object that is mapped to a CurrentConsultation view and have the view coded to return the correct consultation object
  3. Add a CurrentConsultationId field to the Patient table, as a foreign key to the Consultations table, and map to the existing Consultation object

After some additional thought, though, we found that each of these solutions has a few significant problems that were going to cause a lot of trouble.

Options 1 and 2

Both of these options have the problem of duplicating business rules into more than one language and location. We would either have the business rules of what constitutes the current consultation in the NHibernate mapping (the ‘where’ attribute) or in a database view, in addition to the already existing code. Changing the rule would mean changing a minimum of two locations where that rule is handled. This is a bad idea no matter how you look at it.

Both of the options have also created a duplication of knowledge from the concept of a Consultation by creating a “CurrentConsultation” class and a separate NHibernate map for it. We would have the original Consultation class and the new CurrentConsultation class both representing the same data, making an artificial distinction in our code. Again, this is a bad idea. We don’t want duplication of these logical concepts. We’re also not dealing with a bounded context or any other logical separation of concerns at this point, so there is no need to separate the concept of a consultation into multiple classes.

Option 3

This doesn’t appear to have the duplication issue in code, but there is a potential for duplication of data. When we get down to the implementation of NHibernate, we could easily cause duplicate data in the consultation table by saving the current consultation class. We might be able to get around this by not cascading the saves of the current consultation property, but then we’d be forced to ensure the consultation collection was persisted prior to the patient so that we could update the current consultation object’s id before saving the patient. Both of these problems sound like a serious pain to me. I’m betting it’s possible, but I’m also betting that it would be a nightmare of trial and error to get it right and a lot more code than we should really have to write.

Changing Perspectives

As Joe Ocampo pointed out in the comments of my original post, we had a problem in our system that was really caused by our lack of correct perspective in the situation. Rather than forcing the idea of a patient being the root aggregate in this situation, causing us a lot of headache and frustration in trying to model our persistence layer, a simple change in how we looked at the situation helped us solve the persistence problem and greatly simplified how the application worked.

Joe’s comment (with some formatting added):

“One thing I like to challenge developers with when I teach DDD is to flip the aggregate to determine if the model is sound.

I know this is only an example but work with me here.  You indicated you are dealing with a medical system.  We can assume there are certain entities such as Patient, Consultations, Doctor and Practice. In your example you created a model where the patient is the aggregate root for consultations but what if the Doctor simply asked what consultations do I have today?  In this paradigm the Practice is the aggregate root and Consultations are aggregate within where Patient is an aspect of the consultation.  The code would look something like this.

consultations = practiceService(IConsultationService).GetConsultationsFor(doctor);

This also allows the consultation service to encapsulate its own logic for creating a consultation for creating a consultation. You can’t get any closer than that :-)

consultationService.CreateConsultationFor(patient).with(doctor).at(date);

The point I am trying to make is be careful of aggregate roots.  Once you go down that path it is really difficult to back the train up and break it apart.”

Though our actual implementation was different, this was the same basic conclusion that we had come to – our perspective on the situation was simply wrong. When we stepped back from the problem and realized that the consultation was the primary focus of the situation, and that a nurse or doctor would be the primary user of that portion of the system, it became rather obvious that our aggregate was in dire need of rework.

A Reflective Perspective

What we ended up with was a Patient object that dealt with all of it’s demographics information, billing information, etc, without a CurrentConsultation property or even a Consultations list. Then, on the the separated Consultation object, we added a child property of Patient. Once we realized that our Consultation object was the primary focus and made this distinction in our code, we also realized that the Patient object was carrying far too much information around the system. We found that we actually had two very distinct concepts of a patient, determined by two very distinct bounded contexts.

  • In the ‘Billing’ context, we needed all of the address , billing, and other demographics information about the patient – who they are, where they live, what their insurance is, etc. The existing Patient class filled this need.
  • In the ‘Consultations’ context, we did not need anything from the Billing context, except for the person’s name and patient id. What we really care about in the consultations is medical information about the patient – their current prescriptions, allergies, past medical care, etc. So, we created a ‘ patient’ class to represent these needs.

These changed allowed for a much more clearly defined model that was truly reflective of the systems needs. We could easily see the difference between a ‘billing’ patient and a ‘medical’ patient, and we were able to code each of these areas of the system without the concerns bleeding into each other. Essentially, we decoupled the system at a module level, not just at a class level.

We also found that the NHibernate mapping problems suddenly went away. Since the Consultation class had a child of Patient, it was a simple many-to-one mapping with no strange sequencing or duplicate data issues. In the screens that deal with the consultation directly, we load the consultation as the aggregate root and go from there. In the screens that need to show patient consultation history, we did a simple query and returned all of the consultations for the given patient. Again, we found a way to decouple our system – this time, at the persistence model.

Design Smells: Not Just A Design Problem

In the end, we were able to recognize a serious design smell in our system – not by the design itself, though. After all, the original code had encapsulated the needs quite well. But, as it turns out, it was a bad encapsulation at a higher level. It wasn’t until we started working with the model we had created, specifically trying to persist the model, that we realized our design was not right.

This change was a huge breakthrough for us, not necessarily in the code or the system that was being built, but in how we look at our systems and our domain models. The realization that design smells are often evidenced not by the design itself, but by how the design is used in the infrastructure and other supporting roles of the system, has had a profound impact on how we look at system design. I’m now seeing areas of different systems that are encapsulated incorrectly, at a higher level than class design. Recognizing the problem is the first step – and we’re now working to rearrange and invert these models to more accurately reflect reality.

Pay attention to the pain that your application, infrastructure and other supporting services are causing you. You may be staring at evidence of a design problem, without realizing it.


Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Derick Bailey

Derick Bailey is an entrepreneur, problem solver (and creator? :P ), software developer, screecaster, writer, blogger, speaker and technology leader in central Texas (north of Austin). He runs SignalLeaf.com - the amazingly awesome podcast audio hosting service that everyone should be using, and WatchMeCode.net where he throws down the JavaScript gauntlets to get you up to speed. He has been a professional software developer since the late 90's, and has been writing code since the late 80's. Find me on twitter: @derickbailey, @mutedsolutions, @backbonejsclass Find me on the web: SignalLeaf, WatchMeCode, Kendo UI blog, MarionetteJS, My Github profile, On Google+.
This entry was posted in Analysis and Design, Data Access, Design Patterns, Domain Driven Design, NHibernate, Principles and Patterns, Refactoring. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.lostechies.com/members/colinjack/default.aspx colinjack

    Not arguing with the rest of it but in your options you missed an obvious one which is to pick the current consultation at run-time.

    The obvious argument is it could be slow but thats often premature optimization, been some good threads on this in the DDD forum and one piece of advice I value was that you should program as if everythings in memory. If you do that then you obviously need to test and optimize afterwards but its an approach that l’ve seen that work an absolute treat. I’d also argue that the where clause in the NHibernate mapping file is not a big problem, but its horses for courses.

    I should also say that from what I’ve seen often problems in the infrastructure and supporting roles are not really caused by design smells. Sometimes the best domain models are a pain to map if you just use straight NHibernate, what makes them great is that they elegantly solve the problem and build in real domain knowledge. My hope is that in the future NHibernate builds in more knowledge of DDD, if it did I think we’d be flying.

    That doesn’t mean I’m arguing with your design ofcourse, I totally agree with the Consultation being key here and having seperate modules in play. I just think a good domain design (including good aggregate design) has more to do with the reflective part and that whether it maps well or not is sometimes a seperate issue.

  • http://www.lostechies.com/members/derick.bailey/default.aspx derick.bailey

    “pick the current consultation at run-time”

    that was the subject of the previous post, actually. We started with this in place and found that we had an opportunity to better encapsulate the knowledge of which consultation was the current one, and moved away from that.

  • http://www.lostechies.com/members/colinjack/default.aspx colinjack

    “that was the subject of the previous post, actually. We started with this in place and found that we had an opportunity to better encapsulate the knowledge of which consultation was the current one, and moved away from that.”

    Woops, I did read it but added comment late at nught so wasn’t thinking clearly :)

    I definitely get the performance issue but the encapsulation one I don’t really follow fully. I must admit looping through collections (or using Linq) is something I’ve used quite a bit, only falling back on anything more complicated when it was needed.

    Whichever you go for though my real point was just that I find ease mapping can be a sign of a bad domain design but for complex parts of the model it can just be down to issues with the ORM or pain of impedance mismatch so you have to sniff carefully..

  • http://colinjack.lostechies.com Colin Jack

    Just realized, your Consultation is probably a good example of the Relationship Object archetype that Randy Stafford discusssed.

  • http://www.lostechies.com/members/derick.bailey/default.aspx derick.bailey

    @Colin

    “looping through collections (or using Linq) is something I’ve used quite a bit”

    same here. I really didn’t mean to say that you should never do this. I was only trying to illustrate a specific example of how some simple changes in this instance (this post, and the previous) could represent a more clearly cut encapsulation of the business rules.

    “you have to sniff carefully”

    very true. i think i was getting a little over-zealous in trying to make my point.

    “Consultation is probably a good example of the Relationship Object archetype that Randy Stafford discusssed”

    i was thinking the same thing after reading your post on making meaningful relationships. it’s good to see these posts tie into each other so well. helps validate a lot of what i’ve been thinking recently :)

  • http://colinjack.lostechies.com Colin Jack

    @Derick
    Definitely on the tie in, will add a link from my one cause this is a great example. Forgot it was the comment on your other post that made me want to finish the blog entry (otherwise it would have stayed as a draft forever). Definitely hope the pattern makes it into a future version of DDD or into the much anticipated DDD forum FAQ.

    Get you on the rest, if you’re over-zealous then so am I…if there ever is an ALT.NET manifesto I actually hope over-zealous is in it :)