The C42 guide to good code: 1 - Code Smells


This post is the first of a line of posts that are a guide to what we believe is good code. This first one is all about Code Smells. These are indicators that your code is hiding a deeper problem.


Law of Demeter

The formal definition for the law of demeter can be found on Wikipedia, but we will summarise to say that method calls in the following form are a really bad idea.

dog.legs.walk!

Instead, create a delegator in the parent class

class Dog
  def move!
    legs.walk!
  end
end

dog.move!

This allows Dog to control how it walks. If someday, we invent a robot dog that walks using a hover device, then we need only to change the implementation of walk!, that is internal to dog, and the external world need not do

dog.legs.walk! if dog.normal?
dog.hover_craft.hover! if dog.robot?

More formally:

Every method M on an object O should only access these types of methods:

  • methods on O itself
  • methods on M's parameters
  • methods on any objects created/instantiated within M
  • methods on O's component objects

Tell Don't Ask

When you want an object to perform an action, never ask the object for information about it's state, make a decision based on the information, and then tell the object to perform the appropriate action.

The responsibility of how to perform the action should reside in that object, not the caller. In short, tell the object what you want it to do, and let that object worry about how to do it.

Bad

if account.balance >= amount
  account.deduct(amount)
else
  raise NotEnoughBalance
end

class Account
  def deduct(amount)
    balance -= amount
  end
end

Good

account.deduct(amount)

class Account
  def deduct(amount)
    raise NotEnoughBalance unless balance >= amount
    balance -= amount
  end
end

Read further on c2.


Law of 7

If a method is over 7 lines of code, more often than not the method would be doing more than what it should, and a candidate for refactoring. At the very least, pull out a private method to improve readability of the large method.

Having a single level of indentation makes it obvious what every method does at a glance. For example:

def self.import_from_csv(csv)
  csv.each_line do |line|
    if line["type"] == "distributer"
      Document.new(line["foo"], line["retail_price"])
    else
      Document.new(line["foo"], line["wholesale_price"])
    end
  end
end

can be replaced with

def self.import_line(line)
  if line["type"] == "distributer"
    Document.new(line["foo"], line["retail_price"])
  else
    Document.new(line["foo"], line["wholesale_price"])
  end
end

def self.import_from_csv(csv)
  csv.each_line do |line|
    import_line(line)
  end
end


Polymorphism over if conditions and switch / case

Avoid if conditions & switch / cases whenever possible, instead preferring polymorphism over them.

Whenever possible, try to isolate the place where switch/cases are used to a single method where appropriate strategy (Strategy Pattern) is created to care of the behaviour, and allow polymorphism to take over.

def expiry_date
  case plan_type
  when 'subscription' then next_month
  when 'prepaid' then never
  end
end

def monthly_charge
  case plan_type
  when 'subscription' then 300
  when 'prepaid' then 0
  end
end

def charge_per_rental
  case plan_type
  when 'subscription' then 0
  when 'prepaid' then 100
  end
end

becomes

class SubscriptionPlan
  attributes :expiration_date => never, :monthly_charge => 300, :charge_per_rental => 0
end

class PrepaidPlan
  attributes :expiration_date => never, :monthly_charge => 300, :charge_per_rental => 0
end

def plan_type
  case plan_type
  when 'subscription' then SubscriptionPlan.new
  when 'prepaid' then PrepaidPlan.new
  end
end

def expiry_date
  plan_type.expiry_date
end

def monthly_charge
  plan_type.monthly_charge
end

def charge_per_rental
  plan_type.charge_per_rental
end


If you must use if conditions, then use it in one of the forms:

def sales_tax
  return 0 if purchaser.exempt_from_taxes?
  price * .10
end

def car.brake
  switch_on_brake_light
  unless stopped?
    slow_down
  end
end


Contributors:

  • Tejas Dinkar
  • Niranjan Paranjape
  • Smit Shah
  • Srihari Sriraman

Agile delivery needs design facilitators

With the growing awareness of the need to integrate user experience design into agile delivery, there is now a better recognition of the role played by design facilitators in enabling good design faster.  I wrote a blog post to share my insights on what should be a design facilitator's playbook:

A design facilitator's playbook

Would love to hear your thoughts and insights!

/

How would you describe RubyMonk's interface?

We receive a lot of love from our users about RubyMonk's visual design. And while we agree -- it's super cute! -- the site's visuals are hardly its interface. The "user interface" for RubyMonk is really the boundary between the student and the mentor.

How do we evaluate your code? How do we decide whether it is correct or not? These are the interesting questions in the user interaction. And, as emerges in the design of any product, these questions highlight the necessary unification of form and function, engineering and design, purpose and path. When you boil it down, the purpose of RubyMonk is to automate the teacher. On some level, this goal might seem akin to beating the Turing Test. In its current incarnation things are much simpler, of course.

