Growing applications : refactoring for maintainability

gistfile1.txt

# Let's pretend we're building a Rails app to handle issues (or tickets) coming in to 
# our IT department. We'll consider how the complexity grows as our app needs to accomplish
# more things. Simultaneously, we'll look at how we can keep our app maintainable
# as new features force our app to grow.

# Let's start to build our app. Consider a feature request ticket. 
# It will need to be submitted and it will need to be assigned to a developer.

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
  end
  
  def assign(developer)
    update(status: "assigned", developer: developer)
  end

end

# Now imagine we want to send out some notifications when these actions occur. 
# We'll need to build out an ActionMailer for FeatureTickets and tell it
# to send emails when these events happen. We'll also have to send out a different notification 
# when the ticket is reassigned. Let's incorporate this into our FeatureTicket class.

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
    FeatureTicketMailer.ticket_submitted(self).send
  end
  
  def assign(developer)
    update(status: "assigned", developer: developer)
    FeatureTicketMailer.developer_assigned(self).send
  end
  
  def reassign(old_developer, new_developer)
    update(developer: new_developer)
    FeatureTicketMailer.developer_reassigned(old_developer, self).send
  end

end

# Let's increase the complexity. Now we need to send out multiple emails to various 
# interested parties on each event.

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
    FeatureTicketMailer.ticket_submitted_to_IT_department(self).send
    FeatureTicketMailer.ticket_submitted_to_IT_manager(self).send
    FeatureTicketMailer.ticket_submitted_to_submitter(self).send
  end
  
  def assign(developer)
    update(status: "assigned", developer: developer)
    FeatureTicketMailer.developer_assigned_to_developer(self).send
    FeatureTicketMailer.developer_assigned_to_submitter(self).send
  end
  
  def reassign(old_developer, new_developer)
    update(developer: new_developer)
    FeatureTicketMailer.developer_reassigned_to_old_developer(old_developer, self).send
    FeatureTicketMailer.developer_reassigned_to_new_developer(old_developer, self).send
    FeatureTicketMailer.developer_reassigned_to_submitter(old_developer, self).send
    FeatureTicketMailer.developer_reassigned_to_IT_manager(old_developer, self).send
  end
end

# This is already starting to look pretty bad. The FeatureTicket class knows way too much 
# about how to send notifications. Let's make the situation even worse! Let's introduce
# an event system. That way we can record everything that's happened to a ticket.
# We'll use single table inheritance, because all events will record who did what
# to which ticket at a given time. For contextual details we'll use a details hash.

#event.rb

class Event < ActiveRecord::Base
  belongs_to :user
  belongs_to :ticket

  serialize :details, Hash

end

class FeatureSubmitted < Event

end

class FeatureAssigned < Event

end

class FeatureReassigned < Event

end

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  has_many :events
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
    Event.create(type: "FeatureSubmitted", user: submitter)
    FeatureTicketMailer.ticket_submitted_to_IT_department(self).send
    FeatureTicketMailer.ticket_submitted_to_IT_manager(self).send
    FeatureTicketMailer.ticket_submitted_to_submitter(self).send
  end
  
  def assign(developer, assigner)
    update(status: "assigned", developer: developer)
    Event.create(type: "FeatureAssigned", user: assigner, details: {developer_id: developer.id})
    FeatureTicketMailer.developer_assigned_to_developer(self).send
    FeatureTicketMailer.developer_assigned_to_submitter(self).send
  end
  
  def reassign(old_developer, new_developer, assigner)
    update(developer: new_developer)
    Event.create(type: "FeatureReassigned", user: assigner, details: {old_developer_id: developer.id, new_developer_id: new_developer.id})
    FeatureTicketMailer.developer_reassigned_to_old_developer(old_developer, self).send
    FeatureTicketMailer.developer_reassigned_to_new_developer(old_developer, self).send
    FeatureTicketMailer.developer_reassigned_to_submitter(old_developer, self).send
    FeatureTicketMailer.developer_reassigned_to_IT_manager(old_developer, self).send
  end
end

