I'm a Floridian web engineer who planted his roots in Paris. Some of my favorite past-times are analyzing and optimizing development processes, building solid APIs, mentoring junior devs, and long walks on the beach.

Control your points of modification 10-22-2017

I was reviewing a controller the other day and I came across a piece of code in rails that modified incoming params in multiple spots. What follows is a contrived example of a solution implemented to handle a date that had to have a different format on the front-end and so had to be parsed according to the user's chosen date format.


class ShoesController < AdminController

    def update
        parse_and_format_date
        if shoe.update(shoe_params)
            redirect_to shoe_show_path
        else
            redirect_to shoe_edit_path
        end        
    end

    def shoe_params
        params.require(:shoe).permit(:buy_date, :title, :id)
    end

    def parse_and_format_date
        params[:shoe][:buy_date] = DateTime.strptime(params[:shoe][:buy_date], user.date_format)
    end
end

The problem I had with this controller is that we are modifying the params in multiple places, first in parse_and_format_dateand then in shoe_params. Also, we aren't particularly clear about it. Without looking into parse_and_format_date it is not immediately clear that it modifies the params.

For me, this is a major no-no but I had trouble articulating why. The only real answer that I could come up with was that debugging later I would have to ask myself where the changes to the params was happening, and the more places I put the changes, the more places I would have to hunt down to ensure that I was not making a bug. Also, parse_and_format_date does not clearly show that it updates the params object. In this file, it's not a big deal, but the bigger the file, the worse it gets, and if you add some concerns the next person to read this code will start scratching their head.

Source of truth

The shoe_params method should appear to be frozen in time. I should not be able to call it in multiple different places in my update method and get a different result. What's more is that the manipulations being done to the params variable should not be so heavy that they can't be done repeatedly if necessary. Also, strong parameters are not usually called multiple times in an action, so it's not crazy to think that it can contain these transforms.

If we made parse_and_format_date idempotent then we could do something like this:

def shoe_params
    params[:shoe][:buy_date] = parse_and_format_date
    params.require(:shoe).permit(:buy_date, :title, :id)
end

def parse_and_format_date
    @parsed_date ||= DateTime.strptime(params[:shoe][:buy_date], user.date_format)
end

In that situation, we would always now what shoe_params contains and it would be clear at any point of any action we call in our controller.

Exceptional code 01-22-2017

When I started programming, I never gave much thought to exceptions. I did not realize the benefit. If you write mainly interpreted scripts, they may seem like any other error that explodes during execution. Of course the difference, is that they are controlled explosions.

Some of the primary benefits are that they:

  • Allow you to expect some sort of known potential failure.
  • Allow you to know where the failure came from.
  • Allow you to fail gracefully.

The bad and the ugly

One of the worst things that I've seen is what follows:


def source
  begin
    risky_business
  rescue 
    nil
  end
end

The problem here, is that instead of handling failure, we just ignore it. There is no way to know what happens or why it happens. If risky_business normally returns nil, we may also think that function has passed when it really fails.

The next snippit is better, but still not without its problems.


def source
  raise "The source of all your problems"
end

def bad_exception_catcher
  begin
    source
  rescue Exception => e
    clean_up_the_stuffs
    raise e.message
  end
end

The problem here is a little more subtle. It is clear that we are taking advantage of being able to clean up problem code, however, there is one key problem. We are creating a new exception when we "raise e.message". This means that later in the logs, this call will be the source of the backtrace and not the original source function. If our source function is just one line, then there is no problem, but the more complicated that source function is, the more difficult it will be to pinpoint the problem.

A way to get around this problem, is to just reraise the original exception. In that way we will keep the original backtrace history.


def bad_exception_catcher
  begin
    source
  rescue Exception => e
    clean_up_the_stuffs
    raise e
  end
end

Testing

Typically, exceptions are something that you can and should test for. Also, if you are using ruby and something like rspec, exception testing is supported out of the box.


it "should raise error if user has no email" do
  user = OpenStruct.new(email: nil, name: 'A. Person', uid: 10244201)
  FbGraph2::User.stubs(:me).returns(user)
  expect { user = User.facebook_get_user('access_token') }.to raise_exception(
    LoginException, 'Please use an account with a valid email'
  )
