Reek vs RuboCop

Or code linters / analysers for Ruby

I initially stumbled upon RuboCop, a code quality analyzer for Ruby by chance. My interest was immediately piqued.

After trying it out but before committing to use it I searched for alternatives and found Reek, a code smell detector for Ruby.

Both project analyse Ruby source code and give you a series of warnings. Both can integrate with Rake and both have plugins or third parties that allow them to integrate with editors like Vim.

My first instinct was to search for “reek vs rubocop”, a search that led to no results. I was expecting to see some blog posts comparing the two.

I then proceeded to try them both on the same set of files. I then realised they are different programs that can complement each other rather then replace each other. While they will sometimes overlap they each have a different focus.

RuboCop

RuboCop’s main focus is on coding style and adherence to the Ruby Style Guide. Typical warnings will be about:

  • lines being too long
  • methods having too many lines
  • following Ruby idioms prescribed by the guide, like which types of quote to use or preferring each over for for iteration.

Feature wise RuboCop has more features. It has many output formatters and more options to work with. The output is also in color which is nice. Oh and I really like the name, seriously 🙂

Reek

Reek for it’s part will focus on code smells that could lead to refactorings and that aren’t style related.

Examples include:

  • methods with too many parameters
  • code repetition that could be extracted in a new method
  • unused parameters

Like RuboCop, Reek can be configured via a config file, but overall RuboCop has a bigger feature set (which may or may not be a good thing).

While RuboCop will help you enforce style guidelines on your project, Reek will allow you to find bad code that can be made better through refactoring.

Overlap

Both will give out a warning if a class has no class level comment. Both will also give a warning if there are too many lines/statements in a method.

RuboCop will count the number of lines in a method and warn if that number is over 10, Reek on the other hand will count the number of statements in a method.

There are also other overlaps.

They will usually give you a varying number of warnings for the same file. This has a tendency to go over both ways but in the end which one will give you the most warnings will depend on your coding style.

Example

Here is the default output for both linters on the following file:

class Particle
  attr_reader :stability_radius

  def initialize(stability_radius=1)
    @stability_radius = stability_radius
  end

  def drop(height_map, x, y, size_x)
    drop_point = x + y * size_x

    if height_map[drop_point] == 0
      height_map[drop_point] += 1
    else
      agitate(height_map, x, y, size_x, drop_point)
    end
  end

  private

  def agitate(height_map, x, y, size_x, drop_point)
    # compile list of lower level neighbors
    lower_neighbors = []

    for index_x in x - @stability_radius..x + @stability_radius
      for index_y in y - @stability_radius..y + @stability_radius
        check_and_add_neighbor(index_x, index_y, size_x, lower_neighbors, drop_point, height_map)
      end
    end

    if (lower_neighbors.length == 0) then
      # no lower neighbors, leave particle on drop point
      height_map[drop_point] += 1
    else
      # randomly select one of these neighbors and put particle to this neighbor
      selected = lower_neighbors.shuffle.first
      height_map[selected] += 1
    end
  end

  def check_and_add_neighbor(x, y, size_x, lower_neighbors, drop_point, height_map)
    unless height_map[x + y * size_x].nil?
      if height_map[x + y * size_x] < height_map[drop_point]
        lower_neighbors.push(x + y * size_x)
      end
    end
  end
end

Running:

rubocop particle.rb

Will produce the following output:

Inspecting 1 file
C

Offences:

particle.rb:1:1: C: Missing top-level class documentation comment.
class Particle
^^^^^
particle.rb:4:34: C: Surrounding space missing in default value assignment.
def initialize(stability_radius=1)
^
particle.rb:20:3: C: Method has too many lines. [12/10]
def agitate(height_map, x, y, size_x, drop_point)
^^^
particle.rb:24:5: C: Prefer *each* over *for*.
for index_x in x - @stability_radius..x + @stability_radius
^^^
particle.rb:25:7: C: Prefer *each* over *for*.
for index_y in y - @stability_radius..y + @stability_radius
^^^
particle.rb:26:80: C: Line is too long. [97/79]
check_and_add_neighbor(index_x, index_y, size_x, lower_neighbors, drop_point, height_map)
^^^^^^^^^^^^^^^^^^
particle.rb:30:5: C: Never use then for multi-line if/unless.
if (lower_neighbors.length == 0) then
^^^
particle.rb:30:8: C: Don't use parentheses around the condition of an if/unless/while/until
if (lower_neighbors.length == 0) then
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
particle.rb:34:80: C: Line is too long. [80/79]
# randomly select one of these neighbors and put particle to this neighbor
^
particle.rb:40:29: C: Avoid parameter lists longer than 5 parameters.
def check_and_add_neighbor(x, y, size_x, lower_neighbors, drop_point, height_map)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
particle.rb:40:80: C: Line is too long. [83/79]
def check_and_add_neighbor(x, y, size_x, lower_neighbors, drop_point, height_map)
^^^^

