gistfile1.md

#Discovering Better Object Oriented Design with tests

In the zirannanren app, From begging, We just provided one type of courses. We record Videos and share them to our members.

In our videos#index we did this:

class VideosController < ApplicationController
  def index
    @videos = Video.all
  end
end

the test is easy to write:

describe "GET index" do
  it "assigns the video variables" do
    video1 = Fabricate(:video)
    video2 = Fabricate(:video)
    get :index
    assigns(:videos).should =~ [video1, video2]
  end
end

All the things is ok.

Someday we began to provid both OnlineCourse and Video, and we want them all to be listed in one page.

Like

We change the VideosController to CourseResourcesController, because the controller will not just only have videos.

class CourseResourcesController < ApplicationController
  def index
    @videos = Video.all
    @online_course = OnlineCourse.all
  end
end

describe "GET index" do
  let!(:video1) { Fabricate(:video) }
  let!(:video2) { Fabricate(:video) }
  
  let!(:online_course1) { Fabricate(:online_course) }
  let!(:online_course2) { Fabricate(:online_course) }
  
  it "assigns the video variables" do
    get :index
    assigns(:videos).should =~ [video1, video2]
  end
  
  it "assings the online_course variables" do
    get :index
    assings(:online_course).shoud =~ [online_course1, online_course2]
  end
end

And some days later, We want to provide another service, Podcast. We still want to list all of those resources in one page. But this time, we will not divide them, we need to put them together to display like below and we want to sort it by created time.

Now, lets start from TDD.

describe "GET index" do
  let!(:video1) { Fabricate(:video, created_at: Time.now - 1.day) }
  let!(:video2) { Fabricate(:video - 2.days) }
  
  let!(:online_course1) { Fabricate(:online_course, created_at: Time.now - 3.days) }
  let!(:online_course2) { Fabricate(:online_course, created_at: Time.now - 5.days) }
  
  let!(:podcast1) { Fabricate(:podcast, create_at: Time.now - 6.days) }
  let!(:podcast2) { Fabricate(:podcast, create_at: Time.now - 4.days) }
  
  it "assigns the course_resources variables" do
    get :index
    assigns(:course_resources).should == [video1, video2, online_course1, podcast1, online_course2, podcast2]
  end
end

And in the controller

class CourseResourcesController < ApplicationController
  @course_resources = 
end

Here, I really don't know how to write the code to make that test pass. But after a long time thinking, I implement this below:

class CourseResourcesController < ApplicationController
  def index
    videos = Video.all
    online_courses = OnlineCourse.all
    podcasts = Podcast.all
  
    all_resources = [videos, online_courses, podcasts].flatten!
  
    @course_resources = all_resources.sort { |x, y| y.created_at <=> x.created_at }
  end
end

But, what about if we want to add a tag to choose the "free" resources or we just want to find the specifiy type of resources, like below

I can't image how hard to write the code to make the test pass. Why it is so hard to do that?

First, this design is based on data-centric. Our test has a lot of setups, but we just focus on the status that it will return. So this test actually doesn't help us so much to discover any solution.

Second, it did too much things.

1. It collects the resources we needed.
2. It sorts them with order.
3. Now, it needs to select the right resources depending on the tag params.

We call this Context Dependency. If we change the behaviors, we need to change the whole thing.

Now, our dependencies looks like this below

If you refer to somethings, you denpend on it, when the tings you denpend on change, you must change. That's why it is so difficult to add the "Tag" feature.

Is there a way to make the tests give us more hint about design? Yes, there is a way, we use mocks and stubs.

So let's do it again from beginning with stubs.

it "assigns the course_resources instance variable" do
end

First, All I want is that assigns(:course_resources) should equals some list of resource.

assigns(:course_resources) = [resource]

I don't really care about what the resource are for the purpose of this test. So I make it as a stub.

resource = stub
assigns(:course_resources) = [resource]

And we need to invoke the controller, and pass a params called "free" to it.

resource = stub
get :index, tag: free
assigns(:course_resources) = [resource]