end

In testing for specific exceptions, you can also assure that you don't get unexpected exceptions.

Definition vs Function/Methods Style 10-15-2011

I was recently reading The Rails 3 way by Obie Fernandez when I came across a section of the book that was talking about how Ruby allows for a definition-style as opposed to a funtion-style. That is to say, using a function call without including the open and closing parentheses. Typically, I like using functions without parentheses because I feel that the function looks more clear.

I much prefer to see:


link_to "Some page", some_page_path

instead of:


link_to( "Some page", some_page_path )

Especially in a haml template, the first style looks nicer. However, "The Rails 3 way" mentions that one of the key benefits parenthesis-less function calls offer is macro-like feeling and should be a signifier that this call will affect other things and not act directly. Indeed when you look at the following code:


class Client < ActiveRecord::Base 
  has_many :billing_codes
  has_many :billable_weeks
  has_many :timesheets, :through => :billable_weeks
end

class Client < ActiveRecord::Base
  has_many(:billing_codes)
  has_many(:billable_weeks)
  has_many(:timesheets, :through => :billable_weeks)
end

You can see that the first one looks cleaner. It also looks more like a declaration. Indeed, the has_many function alters the object of the class that calls it giving all of it's instantiated objects a slew of other functions. This theme happens over and over again in the Rails framework. The filters that are added to controllers, such as before_filter and after_filter, will alter the functionality of actions that get called.

Ambiguity

The problem with the definition-style method is when there are nested functions which take parameters. For instance, if the function for generating the page for some_page requires parameters as well:


some_page_path( :user_id => current_user.id, :name => 'derp' ) # calls the path for some page

This will cause problems when the following call is made:


link_to "Some page", some_page_path :user_id => current_user.id, :name => 'derp'

In the link_to call, it can't be determined whether the :name parameter should be part of the params for link_to or part of the params for some_page_path.

Since consistency is always preferred, it is better to always use parentheses with function calls only leave them off only when the function is being used to alter the class in some way.

Tags: style ruby Rails

Names n Stuff 09-18-2011

A few days ago, where I work, there was an argument over whether the following function should exist:


def is_premium_or_has_receipt_for?(product)
  self.premium_member? || self.store.has_receipt?(product)
end

I had a hard time explaining myself as to why I thought the function should not exist. At first all I could think of was, "This seems like a bad idea." Then I ran into a similar piece of code today and it became clear to me.

The function should exist, but there is a problem with the name. It simply restates what the logic is. When writing functions one of the benefits is that it allows you to group a bunch of smaller ideas into a bigger one. In this scenario we are merely re-writing an if clause in a prettier fashion; But the lines of code together are conveying a higher level idea. What the function is actually saying is "return true if the user has access to the following product".

So the function should look something like:


can_use_product? product
Tags: ruby

Testing File operations 03-24-2011

I've recently been working on a gem that will integrate git and pivotal into my current command-line workflow. I'll talk more about that in a future post, but check it out if you're interested. As I've been working on it, I have become increasingly aware how awesome mocking and stubbing are. I've been using mocha(gem install mocha #bros) for all of my needs and after a little playing around it has made me a very happy person. I haven't had much use for either until this gem I've been working on. The gem does more file operations that I normally do since most web stuff is heavier on the db action.

On this project I ran into an issue where I wasn't sure how to test the following code.


def load_project_id
  File.open('.git/config') do |file| 
    file.each_line do |line| 
      if line.slice('[pivotal]')
        return file.gets.tr("\s\t\n",'').split(' = ').last
      end
    end
  end
end

For this chunk of code, I wanted to spoof the variable 'file' so that I could make sure that everything in the File.open block does what it needs to. At first I thought that there might be a way to stub the variable that comes in to the block, but it ends up there is a more straight forward thing to do. I ended up turning File.open(".git/config") into File.open(git_config) where git_config is a method that determines the git config directory based on the current working directory. This way I can test this method by stubbing the git_config method with some file I've put in the /tmp directory and keep my config files clean.