bgror

More Tiny Classes »

Created at: 06.02.2012 14:00, source: RailsTips - Home, tagged: gauges refactoring

My last post, Keep ’Em Separated, made me realize I should start sharing more about what we are doing to make Gauges maintainable. This post is another in the same vein.

Gauges allows you to share a gauge with someone else by email. That email does not have to exist prior to your adding it, because nothing is more annoying that wanting to share something with a friend or co-worker, but first having to get them to sign up for the service.

If the email address is found, we add the user to the gauge and notify them that they have been added.

If the email address is not found, we create an invite and then send an email to notify them they should sign up, so they can see the data.

The Problem: McUggo Route

The aforementioned sharing logic isn’t difficult, but it was just enough that our share route was getting uggo. It started off looking something like this:

post('/gauges/:id/shares') do
  gauge = Gauge.get(params['id'])

  if user = User.first_by_email(params[:email])
    Stats.increment('shares.existing')
    gauge.add_user(user)
    ShareWithExistingUserMailer.new(gauge, user).deliver
    {:share => SharePresenter.new(gauge, user)}.to_json
  else
    invite = gauge.invite(params['email'])
    Stats.increment('shares.new')
    ShareWithNewUserMailer.new(gauge, invite).deliver
    {:share => SharePresenter.new(gauge, invite)}.to_json
  end
end

Let’s be honest. We’ve all seen Rails controller actions and Sinatra routes that are fantastically worse, but this was really burning my eyes, so I charged our programming butler to refactor it.

The Solution: Move Logic to Separate Class

We talked some ideas through, and once he had finished, the route looked more like this:

post('/gauges/:id/shares') do
  gauge    = Gauge.get(params['id'])
  sharer   = GaugeSharer.new(gauge, params['email'])
  receiver = sharer.perform
  {:share => SharePresenter.new(gauge, receiver)}.to_json
end

Perfect? Who cares. Waaaaaaaaay better? Yes. The concern of a user existing or not is moved away to a place where the route could care less.

Also, the bonus is that sharing a gauge can now be used without invoking a route.

So what does GaugeSharer look like?

class GaugeSharer
  def initialize(gauge, email)
    @gauge = gauge
    @email = email
  end

  def user
    @user ||= … # user from database
  end

  def existing?
    user.present?
  end

  def perform
    if existing?
      share_with_existing_user
    else
      share_with_invitee
    end
  end

  def share_with_existing_user
    # add user to gauge
    ShareWithExistingUserMailer.new(@gauge, user).deliver
    user
  end

  def share_with_invitee
    invite = ... # invite to db
    ShareWithNewUserMailer.new(@gauge, invite).deliver
    invite
  end
end

Now, instead of having several higher-level tests to check each piece of logic, we can just ensure that GaugeSharer is invoked correctly in the route test and then test the crap out of GaugeSharer with unit tests. We can also use GaugeSharer anywhere else in the application that we want to.

This isn’t a dramatic change in code, but it has a dramatic effect on the coder. Moving all these bits into separate classes and tiny methods improves ease of testing and, probably more importantly, ease of grokking for another developer, including yourself at a later point in time.

Refactoring: be eager, not reckless »

Created at: 19.10.2011 19:54, source: has_many :through, tagged: agile refactoring

The illustrious Chris Eppstein recently tweeted:

If some code should be refactored, stop what you are doing and refactor it.

