Click here to monitor SSC
  • Av rating:
  • Total votes: 33
  • Total comments: 17
Nick Harrison

Code Deodorants for Code Smells

09 July 2009

Code Smells have become an established way of talking about indications that things may be wrong with your code. Nick Harrison extends the idea with the concept of 'code deodorants' and shows how the code smell of 'inappropriate intimacy' can be cured by means of  the code deodorant called 'separation by interface'.

Review of Smells

The concept of ‘Code smells’ was popularized by Kent Beck and Martin Fowler in the book ‘Refactoring: Improving the Design of Existing Code’ (ISBN 978-0201485677).  A Code Smell is an indication that something may be wrong in your code.  Code Smells are popular because they avoid much of the dogmatic nature of Code Analysis while injecting a sense of humor into what could otherwise be a very dry and adversarial process.

Code smells do not always indicate Code problems.  These are not hard and fast rules, more of an indicator that there may be a problem.   Smells should be investigated in the early stages before any chance of them  getting out of hand.

Using a Code Deodorant

You may be the last one in a team to realize that your code smelled.  It is a difficult thing to communicate. It is very delicate matter to have to tell a friend or colleague that their code has a freshness problem.  It is also embarrassing to be told that your code leaves a foul stench when it is run through the compiler. 

It is far better to apply ‘code deodorant’ to the code yourself before it gets too far.  ‘Code Deodorant’ is a term I’ll use for the simple steps that we can take to ensure that smells do not crop up in our code unexpectedly.  These steps won’t prevent smells from ever forming but they will slow them down so that you can have lead time to take your code back to the showers.

Separation by Interface

Separation by interfaces is a deodorant intended to prevent a broad range of smells that can rise from the way objects interact with each other.  The basic process is to separate objects by a well-defined interface and direct all interactions through these interfaces.

Properly structured, these interfaces should add minimal overhead to the implementation.  Once implemented, they should simplify ongoing maintenance.  If these two goals are not being met, we need to review the best practices listed below.

Interface Best Practices

These interfaces need to have several key characteristics:

  • They need to be easily implemented
  • They need to use the simplest data types available
  • They need to not tie directly to a presentation model
  • They need to not tie directly to a specific deployment model

 

There is also a golden rule when dealing with interfaces.   Don't modify a deployed interface.  If new members are needed, derive a new interface and add the new members there.  

 We want these interfaces to be easily implemented.  Implementing them should not add significant implementation overhead to the project.  This is more selfish than anything else.   If the interface is not easy to implement, it will not likely be implemented and our best designs will be for nothing.

Part of making the interfaces easy to implement requires using the simplest data types available.  For example, don't use a sub class when the base class will work.   Also, don't use a concrete type when an existing interface is available.  This widens the net for potential objects to implement our interfaces without having to make substantial changes.   It also highlights the prospects for re-usability.

The interfaces can be greatly  simplified if you  do not tie them to specific UI or deployment models.   This also makes it easier to reuse the interfaces in different situations.  Looking solely at an Interface, we should not know that it is being used in a Web Application or a WinForm application.  We should not be able to tell whether it is a SharePoint application or a DNN application.  We should not be able to tell whether the data is stored in SQL Server, Oracle, or web services.  If the interface reveals any of these details then the interface is limited to those details.

It is much easier to implement an Interface requiring a string than one requiring a TextBox.  It is much easier to implement an Interface requiring a List<BusinessEntities> than one requiring a SQLDataReader.

A Bad Smell: Inappropriate Intimacy

One of the main tenants of object-oriented programming is encapsulation.  This means that the implementation details are hidden within the definition of the object.  When objects are properly encapsulated, the system as a whole is more resilient to change.  When objects violate encapsulation, they smell of inappropriate intimacy, and the system becomes more difficult to change.  Problems in one object will more easily propagate to other objects throughout the system and changes in one object may require changes in other objects.

Inappropriate intimacy means that one object knows more about another object than it should.  For the sake of your application’s stability, you need to be a prude where your objects are concerned and restrict how they interact.

What Does It Look Like

An easy example of inappropriate intimacy happens with controls in the UI.  Whenever a control knows what page it is on, it smells of inappropriate intimacy.  If a window knows how any of its controls store or manipulate its data, it smells of inappropriate intimacy. 

If the UI knows that the data access layer uses Data Sets, then the UI is too intimate with the data access layer.

If the Business Layer knows that the UI is targeting SharePoint, then the Business Layer is too intimate with the UI.

None of these are bad design decisions.  The problem comes when other components start relying on these design decision not changing. 

Why Do We Care