And then We will ask ourselves, how does the controller get the hands on the resource stub. I wish I have a object called "collections", and it should call a mehtod "order", and it returns the list of resources.

resource = stub
collections.should_receive(:order).and_return([resource])
get :index
assigns(:course_resources) = [resource]

What is the collections? It should be an array contains all of the ordered resources we needed. Now I just specify it as a stub. And where does it come from? I wish I have a CourseResources class, it contains all of the Videos, OnlineCourses and Podcasts, and it returns the collections.

resource = stub
collections = stub
CourseResource.stub(:new).with(Video.all, OnlineCourse.all, Podcast.all).and_return(collections)
collections.should_receive(:order).and_return([resouece])
get :index
assigns(:course_resources) = [resource]

Now before we are going to implement the controller, let's take a break here. Why do we need a stop? Because I found a test smell. Test semll doesn't means bad, it is just a warning that did you need to do that?

So here, the test smell is CourseResource.stub(:new).with(Video.all, OnlineCourse.all, Podcast.all). It is coupling with the arguments. What means arguments coupling in here?

If we keep this, We need to implement the controller like:

def index
  course_resources = CourseResource.new(Video.all, OnlineCourse.all, Podcast.all).order
end

If I going to change the arguments, I need to change both the tests and the implementation. It just duplicate under its test. It seems so bad. But if I am sure that the arguments will never be changed, it is ok, because now the arguments becomes to a low dependency, the arguments never been changed, we can depend on them. It seems like we depend on Rails API.

But here, I am not sure that the arguments will be never changed, maybe few more days, we will provide more other resources and we want to display them in that page.

What do we do to replace of that? Think about the role of controller, the controller just needs to get the reources of the course from a class named CourseResource. The controller should not know too much about who the CourseResource will collect. If the controller knows too much, it means that the controller depend on too much things and those objects will be coupling together.

So here we change CourseResource.stub(:new).with(Video.all, OnlineCourse.all, Podcast.all).and_return(collections) to CourseRresource.stub(:new).and_return(collections)

So in our controller, we write the implementation.

def index
  course_resources = CourseResource.new.order
end

And now the controller spec will pass.

Maybe you want to ask how could I find the test smell? In here, there is role that "Single Responsibility Princeple". Follow this rule, the test smell will be easily found.

In here, the controller just need to tell CourseResource to initialize an instance of it, we don't care about how CourseResource will do it, so we get CourseResource.new. But look at CourseResource.stub(:new).with(Video.all, OnlineCourse.all, Podcast.all).and_return(collections), we find the controller either tells CourseResource to initialize an instance and tell Video, OnlineCourse, Podcast to call all() method to get all of their instances. It breaks the rule of SRP, so they all coupling together.

Then let's implement the CourseResource class, and I will make the spec and write the first example of it.

First, I need to ask My self that what's the role CourseResource plays? It will play a role of presenting all resources with the order of created at DESC.

So let's continue. First, I want to introduce "Test Isolation". Our goal of design is to reduce the dependencies, so we abstracted CourseResorce from Rails. It should be a pure ruby code and it is beyond Rails. So if it really doesn't have any dependencies with Rails, we can just test the pure ruby code in isolation without load Rails. So we replace require "spec_helper" with require_relative "../../app/presenters/course_resource.rb"

require_relative "../../app/presenters/course_resource.rb"

describe CourseResource do
  it "collects the resources" do
  end

  it "sort the resources' order"
end

In the first example, I need to setup the test. It should collect with Video.all, Podcast.all and OnlineCourse.all.

it "collects the resources" do
  Video.stub(:all).and_return([video])
  Podcast.stub(:all).and_return([podcast])
  OnlineCourse.stub(:all).and_return([online_course])
end

And I don't care about what are the video, podcast and online_course. They are all stubs.

it "collects the resources" do
  video = stub(:video)
  podcast = stub(:podcast)
  online_course = stub(:online_course)

  Video.stub(:all).and_return([video])
  Podcast.stub(:all).and_return([podcast])
  OnlineCourse.stub(:all).and_return([online_course])
end

