OOP Refactoring: from a god class to smaller objects

In the early years of my programming career, I got infected by the OOP thinking. It made (and still does) sense to model the problem in terms of objects and methods operating on that object.

However, at the beginning, my OOP code resulted in a god class - a class that knows almost everything.

Let’s say I work on a Project Management app. Usually my User class and Project class would be big, like this:

class Project

  def initialize
    @tasks = []
    @members = []
    @budget = Budget.new
  end

  def add_task(task)
    raise Duplicate if @tasks.include?(task)
    @tasks << task
  end

  def tasks
    @tasks
  end

  def assign(developer)
    @members << developer
  end

  def assign_task(task, developer)
    raise AlreadyAssigned if task.assigned?
    task.assign_developer
  end

  def members
    @members
  end

  def current_backlog
    @tasks.select{|task| task.assigned?}
  end

  def increase_budget
    raise ProjectFinished if finished?
    budget.increase
  end
end

The Project class plays too many roles - it serves like a proxy/facade to other objects (developer or tasks). It has business rules (Duplicate, AlreadyAssignedTask). It also has query methods - tasks, members, backlog.

Over time, I learnt some additional techniques which helped me reduce the size of the god class.

The first technique is CQRS - in short, it allows me to extract the queries outside of this object.

class TasksList
  def initialize(event_bus)
    event_bus.subscribe(TaskAdded, add_task)
    event_bus.subscribe(TaskAssignedToDeveloper, assign_task_to_developer)
    event
  end

  def add_task(event)
    Task.create
  end

  def list
    Task.all
  end

  class Developer < ActiveRecord::Base
    has_many :tasks
  end

  class Task < ActiveRecord::Base
  end
end

This is an example query object, or as we call it in CQRS - it’s a read model. Its only purpose is to show data. There is no logic here, just a declaration what events are needed and then simple CRUD (ActiveRecord calls).

OK, but if we need events here, then they must be published somewhere, right?

Let’s start with publishing them in the Project object.

class Project

  def initialize(event_bus)
    @tasks = []
    @members = []
    @budget = Budget.new
    @event_bus = event_bus
  end

  def add_task(task)
    raise Duplicate if @tasks.include?(task)
    @tasks << task
    @event_bus.publish(TaskAdded)
  end

  def assign(developer)
    @members << developer
  end

  def assign_task(task, developer)
    raise AlreadyAssigned if task.assigned?
    task.assign_developer
    @event_bus.publish(TaskAssigned.new(developer.id, task.id))
  end

end

What has changed?

The constructor now accepts event_bus as a dependency. We need to publish the events somewhere, to a bus. Then, it those methods which serve as commands (change something), we publish the events. While, the object grabbed a new responsibility - it also reduced its roles. We no longer have the query methods. All the accessors disappeared. Thanks to the fact that we have the event_bus which connects our object (via event) with the read models, the read models are totally separated and decoupled.

Are we happy now? It’s better, but still feels like a god class.

What else can we do?

First, let’s get rid of event_bus. While we need to have events being published - it doesn’t have to be part of the class. This object is called from somewhere, right? Why not move this event publishing code to the callers?

Usually in Rails apps, we have the service objects layer or as we call it in CQRS - command handlers.

class AssignTaskHandler
  def initialize(event_bus, project_id, task_id, developer_id)
    project = ProjectRepo.load(project_id, event_bus)
    begin
      project.assign_task(Task.new(task_id), Developer.new(developer_id))
    rescue AlreadyAssigned => e
      return Error(e)
    end
    return Success.new
  end
end

Currently it passes the event_bus to the Project, but in many cases it doesn’t have to. Let’s change it to:

class AssignTaskHandler
  def initialize(event_bus, project_id, task_id, developer_id)
    project = ProjectRepo.load(project_id)
    begin
      project.assign_task(Task.new(task_id), Developer.new(developer_id))
    rescue AlreadyAssigned => e
      return Error(e)
    end
    @event_bus.publish(TaskAssigned.new(developer_id, task_id))
    return Success.new
  end
end

Now, we publish the event as part of the command handler, so we no longer need this in the Project class, which now looks like this:

class Project

  def initialize
    @tasks = []
    @members = []
    @budget = Budget.new
  end

  def add_task(task)
    raise Duplicate if @tasks.include?(task)
    @tasks << task
  end

  def assign(developer)
    @members << developer
  end

  def assign_task(task, developer)
    raise AlreadyAssigned if task.assigned?
    task.assign_developer
  end

end

Feels better now, doesn’t it?

Still, this is just an example. It contains only 3 business logic methods. In practice, there would be more, each of them somehow connected to the current instance variables - tasks, members, budget.

Previously, it was useful to have those ivars together, as for some queries we may want to display a report which contains all of them. Now that we extracted read models, they can handle this and we no longer need such place to connect.

Also, this is where my lessons from Domain-Driven Design arrived. In short, DDD is about “a language in a context”. The important part is that there’s no one, dominant language or vocabulary for the whole of our system. In fact, we can (and should) have many models, many languages to describe different parts.

In our case, the code screams to us, that it’s actually at least 3 contexts:

• Budgeting
• Tasks
• Human Resources

and apologies for suggesting that someone may call Developers as Resources…

In each of them, the concept of a Project means something different. In fact, some of them may not even use the term Project to describe what they need. For example, maybe for Budgeting, a Project is just an Account?

Let’s try to split this code then. we will create a module for each context:

module Budgeting

  class Account
    def initialize
      @amount = Money.new(0.0)
   end

    def increase(amount)
      @amount += amount
    end
  end
end

module Tasks
  class Project
    def initialize
      @tasks = []
    end

    def add_task(task)
      raise Duplicate if @tasks.include?(task)
      @tasks << task
    end
  end
end

module HumanResources
  class Project
    def initialize
      @resources = []
    end

    def assign(resource)
      @resources << resource
    end
  end
end

Does it look better now? To my taste it’s better. Apart from the offensive Resource part, but hey, that’s the language they use to describe us.

There are techniques which may help us even further, but for the scope of this article, I think that’s enough.

Did you like this?