Inappropriate intimacy results in applications that are more unstable, are less reusable, and are more likely to have changes in one part of the system impact another part of the system.

When a control knows that it is on a specific page, it cannot easily be moved to another page.  If a window uses the knowledge that the Address control stores the city in a text box called txtCity, the window will need to be modified if the control changes how it stores the information.

If the UI knows that the data access layer uses DataSets, then the data access layer cannot change without potentially making changes throughout the UI.  We also cannot test the UI or run the UI without a connection to the database to populate the DataSets.

If the Business Layer knows that the UI is targeting SharePoint then the UI cannot be re-targeted to any other platform without potentially needing to rewrite the Business Layer.

In each of these cases, the inappropriate knowledge makes the system brittle.  Simple changes create breaking changes.  Objects that should be reusable are not because they require that this intimate knowledge not change and continue to be true.

Deodorising the code smell of inappropriate intimacy

Inappropriate intimacy can be solved with appropriate use of Interfaces.  As long as two objects interact with each other strictly through published interfaces, there is reduced risk of Inappropriate Intimacy.  I say “Appropriate Use of Interfaces” because it is true that the Interfaces can be misused and abused, but for our purposes, we are going to assume that we will follow our best practices in defining our interfaces.

Consider our examples from earlier.  It is not OK for a control to know that it is on page MainDataEntry.aspx.  If a control must know anything about its containing page, the most it should know is that it implements a given interface.  In nearly all cases, it is better for the control to be completely ignorant of the containing page and instead require the containing page to pass data by calling methods or setting properties defined in the interface.

Separation by Interfaces will require the user control to implement a well defined interface in line with our best practices defined earlier.

If the User Control prompts for basic address information, the page should not know what controls are used for the input.  The User Control should implement an interface similar to:

public interface IAddressEditor

{

    string AddressLine1 { get; set; }

    string AddressLine2 {get;set;}

    string City { get; set; }

    string State { get; set; }

    string Zip { get; set; }

    bool IsPrimary { get; set; }

    bool IsMailing { get; set; }

    void DataBind();

    void Validate();

}

 

 As long as the containing page limits the interaction to such an interface, the control is free to change the implementation.  The control can decide to use a drop down to prompt for the state, or the control could use a text box.  The containing page or any consumer of the interface can remain oblivious to these changes.  If the users of the interface make no assumptions as to how this information is stored and used than these two components can be changed without affecting each other.  This will improve overall system stability and will allow the user control to be used anywhere that the IAddressEditor is expected.  As an added bonus, as long as the containing page stays well behaved, it can use any object implementing the IAddressEditor not just the original control.

Whenever the UI is familiar enough with your data access strategy to know which database platform is storing your data, your objects are too intimate and need to be separated.  UI logic or Business logic interacting with a data set or data reader directly makes the logic too intimate with the physical data model. Both of these are disasters waiting to happen.  As a general rule, nothing from the System.Data namespaces should ever be exposed beyond the data access layer.

There are an abundance of relational mapping software and strategies available.  There is no reason for any application logic to ever be so intimate with the data model that it directly manipulates a dataset. 

Separation by Interfaces will require that we define an interface for each entity in our domain model.  These interfaces will require a read write property for each field in the underling database  object. A simple basic example may include a physical data entity similar to:

For a variety of reasons, such entities will often not conform to our naming conventions.  We want to keep the application domain entities loosely coupled to the physical data entities so that we do not have to perpetuate the naming conventions.  We also want to make it easier to change the database without having to make substantial changes to our application. 

We can keep our domain objects and physical data objects separate through any mapping strategy that we want to use.  To meet the requirements or Separation By Interfaces, we will define an interface that our domain objects will implement:

 

public interface IMember

{

    string FirstName { get;set;}

    string LastName { get;set;}

    string AddressLine1 { get;set;}

    string City { get;set;}

    string State { get;set;}

    string Zip { get;set;}

    DateTime DateFirstJoined { get;set;}

    int MembershipLevel { get;set;}

    bool BusinessIndicator { get;set;}

    int ColdCallRotation { get;set;}

}

Our domain object will implement this interface.  The implementation of the domain object can handle the mapping from the data object.  As long as our code is well behaved and operates exclusively in terms of this interface, then any object implementing this interface can be used without affecting our application logic.

Because there is nothing in the interface to directly tie the domain object back to the original data object, no code that uses the interface needs to be modified should the original data object change. Because there is nothing in the interface to directly tie the domain objet to a given database platform, no code that uses the interface needs to be modified if the database platform change. There is not even anything to limit you to a database at all.  Maybe you decide to use web services, or maybe you decide to use XML files  Such interfaces give you the ability to structure your application logic in such a way that you don’t have to modify any business logic even for such dramatic changes.