And we add the assertion

it "collects the resources" do
  video = stub(:video)
  podcast = stub(:podcast)
  online_course = stub(:online_course)

  Video.stub(:all).and_return([video])
  Podcast.stub(:all).and_return([podcast])
  OnlineCourse.stub(:all).and_return([online_course])

  Courseresource.new.should =~ [video, podcast, online_course]
end

We implement the CourseResource

class CourseResource
  def initialize
    [Video.all, OnlineCourse.all, Podcast.all].flatten!
  end
end

But when we run rspec, it failed with an thrown error "uninitialized constant Video". What's wrong with that? Look at our implementation, we specified Video, OnlineCourse and Podcast. It is still have dependencies with those objects.

Acturally, we could find that when we writing the test. Wanting to mock concrete objects is a design smell. It is telling you that the implementation of your actual code has a problem. Because Well designed objects don't know who they are talking to.

So let's re-think about the first test example, I don't really care about which object and how many objects it needs to collect. I just need it could deal with some arrays.

Ok, let's rewrite the test.

it "collects the resources" do
  video = stub(:video)
  podcast = stub(:podcast)
  online_course = stub(:online_course)

  Courseresource.new([video], [podcast], [online_course]).should =~ [video, podcast, online_course]
end

Then we write the implementation

class CourseResource
  def initialize(*resources)
    resources.flatten!
  end
end

Then the unit test passes. But now the new() method of CourseResource needs arguments, the controller test is failed. And in here we haven't specify anything about Video, Podcast and OnlineCourse. Maybe we can pass them in controller like CourseReource.new(Video.all, Podcast.all, OnlineCourse.all), so everything will pass. But, wait! We just go back to the situation that controller is coupling with them, what the hell?

Now we need to find another way to do that, and we will not change the controller implementation. if we won't change the controller implementation, we need to change the way of passing the arguments to CourseResource new() method. So we need a default argument. Then we need to change our test.

it "collects the resources" do
  video = stub(:video)
  podcast = stub(:podcast)
  online_course = stub(:online_course)

  CourseResource.new([[video], [podcast], [online_course]]).should =~ [video, podcast, online_course]
end

We pass an array of any different arrays into CourseResource.new(), then we write implementation.

class CourseResource
  def initialize(resources=[])
    resources.flatten!
  end
end

And we need to specify the objects we want it to collect, we pass it to the argument default value.

class CourseResource
  def initialize(resources=[User.all, Podcast.all, OnlineCourse.all])
    resources.flatten!
  end
end

It is done! Everything passes, and our code is losely coupling and object oriented designed.

Next we need to sort the order of the course resources, first we wirte the test, we don't care about what the video, podcast or online_course is, they are just subs with a method created_at.

it "sort the resources' order" do
  video = stub("video", created_at: Time.now - 1)
  podcast = stub("podcast", created_at: Time.now - 2)
  online_course = stub("online_course", created_at: Time.now - 3)

  CourseResource.new([[video], [podcast], [online_course]]).order.should == [online_course, podcast, video]
end

Then we implement the order method, acturally here we just deal with ruby array, it is so easy to write that.

def order
  self.sort { |x, y| y.created_at <=> x.created_at }
end

Ok, we want to implement tag, it is just the same, we deal with the pure ruby array object! What if someday we want to add another resource like PDFs? Because our code is object oriented designed, it is easy to do that, we don't care about the whole tings of implementation of controller, just add it to the CourseResource new() method default value, and our test will still pass, the code is still working. How flexible it is now! We don't refactor it so many time, we actually just make it once, because we use mock/stub and we isolated test it. Those way of test drives us to write implementation code step by step, and it tells us the problems of our design, then we find that test smell, and make a decision to re design the bad part in time. It doesn't just drive us to development, it also drives us to Design.

###Conclusion:

If the code becomes more difficult to manipulate is because of the dependecies, our goal of design is reduce the dependencies and make the objects losely coupling.

Procedural Programming: Add Code To The Method. OO: Change Object Composition.

SRP Depend upon abstractions, not concretions.