1 file inspected, 11 offences detected

Similarly running :

reek particle.rb

Will produce :

particle.rb -- 20 warnings:
Particle has no descriptive comment (IrresponsibleModule)
Particle takes parameters [height_map, size_x, x, y] to 3 methods (DataClump)
Particle#agitate has 5 parameters (LongParameterList)
Particle#agitate has the parameter name 'x' (UncommunicativeParameterName)
Particle#agitate has the parameter name 'y' (UncommunicativeParameterName)
Particle#check_and_add_neighbor calls (x + (y * size_x)) 3 times (DuplicateMethodCall)
Particle#check_and_add_neighbor calls (y * size_x) 3 times (DuplicateMethodCall)
Particle#check_and_add_neighbor calls height_map[(x + (y * size_x))] twice (DuplicateMethodCall)
Particle#check_and_add_neighbor doesn't depend on instance state (UtilityFunction)
Particle#check_and_add_neighbor has 6 parameters (LongParameterList)
Particle#check_and_add_neighbor has the parameter name 'x' (UncommunicativeParameterName)
Particle#check_and_add_neighbor has the parameter name 'y' (UncommunicativeParameterName)
Particle#check_and_add_neighbor performs a nil-check. (NilCheck)
Particle#check_and_add_neighbor refers to height_map more than self (FeatureEnvy)
Particle#check_and_add_neighbor refers to x more than self (FeatureEnvy)
Particle#check_and_add_neighbor refers to y more than self (FeatureEnvy)
Particle#drop has 4 parameters (LongParameterList)
Particle#drop has the parameter name 'x' (UncommunicativeParameterName)
Particle#drop has the parameter name 'y' (UncommunicativeParameterName)
Particle#drop refers to height_map more than self (FeatureEnvy)

You can get a good feel for the differences between the two by scanning the types of errors produced.

In practice

While I am in favor of using such tools I also believe good judgement always trumps blindly following rules. It’s best to carefully select a subset of rules to adhere by rather than simply try to apply everyone of them. Also be ready to make exceptions even on those rules you have selected.

Trust developers to choose the best formatting for the code they are currently writing even if it goes against your general guidelines. They know more about the current context they are in than anyone.

Hire talent, not five years with Java

Too often we see job requirements for development positions with a long series of bullet points:

  • General Tech X everyone has heard about, 5 years
  • Specific Tech Y that came out last year, 3 years
  • Specific Framework Z that few people use and only takes 5 days to learn, 5 years
  • General thing you were taught in college, 10 years

I can understand this practice from a recruiter’s or HR’s point of view. It is difficult to gauge the skill level of something you have no clue about and you want to filter out as much candidates as possible before having them for an interview. But I do not feel the practice is worthwhile, it’s just lazy.

The skills dilemma

First of all, a specific number of years does not imply skill. You could have written Java code for dead-simple CRUD programs for 5 years, not knowing 50% of what the average Java developer knows and still qualify.

The following phrase I encountered on Programmers StackExchange sums it up nicely:

five times one year of experience

Some people for various reasons will have matured in 5 years, others will have repeated the same experiences and be no farther after five years then they were after one.

Second, what does it mean when a candidate says he has 5 years of experience with said tech? Did he intensely use it for 5 years or did he first start using it five years ago? Is he even still using it? If he lists C#, ASP.Net and JavaScript for 5 years all for the the same job, did he do all equally or did he just do some light JS from time to time to complement his ASP.Net pages?

Finally, while general experience will teach a developer invaluable lessons, prolonged experience with a specific technology will net diminishing returns after a few years. As more and more features are added to a framework or language, experiences gained five or more years ago will have been with a product that was very different from what it is now. Thus some of these experiences might not even apply today.