If the only thing that you are expecting to come from the data access layer is objects implementing such simple interfaces, you are not even dependent on the data access layer being in place before you started using its artifacts.  Anything that can implement these interfaces will allow you to start developing and testing your business logic.

Conclusion

Despite our best intentions, bad things happen to good code.  Even the best code may eventually start to smell over time.  There are easy precautions that we can make in our designs to slow these smells down.  Such Code Deodorant won't eliminate all smells, but deodorant will give us more lead time to deal with problems before they get out of hand.

Separation by Interfaces can be a useful deodorant that provides guidance on how to structure our code to limit the impacts of Inappropriate Intimacy and improve the way our objects interact with each other.

Nick Harrison

Author profile:

Nick Harrison is a Software Architect and .NET advocate in Columbia, SC. Nick has over 14 years experience in software developing, starting with Unix system programming and then progressing to the DotNet platform. You can read his blog as www.geekswithblogs.net/nharrison

Search for other articles by Nick Harrison

Rate this article:   Avg rating: from a total of 33 votes.


Poor

OK

Good

Great

Must read
Have Your Say
Do you have an opinion on this article? Then add your comment below:
You must be logged in to post to this forum

Click here to log in.


Subject: Typo, methinks...
Posted by: Carl White (not signed in)
Posted on: Monday, July 13, 2009 at 4:14 AM
Message: In the paragraph headed 'A Bad Smell: Inappropriate Intimacy' the following sentence is erroneous, surely? 'One of the main tenants of object oriented programming in encapsulation' should read 'one of the main tenets... is encapsulation'.

Enjoyable article, nonetheless... thanks.

Subject: Re: Typo
Posted by: Andrew Clarke (view profile)
Posted on: Monday, July 13, 2009 at 5:06 AM
Message: Oops! Fixed. Glad you enjoyed the article.

Subject: Utter Rubbish
Posted by: Anonymous (not signed in)
Posted on: Monday, July 13, 2009 at 7:33 AM
Message: A bug is a bug, inventing new names, for old, just to make an article, pathetic.

Subject: Nicely done -- as always
Posted by: Bill Jones Jr., MVP (not signed in)
Posted on: Monday, July 13, 2009 at 9:22 AM
Message: Nick you have done it again, thanks. "Code smell" is definitely a term coming into wider use, but what you have done here is help a lot folks who think they understand OO get a much better grasp of encapsulation. Fairly simple topic, but I am amazed at the number of us who show we "don't get it" by the code we write.

Passing this link onto my team with my high recommendation.

Subject: Utter Genious
Posted by: Anonymous (not signed in)
Posted on: Monday, July 13, 2009 at 10:26 AM
Message: Inventing new names, for old, is called "symbolism", and if it is done in a way that makes it more easy to understand, is called "progress".
Any sufficiently complex topic must, at some point, be "refactored" into simpler terms to make it easier to talk about, easier to teach, and to give you simple terms (like "smell", "code intimacy", etc) that can be invoked to convey - with one/two word(s) - what used to take a whole paragraph.
"a bug is a bug" isn't just overly simplistic, it's flat out wrong. They aren't describing a "bug", they are describing a progression of bad decisions that ultimately lead to a hard-to-maintain, error prone, non-reusable monolithic program that isn't necessarily full of bugs, but is just poorly written. They are then trying to describe how to prevent that from happening with specific practices (using interfaces well solves all most of those problems beautifully).
Maybe you didn't read it all, maybe you aren't an engineer, maybe you haven't had any coffee this morning, but whatever the cause, your comment is not only useless, but is completely illogical and false.

Subject: Utter Genious... hear, hear
Posted by: Chris Melvin (not signed in)
Posted on: Monday, July 13, 2009 at 10:58 AM
Message: Nick you have indeed done it again. A clearly written and concise article that may enlighten those new to OO. A follow-up with examples of a few refactorings would make for a great continuation of the topic.

And what a wonderful rebuttal of 'Utter Rubish'. Well said.

Subject: Lame Title
Posted by: Anonymous (not signed in)
Posted on: Monday, July 13, 2009 at 11:36 AM
Message: Why didn't you use the term 'code fart' or 'stink code'? Why does it smell and not taste? I agree w/ Utter Rubbish - we have enough euphemisms in programming. Creating yet another term is indeed pathetic.

