Act on your smelly code quickly before it bites you!

By September 18, 2013 No Comments

This post uses a recent discussion we had in CookiesHQ as an example. It is one of those “but why change it if it works!?” situations that you can find in an early stage application code.

The problem

This is the logic we are working with

  • A product belongs to a category.
  • A product belongs to a vendor.
  • A product can be active or not (active boolean).
  • A category can be active or not (active boolean).
  • A vendor can be active or not (active boolean).
  • In order to be active, a product needs to be active and part of an active category and an active vendor.
  • We need to keep the active boolean clean as, as part of the API, a vendor can choose manually to set a product as active or not, same for a category.

Quickly scanning this, the Rails logic sounds easy

Model product

  belongs_to :category
  belongs_to :vendor

  scope :active, -> { joins(:category, :vendor).where(categories: {active: true}, vendors: {active: true}, active: true) }


The problem with this approach is that we move the logic of an active category and vendor in the product model.

It is fundamentally wrong as if our logic changes later in our application (say to be considered as active a Category need to be active:true and have an image) then we will have a hard time going back to every place where the logic has been set.

We will then end up in an unmaintainable code, hard to test, and not scalable at all.


This problem is easily solved by having a scope and a scope.

Model Category
  scope :active, -> { where(active: true) }

Model Vendor
  scope :active, -> { where(active: true) }

So now we can independently test the active/unactive state of the vendor and category.

Using that logic we can now refactor or old line to:

scope :active, -> { joins(:category, :vendor).merge( true) }

The logic is now abstracted to each model.


Using Rails 4+ where.not the .inactive version of the product is now super easy

Model Product
  scope :inactive, -> { where.not(id: }

Nothing shown here is rocket science, but it is just a reminder for everyone writing code: when you smell that something is wrong, stop immediately and refactor straight away.

The logic showed in this post is now easy to test, reliable and scalable. All this in just 10 minutes. We love the quick wins!