The “we feel we need someone yesterday” mentality

What baffles me most is when I see this hard “requirement of experience with certain technologies” coming from someone amongst the more tech inclined crowd.

Hiring talent will be better in the long run. A talented individual will learn the tech faster than an average developer anyway, and when he does, he will be doing a better job. Some common reply to this is: “we need someone to be productive right now”.

If you feel you need someone to be productive right now and don’t care about hiring the best candidate for the long term, maybe you should be hiring a temporary consultant rather than a permanent employee.

Besides, no matter how experienced with said technology, most people will need a ramp-up time to attain maximum productivity when joining an existing project. A smart person will be able to start mastering a new technology while learning all the other intricacies of the project at the same time.

I have seen a company hold out on hiring because they wanted someone with a very specific set of skills and level of experience and couldn’t find one. Suggestions to hire based on talent were dismissed with claims that they couldn’t afford the training time, they had to find someone who would be productive right now.

Months later, they still hadn’t filled the position. Sure, they had tried out some rare candidates who filled the prerequisites; X months with technology Y, X years with framework Z, but none had worked out.

Instead, had they hired someone with skills, intelligence and the right mind set, that person could have learned said framework and have been a productive member of the team by then.

In another situation, I remember doing a phone interview a few years ago for a company developing UI controls for a certain application framework. I was told I was a great candidate and they would have hired me if it wasn’t for my lack of experience with said application framework.

About two or three years later, said framework was pretty much discontinued. I wondered what that UI company was doing since it’s main products were UI controls for an abandoned application framework. Sure enough, they had converted their offerings to HTML 5 / CSS 3.

In such a situation it pays to have a team that is quick on it’s feet, learns quickly and stays up to date on various parallel technologies. Even if your current tech stack doesn’t get abandoned, having a team of quick-learning generalists will allow you to use the right tool for the job for each project, rather than make another Rails app because that’s all the team has ever did.

Conclusion

Smart, determined, result-driven people will get the job done. They’ll also turn around pretty quickly when you suddenly need to change framework, platform or language.

Why settle for someone who fills check boxes on a requirements list? Hire the best possible candidate. If you can do both, excellent, if not prioritize talent.

I’m not saying years of experience do not have a great value. Years of experience have great value. I’m saying talent trumps years of experience with a specific technology.

Choosing a BDD framework for .Net

tl;dr

Specs aren’t written by developers: choose between SpecFlow and NBehave.

Specs are written by developers: my personal recommendation NSpec. Runner up StoryQ.

Introduction

BDD or behavior driven development is the practice of writing an executable testable specification that describes the application’s behavior. This specification is often written in a fluent interface, a DSL or in plain English (or rather close to plain English).

While in TDD the focus is on writing tests that single out units of the application, BDD is focused on writing tests on the behavior of the application. This can also be tought as testing features of the application.

Before presenting the frameworks I will make a distinction between xBehave and xSpec style frameworks.

xBehave

xBehave frameworks are about writing user level stories in a form comprehensible by anyone. These stories can be written by the users themselves or by a group consisting of developers, users and testers.

These framework typically use a story defined in a DSL close to English and then map this story to a test written in code by the developers. Cucumber is a well known example of such a framework.

xSpec

xSpec frameworks are usually about developers writing tests in code using an approach that favors testing behavior and functionality. These tests are usually closer to unit tests in both appearance and granularity but feature some differences.

Both approaches could be combined on a single project. The user stories could be converted to xBehave tests, while the developers could adopt an xSpec framework to replace or complement xUnit tests.

Available frameworks

Here is a compilation of xSpec and xBehave BDD frameworks for .Net. This list is current as of the writing of this post and while I tried my best to find most frameworks, I realized while compiling this list that there are just too many, I am sure I must have missed a few.

By the way, it’s not an error in the table, I have found two frameworks named NSpec which appear to be totally independent. The first one listed is the most active and most popular. The second one is an older project that hasn’t seen any recent activity.