# Now we have all the behavior we want. Except our app is a mess. The FeatureTicket class is relying
# on a LOT of imperative programming. It's trying to micromanage all the other classes.
# This really adds to the FeatureTicket class' complexity, and it makes it very difficult to 
# test effectively. Why don't we refactor and reduce the brittleness of this code!
# Here's an easy win: the events know the entire context relevant for sending emails.
# We we can simplify the signature of the FeatureTicket methods easily by just passing the
# events in.

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  has_many :events
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
    event = Event.create(type: "FeatureSubmitted", user: submitter)
    FeatureTicketMailer.ticket_submitted_to_IT_department(event).send
    FeatureTicketMailer.ticket_submitted_to_IT_manager(event).send
    FeatureTicketMailer.ticket_submitted_to_submitter(event).send
  end
  
  def assign(developer, assigner)
    update(status: "assigned", developer: developer)
    event = Event.create(type: "FeatureAssigned", user: assigner, details: {developer_id: developer.id})
    FeatureTicketMailer.developer_assigned_to_developer(event).send
    FeatureTicketMailer.developer_assigned_to_submitter(event).send
  end
  
  def reassign(old_developer, new_developer, assigner)
    update(developer: new_developer)
    event = Event.create(type: "FeatureReassigned", user: assigner, details: {old_developer_id: developer.id, new_developer_id: new_developer.id})
    FeatureTicketMailer.developer_reassigned_to_old_developer(event).send
    FeatureTicketMailer.developer_reassigned_to_new_developer(event).send
    FeatureTicketMailer.developer_reassigned_to_submitter(event).send
    FeatureTicketMailer.developer_reassigned_to_IT_manager(event).send
  end
end

# Now let's delegate the responsibility of notifications more rationally. Notifications
# really are about events, not just the ticket. Let's push the complexity of the notifications
# into the event classes.

#event.rb

class Event < ActiveRecord::Base
  belongs_to :user
  belongs_to :ticket

  serialize :details, Hash

  def self.record(args)
    create(args)
  end

  def self.record_and_notify
    event = record(args)
    event.send_notifications
  end

  def send_notifications
    # just in case this is not defined in a subclass
  end

end

class FeatureSubmitted < Event
  
  def send_notifications
    FeatureTicketMailer.ticket_submitted_to_IT_department(self).send
    FeatureTicketMailer.ticket_submitted_to_IT_manager(self).send
    FeatureTicketMailer.ticket_submitted_to_submitter(self).send
  end

end

class FeatureAssigned < Event
  
  def send_notifications
    FeatureTicketMailer.developer_assigned_to_developer(self).send
    FeatureTicketMailer.developer_assigned_to_submitter(self).send
  end

end

class FeatureReassigned < Event

  def send_notifications
    FeatureTicketMailer.developer_reassigned_to_old_developer(self).send
    FeatureTicketMailer.developer_reassigned_to_new_developer(self).send
    FeatureTicketMailer.developer_reassigned_to_submitter(self).send
    FeatureTicketMailer.developer_reassigned_to_IT_manager(self).send
  end

end

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  has_many :events
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
    Event.record_and_notify(type: "FeatureSubmitted", user: submitter)
  end
  
  def assign(developer, assigner)
    update(status: "assigned", developer: developer)
    Event.record_and_notify(type: "FeatureAssigned", user: assigner, details: {developer_id: developer.id})
  end
  
  def reassign(old_developer, new_developer, assigner)
    update(developer: new_developer)
    Event.record_and_notify(type: "FeatureReassigned", user: assigner, details: {old_developer_id: developer.id, new_developer_id: new_developer.id})
  end
end

# In this last step, we leveraged polymorphism to delegate
# the notification logic to the appropriate event class. Now it's really easy to add 
# notifications without cluttering up the FeatureTicket class. The FeatureTicket 
# class is much easier to read and understand now! On top of that, it's easier to test.

# As a final step, let's imagine that our event classes need to do more than just send
# notifications. Maybe they need to dynamically print things about themselves. Let's also 
# get rid of the "reassigned" methods. They don't seem very DRY.

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  has_many :events
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
    Event.record_and_notify(type: "FeatureSubmitted", user: submitter)
  end
  
  def assign(assigner, new_developer)
    old_developer_id = developer.id
    update(status: "assigned", developer: developer)
    Event.record_and_notify(type: "FeatureAssigned", user: assigner, details: {new_developer_id: developer.id, old_developer_id: old_developer_id})
  end
  
end

#event.rb

class Event < ActiveRecord::Base
  belongs_to :user
  belongs_to :ticket

  serialize :details, Hash

  def self.record(args)
    create(args)
  end

  def self.record_and_notify
    event = record(args)
    event.send_notifications
  end

  def send_notifications
    # just in case this is not defined in a subclass
  end

  def log_message
    "#{user.name} #{verb_phrase} ticket '#{ticket.name}' at #{created_at}"
  end

  def verb_phrase
    "took some action on"
  end

end