Subject: Not Lame
Posted by: db (not signed in)
Posted on: Monday, July 13, 2009 at 5:20 PM
Message: How many times have you looked at code and said, "That code stinks." Not a very exact term, and you might be hard pressed to identify a specific line of code that's bad. But you can say generally what's bad about it-- perhaps which specific odor(s) of stinky code it is. Coding is still, and perhaps always will be, as much art as science. I defy you to identify a preexisting term of art that succinctly conveys general badness in code design as well as the idea of "code smells". "Patterns" works well for the concept of identifying how to do things 'right', and the writing in that category has *really helped* communication: I can say a bit of code is a "facade" for a subsystem, and everyone in the department instantly has a pretty good idea of what's going on there. But there haven't been specific, succinct terms of art to describe what's gone *wrong*. I think classifying these ideas into sometimes humorous little monikers will help communication, once someone writes 'the definitive book' on the subject, they way the Gang of Four did with "Design Patterns".

Perhaps the book referenced in the first paragraph will be that book.

Subject: Well written
Posted by: Benjy (not signed in)
Posted on: Tuesday, July 14, 2009 at 7:42 AM
Message: Good stuff. Liked the IAddressEditor example. "Utter rubbish's" comments are well-- utter rubbish :-). Re:"Not Lame", I agree with the sentiment, but declarations need to be factual . Unless the code violated something obvious, you cannot look at a piece of code and say it stinks and if you cant put your finger on the issue and backup your declaration, then you would be wrong to make such a statement. If , as Nick illustrates, a piece of code was written as a control for say, Sharepoint, then it depends on SPS, thats all. Doesnt make it bad. Using it outside SPS is just not possible, but you may not care to use it in that way anyway. Its a fine line between this kind of code smell and overengineering. How many times do we take winforms apps and port them to the web? Most of the time the paradigm is so different that a complete rewrite is needed, so rewriting one user control is insignificant.

Subject: Nice article, title raises concerns
Posted by: Alex Kuznetsov (view profile)
Posted on: Tuesday, July 14, 2009 at 8:30 AM
Message: I loved the article, but I am not sure if the title completely matches the content. Deodorants are designed to eliminate odor, not to fix the cause of smells. You are writing about fixing the cause of code smells, are you not?

Subject: Agree, the title raises concerns; this article is really about prefactoring
Posted by: Brad Appleton (not signed in)
Posted on: Wednesday, July 15, 2009 at 3:12 PM
Message: Hi Nick! I like the article but I agree with Alex in his comment above. Particularly since Joshua Kerievsky, one of the well known refactoring gurus (perhaps second only to Beck/Fowler) uses the term "code deodorant" in his outstanding book "Refactoring to Pattern", where he describes code "deodarant" as something that tries to cover-up a smell instead of trying to eliminate it altogether. See the exceprt at http://www.informit.com/articles/article.aspx?p=360842 where Kerievsky writes:

"Sometimes we look at code and have no idea what it does or how it works. Even if someone could stand next to us and explain the code, the next person to look at it could also be totally confused. Is it best to write a comment for such code? No. If the code isn't clear, it's an odor that needs to be removed by refactoring, not by deodorizing the code with a comment."

What you describe in your article looks more to me like what is called "Prefactoring" in the book of the same name by Ken Pugh. The premise of prefactoring is that, when you refactor you gain experience about coding/design. Gain enough experience refactoring and you begin to learn things you can do beforehand to reduce the amount of refactoring.

Taking these lessons-learned and applying them on subsequent development projects is what Ken Pugh refers to as “prefactoring”. Prefactoring guidelines emphasize things to think about before you start coding, especially interfaces, encapsulation & readability. In fact the subtitle of the book is "Extreme Abstraction, Extreme Separation, Extreme Readability")

See http://www.oreillynet.com/pub/a/network/2005/11/15/what-is-prefactoring.html

So if your desire here is to discuss something that removes smells entirely or prevents them from occurring altogether, than prefactoring would be the better term to use since "code deodorant" is already established as something that attempts to cover up a code-smell instead of eliminating it.

What you wrote about here is an example of prefactoring using one of the "extreme separation" guidelines.

Subject: Really?
Posted by: Don (view profile)
Posted on: Monday, July 20, 2009 at 7:54 AM
Message: Deodorant?

Smells?

Do we really need to keep renaming and making Yet Another Analogy for plain old code reviews and quality control?

Subject: What? Why?
Posted by: Anonymous (not signed in)
Posted on: Tuesday, July 21, 2009 at 10:32 PM
Message: Yet another waste of time in the coding world... I'm guessing people are much too influenced by the pointless stuff MS puts out...