I've explored this idea a bit in my post "defining the interface for rubymonk". Take a look and check back soon as I take this idea forward to discuss the yin and yang of tests and the boundaries they exercise.

/

Why you should respect deprecation warnings

At C42, I have worked on multiple Ruby on Rails migration projects. A simple task of upgrading an application from Rails 2.3 to Rails 3, can take anywhere from two days to three months. After such experiences, I have developed a small set of rules to make sure, my projects don't run into these two to three months long upgrades. A blog post by me on, how a simple discipline of fixing deprecation warnings can help you maintain your ruby applications in the long run.

/

C42 Launchpad - bridging the gap between hackers and the industry

At C42 Engineering, we spend about 108 man hours for every good developer we hire. For a small product company, I would rather spend that time developing features. We've always wanted to hire more awesome developers with less effort, and I think we may have a way to do just that.

We receive about 30-40 resumes a month. Our interview process, although time consuming, allows us to know the candidate better. Even before we call them to our office, we spend about half an hour with each candidate on the phone. Every face to face interview takes about 6 hours. After this extensive investment of time and energy, we consider ourselves lucky if we hire 1 developer a month, i.e. conversion rate of 2.5%, something we always wanted to improve.

While speaking with other startups at RubyConf India 2012, I realized, rather, that a 2.5% conversion rate is extremely high. I have heard horror stories of interviewing 150 candidates to hire one or two developers. The point is, businesses spend enormous amounts of energy and money in the hiring market with returns that are too low to be considered acceptable.

If we ask any consultancy or product company what skill set they would like prospective hires to have, the following common points come up over and over:

  • Understands what is good code
  • Understands maintainability
  • Understands Object Oriented Programming principles
  • Familiarity with TDD
  • Deployments and DevOps
  • Awareness of standards and conventions

In summary, every business needs developers who care about good code and are capabale of being productive from day one.

The missing rockstars

Most engineers who don't make it through our interview process are asked to apply again in six months. We don't do it just out of politeness, but because we actually believe that they can in time grow to become the capable developers we need.

These developers have the potential and inclination to become experts, but lack the correct exposure and guidance. Focused training on OO, TDD and Agile, with discussions around nuances of development practices and principles will convert these talented folk into the rockstars that every company seeks.

As always, we gave this feedback to the candidates in question. A consistent pattern emerged where candidates would get back to us asking for more information on how they could go about learning the requisite skills.

Unfortunately, there are no organizations like hacker/school in India, places that expose programmers to the art of software craftsmanship. Be it universities or university-like classroom training programs, they just can't keep up with our fast moving industry. The gap is already large and growing every day.

Most lot of these skills are best learned by working closely with experienced developers and following practices for an extended duration. A 2 or 3 day workshop is useful for knowledge transfer, but it's impossible to develop discipline or grok the principles underlying the methodology in that short a duration.

The launchpad

C42 Launchpad hopes to be an answer to this problem. A 4-6 weeks full time hands-on workshop that will cover everything C42 Engineering expects a developer to know before she starts working on a codebase. We will have different batches for fresh graduates and for experienced developers. A rough sketch of the program for freshers:

  • Programming 101
    • Introduction to Ruby
    • Introduction to Git
    • OO Design Principles
    • OO Patterns
      • Creational
      • Structural
      • Behavioral
    • Programming/Design anit-patterns
  • Web Application Development
    • Working within browsers
      • Introduction to HTML & DOM
      • Introduction to CSS
      • Introduction to Javascript and Prototype based programming
    • Ruby on Rails 101
      • Journey through Rails components
      • TDD with rails
  • Introduction to Agile
    • Agile principles
    • Planning and estimation
    • Importance of Automated Testing and Continuous Integration
    • Pair programming, TDD, Simple Design, Refactoring
    • Basics of XP and Scrum
  • 2 Weeks of Ruby on Rails project development in teams, following all the methodologies and principles mentioned above.

The course structure as you can see is still being finalized, but the guiding principle is simple. Provide opportunity and inspiration to the willing engineers to hone their skills.

Bridging the gap

Lots of potential hackers join the Indian IT industry with big dreams. Due to lack of inspiration and opportunities, they stop caring about programming and become an hourly billable unit in an excel sheet somewhere. We want to show them what real development feels like, enable them with both the tools and the practices to become productive and enjoy coding as we do.

Providing this training is pointless if they don't get to work at places where these skills are appreciated. A number of startups in India are being limited by the shortage of technical talent. Businesses are unable to grow as they struggle to hire talented programmers that can build high quality, maintainable codebases that scale and are easy to change. If their codebases are in good hands, business owners are free to concentrate on the non-technical aspects that are often extremely critical for the success of the business.

We plan to connect developers, who are looking for new challenges, with companies who need and appreciate the value they bring to the table.

 

Aakash is VP Consulting at C42 Engineering. If you liked this post, please consider...

/