class FeatureSubmitted < Event

  def verb_phrase
    "submitted feature"
  end
  
  def send_notifications
    FeatureTicketMailer.ticket_submitted_to_IT_department(self).send
    FeatureTicketMailer.ticket_submitted_to_IT_manager(self).send
    FeatureTicketMailer.ticket_submitted_to_submitter(self).send
  end

end

class FeatureAssigned < Event

  def verb_phrase
    "assigned #{new_developer_name} to"
  end

  def new_developer_name
    new_developer ? new_developer.name : ""
  end

  def new_developer
    details[:new_developer_id] ? User.find(new_developer_id) : nil
  end

  def old_developer
    details[:old_developer_id] ? User.find(old_developer_id) : nil
  end

  def send_notifications
    if old_developer
      FeatureTicketMailer.developer_reassigned_to_old_developer(self).send
      FeatureTicketMailer.developer_reassigned_to_new_developer(self).send
      FeatureTicketMailer.developer_reassigned_to_submitter(self).send
      FeatureTicketMailer.developer_reassigned_to_IT_manager(self).send
    else
      FeatureTicketMailer.developer_assigned_to_developer(self).send
      FeatureTicketMailer.developer_assigned_to_submitter(self).send
    end
  end

end

# Well, now! The Event class if getting pretty hairy. The notification logic is becoming more complex. 
# It's a good bet that it will become more complex as our app grows. It might make more sense to delegate that
# responsibility to dedicated class, an EventNotifier.

#event_notifier.rb

module EventNotifier

  def self.send_notifications_for(event)
    method_name = method_name_from(event)
    respond_to?(method_name) ? send(method_name, event) : raise "Notification not defined"
  end

  def self.method_name_from(event)
    event.type.to_s.underscore.to_sym
  end

  def self.feature_submitted(event)
    FeatureTicketMailer.ticket_submitted_to_IT_department(self).send
    FeatureTicketMailer.ticket_submitted_to_IT_manager(self).send
    FeatureTicketMailer.ticket_submitted_to_submitter(self).send
  end

  def self.feature_assigned(event)
    if event.old_developer
      FeatureTicketMailer.developer_reassigned_to_old_developer(event).send
      FeatureTicketMailer.developer_reassigned_to_new_developer(event).send
      FeatureTicketMailer.developer_reassigned_to_submitter(event).send
      FeatureTicketMailer.developer_reassigned_to_IT_manager(event).send
    else
      FeatureTicketMailer.developer_assigned_to_developer(self).send
      FeatureTicketMailer.developer_assigned_to_submitter(self).send
    end
  end

end

#event.rb

class Event < ActiveRecord::Base
  belongs_to :user
  belongs_to :ticket

  serialize :details, Hash

  def self.record(args)
    create(args)
  end

  def self.record_and_notify
    event = record(args)
    EventNotifier.send_notifications_for(event)
  end

  def log_message
    "#{user.name} #{verb_phrase} ticket '#{ticket.name}' at #{created_at}"
  end

  def verb_phrase
    "took some action on"
  end

end

class FeatureSubmitted < Event

  def verb_phrase
    "submitted feature"
  end

end

class FeatureAssigned < Event

  def verb_phrase
    "assigned #{new_developer_name} to"
  end

  def new_developer_name
    new_developer ? new_developer.name : ""
  end

  def new_developer
    details[:new_developer_id] ? User.find(new_developer_id) : nil
  end

  def old_developer
    details[:old_developer_id] ? User.find(old_developer_id) : nil
  end

end

#feature_ticket.rb

class FeatureTicket < ActiveRecord::Base
  has_many :events
  belongs_to :developer, class_name: 'User'
  belongs_to :submitter, class_name: 'User'

  def submit(submitter)
    update(status: "submitted", submitter: submitter)
    Event.record_and_notify(type: "FeatureSubmitted", user: submitter)
  end
  
  def assign(assigner, new_developer)
    old_developer_id = developer.id
    update(status: "assigned", developer: developer)
    Event.record_and_notify(type: "FeatureAssigned", user: assigner, details: {new_developer_id: developer.id, old_developer_id: old_developer_id})
  end
  
end

# At last, all of our classes are EXTREMELY simple. By using a little metaprogramming magic,
# we're dynamically sending messages to the appropriate EventNotifier methods. The Event class
# can focus on its own responsibilities and forget about notifications. And we have exemplary
# code that makes it easy to identify where to extend event notification logic. We've DRYed up our 
# methods and abstracted the assign/reassign logic. We have almost no IF blocks because we've 
# leveraged polymorphism in our classes, so the cyclomatic complexity is very low.
# As our app grows, it will be much easier to test and extend safely!