Unconventional tools, #2

Hi All;

Now, we’ve all seen code that at least appears to work, but is very poorly written. You might even say the code “stinks”. Now, it can be really hard to detect code smells in your own code. A major air freshener company speaks of people going “noseblind” to smells. The old solution to this problem was to have code reviews. This never works. People gloss over problems, the reviews take forever, and feelings can be hurt or axes can be ground. What is needed is an objective, easy-to-perform, review of the code. What is needed is an automated tool!

For me, programming in free ruby, the tool is called… reek!

Now you may think: “Why do I need a tool to complain about my working code?” I must admit, I sometimes think that too. The answer to that question is that whenever I dig into the code and clean up the smells, the result is almost always much better code. Now, I know that’s a serious claim, so I am going to back it up with a real example from my own code. The following bit of code is used to process snippets of ruby code embedded in strings and surrounded by {{  }} structures. They generally go by the name “handlebars”. Here is our starting point:

#Process a string with code embedded in handlebars and 
#backslash quotes.
def eval_handlebars(in_str)
  out_str = ""

  loop do
    pre_match, match, in_str = in_str.partition(/{{.*?}}/m)
    out_str << pre_match
    return out_str.gsub(/\\\S/) {|found| found[1]} if match.empty?

    code = match[2...-2]
    silent = code.end_with?("#")
    result = instance_eval(code)
    out_str << result.to_s unless silent
  end
end

 When I ran reek against this code, it made the following observations:

lib/mysh/user_input/handlebars.rb -- 2 warnings:
 [25, 27, 32]:FeatureEnvy: Object#eval_handlebars refers to
  out_str more than self (maybe move it to another class?)
 [19]:TooManyStatements: Object#eval_handlebars has approx 10 
  statements
2 total warnings

Well, the code is pretty ugly, so perhaps this is not surprising. So; how to proceed? The first clue comes from the comment line.

#Process a string with code embedded in handlebars and 
#backslash quotes.

This method is doing two distinct things! Maybe it should be two distinct methods? So here’s the first fixup:

 
#Process a string with code embedded in handlebars and 
#backslash quotes.
def eval_handlebars(str)
  do_process_handlebars(str).gsub(/\\\S/) {|found| found[1]}
end

private

#Process a string with code embedded in handlebars.
def do_process_handlebars(in_str)
  out_str = ""

  loop do
    pre_match, match, in_str = in_str.partition(/{{.*?}}/m)
    out_str << pre_match
    return out_str if match.empty?

    code = match[2...-2]
    silent = code.end_with?("#")
    result = instance_eval(code)
    out_str << result.to_s unless silent
  end
end

The new method do_process_handlebars just does handlebars while the original method takes that result and processes backslash quotes. So what does reek think of the code now?

lib/mysh/user_input/handlebars.rb -- 1 warning:
 [26]:TooManyStatements: Object#do_process_handlebars has approx 9 
  statements
1 total warning

This is a lot better, but now my new method is too long! What to do? I could ignore the problem; I could tell reek to just ignore the problem; or I could mashup the code and use sneaky tricks to make it use fewer lines. None of these are good choices here. Instead, let’s really look at what the code is doing. We see a loop, processing a string by repeatedly looking for a regular expression and then performing a substitution of the found text with new text. Wow! Did I really write that code? What we have here is a kludgy reinvention of the gsub method! The very same method already used to perform the backslash quoting. Zounds!

OK; so let’s see what happens when we replace the kludge of gsub with that actual gsub method:

#Process a string with code embedded in handlebars.
def do_process_handlebars(str)
  str.gsub(/{{.*?}}/m) do |match|
    code = match[2...-2]
    silent = code.end_with?("#")
    result = instance_eval(code)

    (result unless silent).to_s
  end
end

 Wow! That code is MUCH better looking! What does reek think about it?

0 total warnings

No more code smells found, well at least by the reek tool. I did make one further change. I corrected the ambiguous top comment line:

#Process a string with backslash quotes and code embedded 
#in handlebars.

It would seem that even automated code scanning tools do not check for poorly written comments.

So, I think it is pretty clear that the new code is much better than the original. I can tell you that it also runs faster and uses less memory. Can we draw a conclusion from all of this? How about:

When things smell bad, put away the scented air spray and just clean up the mess!

Best regards;

Peter Camilleri (aka Squidly Jones)

Notes:

  1. In the quest to write better code, I am inspired by Sandi Metz. An awesome video by her on the matter of smelly code is Get a Whiff of This by Sandi Metz.
  2. Some code was slightly reformatted to fit into the blog post.

 

 

 

 

The best news I’ve heard in a LONG time!

Hello All!

One of the most pernicious and damaging mistakes, injurious to world progress, has to have been the decision to allow the patenting of software. These patents have served to obstruct progress, destroy innovation, and fatten the wallets of lawyers and greedy patent trolls.

Now, to some, software patents are an awesome idea. If your goal is to provide more work for legal departments at large companies or you need to attack a competitor with the gaul of being better than you, then software patents are for you! They’re also great at destroying small, upstart companies with all of their “new ideas” stuff. Heck, even if the patent does NOT apply, prolonged legal proceedings can serve just as well.

Seriously, software patents are of dubious virtue at the best of times. Now finally, after many years of suffering under the lawyer’s savage yoke, there may be some hope:

Here’s Why Software Patents Are in Peril After the Intellectual Ventures Ruling

This a great article with a video for the tl;dr; crowd. Still, I’d like to focus on a little excerpt from the article that is, I think, the heart of the matter:

Pointing out that intellectual property monopolies can limit free speech, Mayer notes that copyright law has built-in First Amendment protections such as “fair use” and that patent law must include similar safeguards. He suggests that the safeguard comes in the form of a part of the Patent Act, known as “Section 101,” which says some things—including abstract ideas—simply can’t be patented in the first place.

I really hope that this is the beginning of the end for software patents! I really do! I look forward to the day when they and the profits of the trolls are all dead and buried. Just don’t expect me to bring flowers to the grave site.

Best regards;

Peter Camilleri (aka Squidly Jones)