I was about to respond, but realized I had more to say than would fit in a tweet. (Waiting for someone to fix that problem!) (Then I got distracted and didn't finish this article for a few days, oops.)

Now, Chris is really smart and probably doesn't mean exactly what he said, but it's easy to misinterpret his advice. I'll agree with him that you should be eager to refactor code when you discover the need. Don't let that technical debt accrue interest longer than necessary! However, you shouldn't be reckless about it.

Please keep this in mind:

Don't do a refactoring in the middle of making another change.

If you are working on a story and in the middle of making a code change when you discover the need to refactor something, make a note of it (I usually create a chore in Pivotal Tracker) and forget about it until you're done with the change in progress. After you complete the change, come back and do the refactoring. Make sure all tests are green before starting to refactor, and are green when done. The refactoring change should be a separate commit in git (separate checkin in SVN, etc).

OK, I'm pragmatic and realize that that approach doesn't work all the time, but it's a good ideal to shoot for. I considered discussing cases where it would be OK to refactor something in the middle of another change, but on second thought I think I'll leave that be for now. You'll learn that for yourself better than by following someone else's advice on the subject.

Let Them Code Cake! »

Created at: 09.02.2010 20:00, source: Engine Yard Blog, tagged: Technology activerecord cake datamapper refactoring

In TDD and BDD we write small, focused, technical tests, sometimes called micro-tests. One of the core ideas is that these tests should run fast, really fast—each one measured in milliseconds. If you’re writing plain Ruby code, that’s pretty easy to accomplish. However, when you’re using something like Rails or Merb and DataMapper or ActiveRecord, it can get a bit more challenging.

Why do we end up with model tests/specs that run slowly? In looking at how we write and test our code, let’s ignore controllers and views, and focus on models. After all, models are where you put your business logic.

There can be several reasons for a slow test suite: database access, unnecessary objects, a massive setup that takes a while to load—just to name a few. This time though, I want us to focus on a new reason, one that’s a result of operating in the context of ActiveRecord or DataMapper. One that tricks us into thinking we’re doing well. One that exposes a few somewhat major flaws in the design of AR and DM.

With ActiveRecord and DataMapper, each model class is responsible for its own persistence, hence DHH using the name “Active Record.” It was inspired by Fowler’s writing on the pattern in Patterns of Enterprise Application Architecture and is the enabler of our model addiction.

OverActive Record

Used properly, the active record pattern is great for persisting data. These objects are great at handling their own persistence. Accessors, associations, and validation make a good wrapper around the transactional nature of a database. This is fine when the model is a simple data object, but we all run into problems when we start adding other behavior to the model class.

Weighted Companion Cake: The cube was the cake!

(Compliments to flickr user Mandrake88)

Recall the Single Responsibility Principle: “There should never be more than one reason for a class to change.” As we commonly use them, ActiveRecord and DataMapper¹ classes almost always violate this principle. There are usually at least two responsibilities handled by every AR/DM model: persistence and business logic. Carrying around that persistence behavior, and all the dependencies that go along with it, is what bloats and slows down our specs.

There have been various attempts at dealing with this: in memory databases, stubbing parts of the DM/AR frameworks, etc. None of these are ideal. Either they don’t speed things up as much as we’d like, they’re awkward, or they bulk up the specs an objectionable amount.

Is There a Better Way? Why, Yes! Yes There Is.

DM/AR lets you define a model’s properties, its persistent parts, and generates the associated accessors and mutators. All access to the persistent properties is handled through these accessors and mutators. This gives us a perfect seam along which to split the class. All of the persistence-dependent functionality is on one side of this seam, and the business logic of the model is on the other.

We need a good saving throw against this frosting.

Courtesy of Mike’s Amazing Cakes

Pretend… It’s a Cake.

Yes, you read that correctly. Keep on reading—it makes sense!

Each of our models is like a frosted cake. We have the nice fluffy cake batter, baked with our properties, validations, and persistence. Then we frost the outside with our business logic. We can take a simple ActiveRecord cake and make it into almost anything, with a good amount of frosting.

Now herein lies the problem: the frosting starts to take on a life of its own. Before we know it we’re baking a few 100 ActiveRecord cakes each time we need to test the frosting behavior… There are two really good ways to get around this: cupcakes and cardboard.

Strategy 1: Oh Look! Cupcakes!

When we build a web application with ActiveRecord or DataMapper, it’s really hard to think outside the persistence box. If something is persisted on that model, or if it uses something on that model, we automatically add the code to the model. This leads quickly to a gigantic cake that does everything.

This is especially bad with primary models like User. The poor bloated User gets tons of code stuffed into it because our app is usually focused around users. Users do things in our app. Users get affected by things. They control the app and most things belong to the user in some way. But this doesn’t mean our User class should be 1000 lines.

This cake does too much (yes that’s cake)

The solution is to break out chunks of code into their own classes. There are many common patterns that we can find in our classes: Factory, Provider, Policy, Strategy, and plenty of types that are custom to each domain (Cake Decorator?). Let’s lump these all into something that we’ll call Cupcake Classes.

At Engine Yard, one of the places we found an opportunity for a Cupcake was our Deployment initialize. The method had to do a lot of data mutating before it could create an instance. Blank strings needed to become nil, string representations of a type needed to be converted to the actual type, and defaults needed to be applied to fill out the incoming data. The initialize method was a ridiculous 50 lines long. This needed to become its own class.

The result is a really clean initialize:

  def initialize(params={})
    merger = Deployment::DefaultsMerger.new(params)
    merger.each_attribute { |att, val| send("#{att}=", val) }
  end

…and a class that knows all about how to merge incoming params and defaults. It has a clear API, and it’s easily testable. We know what comes in and what comes out and we don’t have to create a database record every time we test it.

class Deployment
  class DefaultsMerger

    def initialize(params)
      @attributes = merge_with_defaults(params)
      scrub_blank 0, :data_volume_size, :app_server_count
      scrub_blank nil, :db_volume_id, :data_volume_id
      coerce_value(:instance_size, :db_instance_size) do |val|
        Instance::Size.coerce(val)
      end
      # ...
    end

    def each_attribute
      @attributes.each do |k,v|
        yield k, safe_to_i(v)
      end
    end

  private

    def defaults
      Mash.new({
        :data_volume_size => Volume::DEFAULT_SIZE,
        :app_server_count => 1,
        # ...
      })
    end
    # ...
  end
end

Cupcake Coding Rule of Thumb: Messy Code Makes for Good Cupcakes!

When you want to clean up your models, look for messy code. Long methods or sets of utility methods (methods that barely, if at all, touch instance state) are just begging to be cut up into smaller classes.

What tipped us off was that the initialize method was long, nasty, and really had nothing to do with the class it was in. Mostly, it dealt with params, a hash. Almost every line in the old method did hash operations, not operations dealing with the actual model.

We clearly needed a class that would know how to work with merging our defaults into an incoming hash. The resulting class would also improve our real test coverage. It’s much easier to test a bunch of branches when they don’t have to pass through the model life cycle.

describe Deployment::DefaultsMerger do
  describe_attribute :data_volume_size do
    it_defaults_to Volume::DEFAULT_SIZE
    it_converts ["", nil, 0, "0"], :to => 0
    it_converts [123, "123"],      :to => 123
  end

  describe_attribute :app_server_count do
    it_defaults_to 1
    it_converts ["", nil, 0, "0"], :to => 0
    it_converts [2, "2"],          :to => 2
  end
  # ...
end

With a focused class, it’s easy to write focused specs. With a few custom methods in your specs, you have beautifully concise descriptions of behavior to match your simple code.

Looks yummy!

Thanks to flickr user hazellterry

Strategy 2: Cardboard Cake

The next way to improve your models and your testing is to extract the model’s behavior from the persistence. The idea is to test the frosting without having to bake a cake every time. Our example class after the behavior has been extracted:

class Oven
  include DataMapper::Resource
  include Oven::Behavior

  ## Properties

  property :id,           Serial
  property :oven_type,    String
  property :temp_setting, String

  has 1, :heating_element
  has 1, :latch
  ## ...

end

Here’s the frosting, or behavior, that has been extracted from it:

class Oven
  module Behavior

    def check_lock
      latch.engage! if dangerous_temperature?
    end

    def dangerous_temperature?
      Oven::Temperatures.dangerous? temp_setting
    end

    def turn_on(temp = 350) # ...
    def turn_off # ...
    def preheating? # ...
  end
end

Once this is done, you can write specs that focus on the logic in isolation. Instead of testing every aspect of behavior on top of the persistence framework (slow), you can isolate the behavior (fast). Sort of like making the cake out of cardboard so you can test our frosting without the baking time (See? Told you it would make sense!).

How Do We Achieve This Isolation? Mock Object Trickery, But of Course!

Let’s create a mock to stand in for the model object being tested. Let’s call these Cake Mocks. Then, we’ll extend the mock object with with the Behavior we’re testing.

  @oven = mock("Oven").extend(Oven::Behavior)

It still acts like an oven, but instead we’ve frosted the outside of a cardboard cake with the same business logic that we would add to a real ActiveRecord object. The trick is to have the mock object extend the module that contains the business logic. The result is an object containing the behavior of the model but not its persistence.

Now we can stub accessors on the mock (but only those involved in the example) and set expectations on the mutators. Then the example can call methods on the mock oven as if it was a real Oven instance. Those calls invoke the actual business logic methods that we want to test. When those methods access the internal behavior of our persistence layer, our mock answers with the methods we stubbed. The mock merely fills in the center of the cake.

describe Oven::Behavior do
  describe "preheating" do
    before(:each) do
      @latch = mock("latch")
      @oven = mock("Oven", :latch => @latch).extend(Oven::Behavior)
    end

    it "isn't locked when temperature setting is less than 500" do
      @oven.stub!(:temp_setting).and_return(350)
      @latch.should_receive(:engage!).never
      @oven.check_lock
    end

    it "isn't locked when temperature setting is 500" do
      @oven.stub!(:temp_setting).and_return(500)
      @latch.should_receive(:engage!).never
      @oven.check_lock
    end

    it "is locked when temperature setting is greater than 500" do
      @oven.stub!(:temp_setting).and_return(505)
      @latch.should_receive(:engage!).once
      @oven.check_lock
    end
  end
end

This cake mock style encourages another good testing practice: integration testing. When the model is completely mixed up with behavior, the need for integration testing is easier to ignore, but no less important. With separated behavior and persistence we can write simple integration tests that make sure the seam between the cake and the frosting is still intact.

Give Your Models Some Attention

When looking at models this way, whether you’re breaking them up in to cupcakes or separating the frosting, it’s easier to see the difference between behavior and persistence. If the logic can stand on its own, make a new class. If it’s more like an extension of the model, make a module and mix it back in. Allow ActiveRecord and DataMapper models to take care of what they do best without all the extra weight.

Take a few minutes and look at the models in your project. If the classes weren’t backed by the database, would you still have all that stuff jammed into one place? Are your models more complex than they need to be? If so… do something about it!

Correct, Beautiful, Fast (In That Order) »

Created at: 26.01.2010 20:00, source: RailsTips - Home, tagged: refactoring

One of the books I am occasionally grazing on is Beautiful Code. The title of this post is the title of Chapter 6. I’ll be honest, I only skimmed the chapter as one can only digest so much Java, but the title is spot on.

Step 1: Correct (With Tests)

I have been thinking exactly this as of late, though I will admit in no way as succinctly. First, you get it working. It does not matter if it is dirty. Rather, what matters is that it functions and is well tested.

If it works and it is not tested, you can never make it beautiful with confidence, so testing is very important to this step.

Step 2: Beautiful

Once your program is working and testing, make it beautiful. Leave the tests alone and head back into the code. Branch out and try new idioms that you have not tried before. Ensure that someone with no prior knowledge could jump in and know what is going on (without ridiculous comments all over).

Note that this step is not optional. Once your code is functional and well tested, you still have an obligation to make it beautiful. I think a lot of people skip this step. They think once it works and is tested, that is the end. Wrong.

Not only is this step the most fun, it also forces you to think through the code more. I often find edge cases during this step that I would never find if I stopped and correct and tested.

Step 3: Fast (optional)

If you pass these first two steps and you run into slowness at an unacceptable level, make it fast. The author puts it best in the summary at the end of the chapter, so I will quote them:

If there’s a moral to this story, it is this: do not let performance considerations stop you from doing what is right. You can always make the code faster with a little cleverness. You can rarely recover so easily from a bad design.

So true. I have never focused on speed in MongoMapper. No benchmarks to wow the noobs. No dramatic statements about performance. I have focused on cleaning up the code and making it easier for myself to maintain and others to contribute.

Not long ago, one of those contributors sent me a tiny patch (~30 lines) that improved overall performance by at least two, maybe three times what it was in 0.6.10. I do not think that it would have been that simple to to find and fix the performance issues if the code had been a mess.

Conclusion

• Correct: make it work and test it.
• Beautiful: refactor original code in a manner that others (and yourself down the road) can easily understand.
• Fast: often the first two steps leave code fast enough, but if they do not, make it faster.

Just In Time, Not Just In Case »

Created at: 24.01.2010 08:30, source: RailsTips - Home, tagged: refactoring

Something that has finally become a habit for me is adding code when it is needed, not in case it is needed. Often times we “think” we are going to need something, so we add the code to support it. What happens most often with that code is it sits and rots. It adds bloat and weight to your program that is not needed.

Example

I like to see examples first, so I will start with one. In Harmony, sites have users. I had a method in the site model named add_user.

class Site
  def add_user(user)
    user_id = user.is_a?(User) ? user.id : user
    # add user code here, removed for clarity
  end
end

Some might look at that and think it is a perfectly fine method, but I would disagree. A quick search through my code revealed that I was never passing a user id. Every time I called the method, I passed in a user as an argument. That was the first smell.

The second, which may be for another article is that I used is_a?. We are programming in Ruby folks. It is always better to duck type and ask respond_to? than is_a?. The difference is asking an object what it is, instead of asking an object what it can do.

Having add_user take either a user or a user id meant twice as many tests and an extra line of code. I killed the is_a? ternary line and the accompanying tests and went on my merry way.

How to Avoid Just in Case

The best way that I have found to avoid “just in case” is to test first. When I write my tests first and start with the most basic test I can think of and go from there, my code tends to only be as complex as the test requires. Whenever I begin with program code instead of test code, I end up with “just in case” features like the one above.

This might seem small as it was only one line and it was well tested, but this was only one small example. Cases like this multiply as the program grows and the “real” code begins to get obfuscated behind the cases that might happen someday.

The moral of the story is a line removed is a line that will never have bugs down the road.

*To give credit where I believe credit is due, I am 99% positive that Jamis Buck said “Just in time, not just in case.” at a conference. Could not find a reference though.