A Question of Style

Readability / Maintainability

Matz gave us a wonderful language

Clean syntax

No statement terminators required

Flexible

Why do programmers have to come along and...?

ReadabilitymaintainabilityrEaDabiliTymAIntaiNaBIlityreadabilityMaintainabilityreadabilitymaintainabilityreadabilitymaintainability

An Anecdote

Early in the morning after a deply
A big client calls and says "We can't log on"
It's just Mike and me
Game on...

  def valid_role_nbr?(role_nbr)
    role_nbr.present? && role_nbr =~ /#{ROLE_NBR_RE}/ && 
            role_nbr.split (',').length == Permission.where("access in (?)", role_nbr.split ...
  end

Narrowed it down to this.

def valid_role_nbr?(role_nbr)
    role_nbr.present? && role_nbr =~  /#{ROLE_NBR_RE}/ &&
            role_nbr.split(',').length == Permission.where("access in (?)", role_nbr.split(',').map {|p| p = "facility_level_admin" if p == "admin"; p.strip}).count
  end

Changed the settings on my editor and I see...

Oh yeah, that's a lot better...

def valid_role_nbr?(role_nbr)
    role_nbr.present? && role_nbr =~  /#{ROLE_NBR_RE}/ &&
            role_nbr.split(',').length == Permission.where("access in (?)", role_nbr.split(',').map {|p| p = p.strip.downcase; p = "facility_level_admin" if p == "admin"; p}).count
  end

The fix

That's better, isn't it?

    user.permissions = Permission.where("access in (?)", @params[:role_nbr].split ...

Oops, missed a spot...

    user.permissions = Permission.where("access in (?)", @params[:role_nbr].split(',').map{ |r| r = r.strip.downcase; r = "facility_level_admin" if r == "admin"; r })

Deja Vu all over again...

Would the defect have occurred if we'd taken the time to make the code readable / maintainable?

The End

Didn't lose the client...

What about one-line methods?

def say_hello
  puts "Hello"
end

def say_hello() puts "Hello" end

def say_hello; puts "Hello" end

def say_hello()
  puts "Hello"
end

def say_hello_and_shut_the_door; self.door_handles.map{|d|d=="knob"?"open":"shut" }.join(" and ") end

See anything easy to read?

VS.

def invalid_content?(record) record.errors [:identifier].empty? && !record[:identifier].blank? && record[:identifier] !~ /\A[a-zA-Z0-9\-\.^]+\z/ end

Seriously?

What about semicolons?

b = coll.inject([]) do | a, v |
  a << v unless v < 1
  a
end

b=coll.inject([]){|a,v|a<<v unless v<1;a}

VS.

Or how about something like this...

a=0;b=1;c=0;d=5;e=12

Don't like anecdotal evidence? How about some facts?

num_lines                    = 48887
num_lines_over_80_characters = 5474
max_line_length              = 1205
num_lines_with_semis         = 773
max_num_semis_per_line       = 30
num_one_line_methods         = 388
long_one_line_methods        = 99

Why is this important?

Because you might end up being the poor dumb schmuck that has to try to unravel this garbage at 7:30AM after a deploy with a big client breathing down your neck.

Is this the way A3 is going to be?

Style Guide

1. No One Line Methods

The Style Guide (continued)

2. No lines of code longer than 80 characters

Don't take my word for it.

Listen to our good friend Douglas Crockford on "Understanding Good Programming Style"