Skip to content

Everything Is Smelly

January 14, 2012

I keep seeing this kind of descriptions in code and in design reviews:

// Contains all the logic for handling Products
public class ProductsService { ... }

To me, they looks like this:

Clean ALL the things!

That is, while these attempts are backed by good intentions, they are ultimately doomed to fail and get us farther from the stated goal of cleanliness.

My impression is that “all the things about X” classes are a misguided attempt at increasing cohesion: putting closely-related things together. Or put more formally:

In computer programming, cohesion is a measure of how strongly-related or focused the responsibilities of a single module are.

This Is Not Cohesion

Now, cohesion in itself is a worthy value in design, and is a major element in my “Shift-Deletable Feature” design philosophy (that’s another post for another day). And surely, “all the logic for handling products” is strongly-related: after all, it all deals with handling products. Right?

Well, not really. When we use “all”, and its close friend “everything”, in a class’s description, we usually imply a grouping together of different things, with a single common shared trait or relation between them. If that group of things was cohesive, there would be a better description for it that doesn’t use such foul language. Having to use “all” or “everything” has that familiar, sour smell of an SRP violation, which is closely related to (but not entirely the same as) lack of cohesion.

How Our Need of Structure Impairs Our Design

So why do we keep doing this? Sometimes we don’t know better (and it doesn’t help that new programmers are sometimes taught that this is a good way to structure code), but sometimes we’re misled to think it’s a good idea because it feels organized. Grouping together all the things that have to do with something, as broad as it may be, appeals to our inner nerd’s sense of organization and structure.

In real life, this behavior is justified: putting all those papers and bills and receipts and coconut-scented plastic trees together in an “all the things that relate to the car” folder makes perfect sense. This way, when we need something related to the car, we just have to look through that folder to find it.

But code doesn’t work that way. We don’t organize code just so it’ll be easy to find; we also try to make it easy to change, which is far harder. To achieve this, we need a Separation of Concerns approach, not a Grouping of Loosely-Related Stuff one. Uncle Bob’s alternative wording of the SRP comes in handy here: “A class should have one and only one reason to change”. If a class handles “all the things” that have to do with something, then by definition it has all those reasons to change.

There’s nothing inherently wrong with putting somewhat-related things in the same container; but if we really want to make it easy for us to find things and be able to change one concern without affecting others, then we need to keep on nesting smaller and smaller containers until we reach a single container per concern.

In code, those single-concern containers are classes. For everything else, we have namespaces, folders, projects, packages or whatever your language of choice provides to satisfy your inner organizational urge.

So, whenever I see “all” or “everything” used in a class’s description, I get suspicious and start asking some hard questions.

Clean ALL the things?

The comic behind this image is way better than this post, you should totally read it.

Advertisements

From → software