Name Project development still active Type License NuGet package
SpecFlow Yes xBehave BSD style Yes
specunit-net No xSpec Apache License 2.0 No
Machine.Specifications / MSpec Yes xSpec xUnit and MS-PL Yes
NSpec Yes xSpec MIT License Yes
NSpec (older project) No xSpec zlib/libpng license No
NBehave Yes xBehave BSD 3 Yes
StoryQ No *** MIT Yes
NSpecify No, seems never to have taken off. xSpec ? No
.NetSpec No xSpec None No
xbehave.net Yes xSpec MIT Yes
SubSpec No xSpec MS-PL Yes
NJasmine Yes xSpec MIT Yes
SpecsFor Yes xSpec None Yes
Behavioral No xSpec BSD Yes
NaturalSpec Yes xSpec MIT / MS-PL Yes
BDDish No xSpec None Yes

*** This framework is harder to classify. Based on my interpretation of xSpec/xBehave and my comprehension of the framework, I personally place it in a gray zone. They suggest to start with a plain text story but it isn’t directly used by the code. The specifications are directly in C# test code contrary to other more “pure” xBehave frameworks. It is a sort of hybrid and does tend to lean much more on the xSpec side than on the xBehave side.

Narrowing the selection

First of all, I would eliminate the following frameworks from investigation: specunit-net, NSpec (older project), NSpecify and .NetSpec as they are abandoned inactive projects with most never having really made it to “production status”.

NaturalSpec is in F# which is great for those doing F#. There is a link to a post on it’s GitHub page about how to use it with C# objects, but I would personally prefer a library that targets C# for a C# project.

I wasn’t able to see much activity for BDDish (last commit to GitHub more than 2 years ago, only 3 questions on StackOverflow, not much documentation or examples). I would also skip this one.

As an unrelated side-note, out of the remaining more serious contenders xbehave.net and SubSpec are strikingly similar. At first glance, both projects seem to be closely related. If you are considering one of these two, you might as well look into the other as well.

Making a selection for an xBehave framework

The first question you need to ask yourself is if you want to do xBehave style tests in a language closer to plain text, with specs that can be written by non developers.

If that is so, your choice is pretty slim. Your choice is between SpecFlow and NBehave.

StoryQ could also be considered if SpecFlow or NBehave are not satisfactory.

Both SpecFlow and NBehave use Gherkin (the DSL used by Cucumber) to write their specs. Both store specs in *.feature files. Both also use attributes on methods when writing tests.