Subject: Regarding pointless company bashing...
Posted by: Gary Varga (not signed in)
Posted on: Wednesday, July 22, 2009 at 3:24 AM
Message: As for the What? Why? comment, I think you'll find that Kent Beck and Martin Fowler have little to do with Microsoft or the "stuff MS puts out", even if the author does.

Having said that the article provides an interesting viewpoint. Personally, I think the default use of an Interface is too simplistic and would be used to validate incorrect decisions. The "interface" provided either by a Class or by an Interface can be good or poor as the author suggests, however, the choice between an Interface or a Class goes deeper than I think the author suggests as "Separation of Concerns" is not resolved by "Separation by Interface".

The article is certainly valid as it provides a good basis for discussion.

...but why do I get drawn in by people seemingly obviously trying to bait someone into an argument?

Subject: Interface or not to Interface
Posted by: Anonymous (not signed in)
Posted on: Wednesday, July 22, 2009 at 3:58 AM
Message: One thing that a lot of articles on interfaces forget to ask and mention is that if you are writing self contained code, and code that will never be used by someone outside of your department, or even business, is there a need to code to interfaces?

I do know that interfaces are extremely useful for many things - applying usercontrols dynamically to a page (e.g. IControl that has methods to build the control and apply paramters) or for being able to make code generic (e.g. IEntity that contains properties such as IsNew, IsDirty, Save, etc. allowing you to have a base class etc).

However if you are coding to interfaces all the time you need to cast backwards and forwards, and you can get tangled up and readability of code is lost "what am i actually working with here?".

Having 1 interface for each class in an API is overboard IMHO. Interfaces should be small and simple and be applicable to more than 1 object in your API. And again if you are not publishing your code for anyone to use, then iterfaces might be more of a hinderance than a help?

Is IList<IMember> better than List<Member>? With option 1 you are limited in what you can do, unless you cast the IList to a List to be able to use the nicer .netty stuff.

And finally (you will be glad) if you are never going to change databases from SQL or even require to get your data from a webservice (or even a webservice you don't build yourself) is there a need to use interfaces so much?

Keep a level head when building you APIs. Simplicity in my opinion makes you code better rather than overcomplicating things with extras that might not really be needed.

Subject: Genious
Posted by: Anonymous (not signed in)
Posted on: Wednesday, July 22, 2009 at 3:59 AM
Message: Yeah genius!

Subject: Not a magic bullet
Posted by: Anonymous (not signed in)
Posted on: Wednesday, July 22, 2009 at 7:15 AM
Message: While I can see what Nick is getting at here, I agree with a couple of the posters above - interfaces are not a magic bullet, not even for implementing separation of concerns.

A knee jerk reaction to use interfaces is as bad as not using them at all.

Finally, I know myself how hard it is to create small, useful examples in articles and presentations, but I don't like the example here, as it would actually be better without interfaces.

 

Top Rated

Acceptance Testing with FitNesse: Multiplicities and Comparisons
 FitNesse is one of the most popular tools for unit testing since it is designed with a Wiki-style... Read more...

Acceptance Testing with FitNesse: Symbols, Variables and Code-behind Styles
 Although FitNesse can be used as a generic automated testing tool for both applications and databases,... Read more...

Acceptance Testing with FitNesse: Documentation and Infrastructure
 FitNesse is a popular general-purpose wiki-based framework for writing acceptance tests for software... Read more...

TortoiseSVN and Subversion Cookbook Part 11: Subversion and Oracle
 It is only recently that the tools have existed to make source-control easy for database developers.... Read more...

TortoiseSVN and Subversion Cookbook Part 10: Extending the reach of Subversion
 Subversion provides a good way of source-controlling a database, but many operations are best done from... Read more...

Most Viewed

A Complete URL Rewriting Solution for ASP.NET 2.0
 Ever wondered whether it's possible to create neater URLS, free of bulky Query String parameters?... Read more...

Visual Studio Setup - projects and custom actions
 This article describes the kinds of custom actions that can be used in your Visual Studio setup project. Read more...

.NET Application Architecture: the Data Access Layer
 Find out how to design a robust data access layer for your .NET applications. Read more...

Calling Cross Domain Web Services in AJAX
 The latest craze for mashups involves making cross-domain calls to Web Services from APIs made publicly... Read more...

Web Parts in ASP.NET 2.0
 Most Web Parts implementations allow users to create a single portal page where they can personalize... Read more...

Why Join

Over 400,000 Microsoft professionals subscribe to the Simple-Talk technical journal. Join today, it's fast, simple, free and secure.