14 Comments
  1. In think this is often the result of a SOA architecture (ProductsService) which seems to stand on solid architectural ground (“let’s make sure we know where all the stuff related to products is so we can scale/replace/whatever it”).

    Obviously this is not an excuse to make it a “God of Products” class, but why don’t you suggest something better?

    • I’m not very experienced in SOA but I’d argue that “products” is not specific enough to be a service. Even if it were, you wouldn’t write it as a single class, would you?

      Regarding a better suggestion, I’m pretty sure I did — organize stuff into logical containers (namespaces, folders, packages, whatever) but keep breaking things down until your classes are cohesive and have a single responsibility.

    • ProductsService has little to nothing to do with SOA

  2. Tal Kedar permalink

    But how does it all relate to Zizek’s work? :)

    • Easy: while everything is smelly, we act as if it isn’t, in order to maintain normal routine; that’s the big Other that we all recognize and subscribe to by social norm. Or something like that.

  3. Ok so the alternative is a million one-public-method classes neatly organized in folders?

    • Sergey, I’m advocating having more classes with less responsibility each, yes. That doesn’t necessarily mean a million and it doesn’t necessarily mean a single public method, although these are quite common. Organizing them neatly in namespaces or folders is just good organizational common sense, and the question of how to structure these deserves a different post.

      At any rate, I see no inherent value in having multiple public methods in a class, and there’s no inherent “wrongness” in having just one. Personally, I take the most pride in classes that I can point to and say “you see this baby here? That’s this requirement from the spec translated neatly into code”. Often this will mean implementing a single-method interface.

      • Well I can think of a PhotoManager that has a public .Add(Photo photo) .Delete (…), .Tag(…), .SetPermissions(…) etc.

        Do you recommend I have something like PhotoAdder.Add(…) PhotoDeleter.Delete(…), PhotoPermissionSetter.SetPermissions(…)?

        Or is it something in between?

        • Avish permalink

          It really depends on how complex each of these are. Adding and Deleting photos can be thought of in the context of persistence (e.g. in a DAL) in which case they are perfectly fine together (the concern here is “persisting Photos”), with or without querying in the same class.
          Or you can think of adding and removing photos in the context of a “user action”, in which case they can be so totally different to be thought of as different concerns and hence different classes.

          For example: the user action of adding a photo might do some image validation (dimensions, etc.), maybe generate a thumbnail, and eventually save the photo to persistent storage. Maybe it also generates a newsfeed story about the new photo upload, or sends someone an email notification. These are all different concerns.

          Continuing this example, the user action of deleting a photo does some authorization checks to see whether the current user is allowed to delete this particular photo, which can itself be a collection of different concerns — e.g. “every user is allowed to delete a photo they uploaded”, “page admins can delete all photos on their page” and “a user who was explicitly granted a delete permission on a photo can delete it”. Since this is a potentially open set of validation rules, it might be appropriate to model them as different classes and have some higher-level class use all of them to get the final result. Getting back to the delete action, after validation succeeds it’ll need to delete the photo from the storage and possibly do some related cleanup. So we end up with a bunch of different concerns here.

          Granting and revoking permissions, and checking them where appropriate, is a different feature/concern and I would try to get it uncoupled from the persistence stuff. The same goes for tagging.

          So yes, you might end up with any or all of the following: PhotosStorage, ThumbnailGenerator, PhotoValidator, UploadPhotoAction, DeletePhotoAction, PhotoPermissionsProvider, PhotoPermissionsStorage, IPhotoDeletionAuthorizer with several implementations (OwnPhotoDeletionAuthorizer, PageAdminDeletionAuthorizer…) — and that’s just for uploading and deleting photos (not mentioning tagging and other stuff). Of course this is a relatively high-resolution breakdown, and you may certainly find that fewer classes that combine some of these concerns make more sense for your case — maybe you’ll end up with just a PhotosStorage, PhotosPermissionsProvider and AddRemovePhotoActions. That’s fine too.

          Breaking down responsibilities into classes is a spectrum, and you need to find the sweet spot on it that makes sense for you and balances cohesion and separation of concerns against over-design. However, I think one extreme side of this spectrum — namely, a single PhotosManager class that handles all of these concerns — is almost always a bad idea. In fact, anything with “Manager” in its name is a smell that indicates you might not have thought through the full set of concerns you’re trying to cover.

    • With Avish on that – As long as you have a decent navigation tool (like Resharper), I don’t see any advantage in having all the codes of a million classes in one God class. If you wanna fix that serialization bug – you can easily find the Serialize method without knowing its class.
      It is not feasible for the human brain (well mine at least) to store a Gods class code in the brain without missing something. You are better off focusing on a small class that does one thing (See http://anarchycreek.com/2009/09/08/why-more-and-smaller-classes-helps/).

  4. I often sin in favor of too little “proper design”.

    When I want something done, I code it up … and usually I don’t care a lot at the time which class hosts it. I’ll obviously try to find the best suitable class, if I already one (e.g. Complex.Multiply), but I do sometimes end up with “yet another Product method” that doesn’t have a lot to do with other Product methods.

    The above is often the case with me when I’m writing methods that don’t really rely on internal state, but just “does something with a person”. It is code smell, I agree, but I usually prefer to pause later – when I get the sense that a class is getting too big/cluttered/incoherent, and break it up to smaller pieces, than to sit down and think before every new method/class I write.

    Not saying my way is better – I admit I do occasionally have “WTF moments” when I read a class I wrote some time ago – but I guess I still prefer working this way because I feel the clutter is not too great and I don’t need to interrupt my train of thought just to place a new method.

    • I’m troubled by this. I’ve learned the hard way that reducing messiness is much harder than not introducing it in the first place, and from my experience with both my code and yours, the “cleaning phase” almost never happens and when it does it’s usually sub-optimal. That’s why it’s important to set up the environment so that things are not messy to begin with.

      I’m not saying you should think for 10 minutes before every new method, but you definitely should structure your code base in such a way that things naturally go into the right place.
      This requires some general thinking at the beginning and at certain points in the evolution of your code, but in day-to-day programming it should just be the norm. For that to happen, you need to make it especially frictionless to add and use new classes and move things between them (e.g. with a decent IoC Container and some good conventions). This in turn allows constant evolution and refactoring of the code so that it maps more naturally to your mental model of things, and is thus easier maintain and change.

      I think that for smaller projects (or large ones made of multiple loosely-coupled components) your way of programming might work, but it breaks down with any code base above a certain scope or age.

  5. Can’t argue with a man who’s seen my code…
    In my defense, I wasn’t the sole author :)

    At least I’m fairly certain you’re not a psychopath. You do know where I live though.

    http://stackoverflow.com/questions/876089/who-wrote-this-programing-saying-always-code-as-if-the-guy-who-ends-up-maintai

  6. I’m a strong believer in methodologies and having a natural design (just because it feels right) is often misleading. Design Patterns are a nice way to avoid common mistakes but they don’t enforce coding standards. Are you familiar with SOLID principles http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)?

    What I personally like about principles is that they give a different perspective to programmers. For example when you review your code you can sometimes easily see that you are breaking one or more principles which leads you to justify your choices.

    I agree with Avish, clean code is crucial. The reason why it is so important is that ‘change in requirements’ is the only thing that will never change.

Say Something

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s