[Given("I am not logged in")]
public void LogOut()
{

Both have documentation available on GitHub.

SpecFlow seems to have more documentation and it is also recommended in this StackOverflow question in the top two rated answers.

On the other hand the SpecFlow doc is pretty dry on code examples. I personally preferred NBehave’s documentation over SpecFlow.

Making a selection for an xSpec framework

If you do not need for your specs to be written in a plain text like format and want to save time and work use an xSpec framework. You won’t need to write the binding code and writing tests will be easier for those already familiar with an xUnit framework.

On the xSpec side, you have a lot of options.

I narrowed my search to the following frameworks: MSpec, NSpec, SpecsFor, StoryQ and xBehave.net.

The following table compares those frameworks by considering their total number of questions on StackOverflow. I also rated the official documentation on a scale of 1 to 3, with 1 being the poorest and 3 the best score. This rating is largely subjective and represents my appreciation of the available documentation. It should be noted I prefer a style which includes code samples and which gets you rapidly on your feet.

Name Number of questions on StackOverflow Documentation
MSpec 519* 1 / 3
NSpec 62 3 / 3
SpecsFor 5 3 / 3
StoryQ 36 3 / 3
xBehave.net 5 2 / 3

*: I added the number of questions for both MSpec and Machine.Specifications. Even if just looking at the number of questions for either MSpec or Machine.Specifications, it is the clear winner here.

My personal recommendation for an xSpec framework is NSpec.

After trying each of these frameworks for a simple two tests scenario here is my personal rating:

  1. NSpec
  2. StoryQ
  3. SpecsFor

Here is a code sample of two simple tests using NSpec:

namespace NSpecTests
{
    class TicTacToe_specifications : nspec
    {
        void given_a_new_board()
        {
            it["A new tic tac toe board is empty"] = () => ticTacToeBoard.IsEmpty.should_be_true();

            context["When placing a first X"] = () => 
            {
                before = () => ticTacToeBoard.PlaceXat(1);
                it["Board should not be empty"] = () => ticTacToeBoard.IsEmpty.should_be_false();
            };
        }
        TicTacToeBoard ticTacToeBoard = new TicTacToeBoard();
    }
}

And here is the output that will be generated using NSpecRunner, NSpec default test runner.

Output :
TicTacToe specifications
  given a new board
    A new tic tac toe board is empty
    When placing a first X
      Board should not be empty

2 Examples, 0 Failed, 0 Pending

Here are the two same tests and their output in StoryQ:

namespace StoryQTests
{   
    [TestClass]
    public class TicTacToe_specifications
    {
        TicTacToeBoard ticTacToeBoard;
        bool result;

        [TestMethod]
        public void given_a_new_board()
        {
            new Story("A new tic tac toe board is empty")
                .InOrderTo("to start a new game")
                .AsA("player")
                .IWant("to have an empty board")
                .WithScenario("new board")
                .Given(ANewBoard)
                .When(CheckingIfBoardIsEmpty)
                .Then(ItShouldBeTrue)
                .Execute();

            new Story("After placing a first X")
                .InOrderTo("to make my first move")
                .AsA("player")
                .IWant("to have a non empty board")
                .WithScenario("new board")
                .Given(ANewBoard)
                .And(PlacingAFirstX)
                .When(CheckingIfBoardIsEmpty)
                .Then(ItShouldBeFalse)
                .Execute();
        }
        
        public void ANewBoard() { ticTacToeBoard = new TicTacToeBoard(); }
        public void PlacingAFirstX() { ticTacToeBoard.PlaceXat(1); }
        public void CheckingIfBoardIsEmpty() { result = ticTacToeBoard.IsEmpty; }
        public void ItShouldBeTrue() { Assert.IsTrue(result); }
        public void ItShouldBeFalse() { Assert.IsFalse(result); }
    }
}

And the output:

Story is A new tic tac toe board is empty
  In order to to start a new game
  As a player
  I want to have an empty board

      With scenario new board
        Given a new board                 => Passed
        When checking if board is empty   => Passed
        Then it should be true            => Passed
Story is After placing a first X
  In order to to make my first move
  As a player
  I want to have a non empty board

      With scenario new board
        Given a new board               => Passed
          And placing a first x         => Passed
        When checking if board is empty => Passed
        Then it should be false         => Passed

I tried to emulate the style in the StoryQ examples but I’m not sure if I am using StoryQ as efficiently as possible.

NSpec

Pros:

  • concise
  • easy to get started with
  • makes me think of RSpec
  • it kicks ass

Cons:

  • can’t use MS test runner
  • must manually rebuild test library before each NSpecRunner run
  • tests will fail if some conventions aren’t followed (ie: underscores in method names)

StoryQ

Pros:

  • uses MSTest or NUnit test runners and standard test attributes
  • great tools
  • focus on user stories

Cons:

  • not sure if I am using it correctly
  • very verbose

Conclusion

For my conclusion read the tl;dr section at the start of this post.

Consequences of breaking the build

Some teams have consequences for breaking the build. Once I worked someplace where the proposed consequence was to bring doughnuts to your colleagues.

This was mostly done in jest and in a lighthearted way. The rule wasn’t strictly enforced, which is the way it should be, but people still complied from time to time.

I carried over that tradition to other teams where it’s application varied. These kind of consequences can be fun as long as it doesn’t get out of hand or generate any drama.

In the past I printed out some form of “poster” which proclaimed:

And the Lord said unto man, he who breaks the build for others, shall pay a penance of doughnuts. That is the law.

I did not come up with this phrase, it was handed down to me by a more senior programmer and can readily be found on the internet in many variations.

A poster with a picture of a doughnut and the caption: "And the lord said unto man: He who breaks the build for others, shall pay a penance of doughnuts. That is the law."
Example “doughnuts” poster.

On the project I am currently working on, our team lead came up with what I think is a pretty nifty idea.

He created a special status in our issue tracker to assign to certain tasks. When someone breaks the build he gets these tasks assigned to him. These are non-urgent yet important tasks that need to be done. The kind of tasks people don’t usually fall over themselves to do.

For example, going over the UI’s validation message and update them to conform to a new syntax/punctuation standard. That sort of thing.

Of course the system can fail because of too many or too few broken builds in regards to the number of tasks. He agreed to do the tasks himself if no one breaks the build for a long time.

If you have or had in the past some form of similar “broken build” tradition, please let us know with a comment.