Clean is not only for the code or tests!

TL; DR
Branching -> Cleaning-up commits -> Merging -> Pulling -> Pushing;

Working in a clean and neat environment is not ONLY for the production code and the tests of the software, it also can apply on different aspects of software development process. One of these many different aspects can be Version Control that you’re using! How clean is your history/branches/check-ins/commits/etc.? It’s very important to make the environment that we’re working in SUPER clean. Cause it makes the development process much smoother and faster and more efficient. Always remember that the only way to go fast, is to go clean! And clean is not only for code!

I use Git for version control most of the times (I believe at this moment, EVERYONE is using some sort of version control software and if someone doesn’t, then God help them). And after different kinds of experiments and ways of working with it (both as a solo developer and as a team member) I found a clean, neat, smooth and headache-less (that’s not even a word) workflow with Git. I’m going to share it with you here so probably someone finds it useful. There are hundreds of articles and blogs on how to work with Git in a better way and this is only my personal preferred way of working which happens to work very well for me and I’m sure it’s going to be the same for someone else.

Here’s how it works.

Imagine we’re working on a software program and we have 2 remote branches for it! One is the “master” (which we want it to be as clean as possible) and another is “foo_feature” which we branched it out of master in order to implement feature “foo” (whatever that is)!

Obviously we have the corresponding local branches of these two remote ones on our own machine and we work on them locally whichever branch that we’re on at any moment. Now if I want to work on something related to “foo feature”, this is my workflow for doing it:

First I pull the latest changes from the remote foo-feature branch to my local foo-feature branch:

git pull origin –rebase foo-feature

I’m pretty sure a lot of feature are against using –rebase for a lot of reasons which I’m not gonna mention here but the reason that I like using it most of the times is that it’ll give me the ability to solve merge conflict issues one step at a time and continue the process and at the end I won’t have that one extra merge commit which is being generated automatically by Git when you pull. Also it put my latest changes on top of the latest changes in the remote branch history.

Anyway, then I create a new local branch on my machine out of the latest version of foo-feature (I rather not mess with the local foo-feature repository during my experiments and keep it clean and do! So here’s how’s it’s gonna work: (imagine I want to do some Refactoring on the code)

git checkout -b foo-feature-refactoring

After that, I start making my changes and commit them as much as I need during this Refactoring and when I get a log from my foo-feature-refactoring repository imagine here’s what I get as a result:

git log –oneline
e329shf commit message 1
e329shf commit message 2
e329shf commit message 3
e329shf commit message 4

At this point I’m done with the Refactoring and I want to merge it back to foo-feature branch and push it to the remote branch named foo-feature so other people in the team can see them as well! But since I was experimenting a lot during this Refactoring and made some mistakes I ended up with some commits that I don’t want to be in the clean history of foo-feature! I’m sure this happens to everyone during the development. (Some of these commits are not providing any value by being in the history and they’re just bunch of noise there)

So I try to make all these commits into few nice and meaningful commits which totally make sense and they provide some value if they’ll be in foo-feature repository. Beautiful Git let me to do it like the following:

git rebase -i HEAD~4

This will give me the last 4 commits that I made! It will open up an interactive environment (editor) for me including my last 4 commits (which I showed above as a result of git log –oneline) and I see something like the following:

pick e329shf commit message 1
pick e329shf commit message 2
pick e329shf commit message 3
pick e329shf commit message 4
# Commands:
# p, pick = …
# r, reword = …
# …
# s, squash = use commit, but meld into previous commit
# …

As you can see it gives the list of commit messages in an ordered manner (the oldest at the top) and a set of commands that we can manipulate the commits with them!
The one that I’m interested in here for this use case is “s, squash” which will meld the commit into the previous one! Now I Refactored the code in my local branch named foo-feature-refactoring and I want to have only 1 commit with a clear message about what I did so I’ll edit the text that git generated for me to the following:

pick e329shf commit message 1
squash e329shf commit message 2
squash e329shf commit message 3
squash e329shf commit message 4
# …

After doing that all these 4 commits will be meld into one commit and git will give me another editor withe the following structure so I can write my commit message for this whole action:

# This is combination of 4 commits
# The first commit’s message is:
commit message 1

# The first commit’s message is:
commit message 2

# The first commit’s message is:
commit message 3

# The first commit’s message is:
commit message 4

–> Refactored the FooFeature, got rid of some duplication (DRY).
# Please enter the commit message for your changes. …
# …

Now after save/quit of this editor I have only one commit with the message “Refactored the FooFeature class, got rid of some duplication (DRY).” which is succinct and clear. I got rid of all those noisy commits (message 1, message 2, etc.)

Now I need to merge this thing back to my local foo-feature branch which is a 2 step process:

git checkout foo-feature # switched to foo_feature branch
git merge foo-feature-refactoring # merged those 2 branches

Now I can delete the foo-feature-refactoring or if I need it, I’ll keep it there in the collection of local branches! (depends on the scenario obviously)

Now I need to push this change to the remote branch origin/foo-feature so others can see what’s going on! I just do a pull first (after I committed anything I have in my working tree of course):

git pull –rebase origin/foo-feature #and solve any conflict if any exists

Then I push my local changes to the remote branch:

git push origin/foo-feature

Now if someone else pulls the origin/foo-feature they will see my commit:

e329shf Refactored the FooFeature class, got rid of some duplication (DRY).

Instead of 4 commits with confusing messages which are just messing the history and log of the repository!

Also for having better messages in the commits I highly recommend reading these great points by Tim Pope on his blog here!

Maybe a lot of you are already working in this way! I just wanted to share this approach with you since I found it super useful and clean during the development process SPECIALLY in a team.

Worth a try! Hope that helps.

* Special thanks to Phil Corliss for telling me some interesting points about having a nice workflow in Git and its benefits over time!

* If you haven’t already, definitely read this part of “git” book/documentation on Rebasing and why it’s powerful & when you should not use it!

Few more characters better than jumping around

This is a short post for pointing out a quick and handy little thing that I found helpful in some cases while programming.

Some people are religiously against comments and they know the existence of comments in the code as a sign that means the code is not readable! (I was in that group for a while as well :P)

That’s not necessarily a true thing to say or believe. Comments can also exist in a readable and clean code. And they can be very helpful. In one specific case that I want to mention here, they can save you some scrolling and jumping around the file in order to get a sense out of the code you’re reading.

I’m a huge fan of the statement: “Imagine the next reader of your code is a serial killer and he knows where you live!” I always try to make my code as readable as I can at the moment. I like to be able to get a sense of my code with a glance without any need to dig deeper. Of course this is hard to achieve 100 percent but I try to make it really close when I can. One of the things that I noticed in my test code is that some times in order to have DRY (Don’t Repeat Yourself) I extract out a helper method or a setup block for setting up some data that I need to reuse in different tests that I’m writing.

Imagine you’re having a model named Event and you’re testing some validation rules for this model. Instead of keep repeating some values for its attributes in every single test it’s a good practice to put that as a setup in a “let” block in rspec as following:

let(:attributes) do
    {
        event_type: “CreateEvent”,
        generated_at: sometime,
        event_no: 1
    }
end

and  just change the one that is relevant to the validation rule that you’re about to test in your current test like the following:

it “requires a type” do
    event = Event.new(attributes.merge(event_type: nil))
    event.should_not be_valid
end

This makes your tests less polluted with repetitive values for the attributes over and over again. It’s a small thing with almost a big effect in the code cleanness.

Sometimes having this setup block makes you to scroll and jump around the test code to realize what are the values cause you’re depending on them in order to understand the test you’re reading and trying to understand. Not always you need to know the values. For instance in the above test with a quick glance you see what’s going on and you don’t care what are the values for the attributes cause the only thing that is matter (being tested) here is that event_type can’t be nil and it’s a required attribute.

But sometimes you need to know what’s going on in the attributes. Imagine a scenario that you need each event have a unique value for the event_no attribute. And you wanna test it like the following:

it “needs a unique event_no” do
    event1 = Event.create!(attributes)
    event2 = Event.new(attributes.merge(event_no: 2))
    event2.should be_valid
end

Now here you cannot understand the complete picture with just a glance. You can guess it obviously and I want to make the code completely readable so the reader won’t have to think/guess as much as possible or she won’t have to jump around the code/file to realize what’s going on. When you see this you can guess that the first event (event1) has a different event_no value so that’s why the second event (event2) is valid. Which is totally fine but I don’t think there is any harm in being even finer when it’s the least amount of effort required in order to get there:

it “needs a unique event_no” do
    event1 = Event.create!(attributes) #=> event_no = 1
    event2 = Event.new(attributes.merget(event_not: 2))
    event2.should be_valid
end

That’s it!!! Now with this few characters you saved your life (if the reader was a serial killer and he was pissed by having to scroll up and see the value for event_no in attributes)

I’m sure you think this is TOO MUCH. But again since it’s the least amount of effort and it makes the code even cleaner and more readable than before why not?! It’s so tempting to make it like this and save some scrolling and jumping or unnecessary guessing! Cause this makes the reader to get COMPLETELY what’s going on with just a glance which in my opinion is the best level of readability in the code => Glance-Understandable!!!

Anyway, I stop preaching right now and that’s all I have to say about this. I hope that will be useful.

Sometimes remove the tests after they served you!!!

I know it sounds a little bit strange when I say removing tests after they served you specially because we all know that one of the great benefits of having automated unit-tests is that you’ll end up with a regression suit that you can always run whenever you make a change in your code! So as long as you’re maintaining a project they are serving you in that matter!

What I’m targeting here is the tests that we write during the development directly for methods that should be private in the class-under-test (CUT)! I’ll show you what I mean with a SUPER simplified example. Technically I eliminated everything in this code for showing my main point!

Imagine you’re writing a microblogger which is using a third party API as a twitter client. As we know one of the features of twitter is Direct Message  (known as DM). For implementing this feature you will write a method in your MicroBlogger class named “dm” for example. Of course we’re doing it TDD so one test for this class would be like the following:

it “doesn't send message for someone who’s not following you” do
    twitter_client = stub(:followers). and_return([])
    microblogger.dm(“anyone”)
    twitter_client.should_not_receive(:dm)
end

Now we’re gonna write enough code to make this pass. When we start to implement the production code for this test we realize that we need to check whether the receiver is following us or not![1] This is a unit of work on its own! There are 2 options here for us. A) We start implementing the dm method and inside that method we insert the logic of this check and see whether the test will pass or not B) Make the current test pending and start TDD-ing that unit-of-work (checking whether the receiver is following us or not) and as soon as we have that functionality in place un-pending (weird word) the test and continue the development of dm method.

IMHO the second option is better for several reasons. First of all the first option is violating the rule of taking the smallest possible steps and going in the baby-step style that eXtreme Programming and TDD talk about it cause it’s obviously going through more than one unit-of-work. Also if the test fails after implementing the dm method with the first option we need to check more than one thing, either the check for followers has a problem or the other part of the dm method. And we should have the least number of reasons for a test to fail (1 is the best). And some other reasons which are not for this post.

Then we make the original test pending and start TDD-ing the check functionality for followers. Couple tests for something like that would be the followings:

it “knows when someone is following you” do
    …
end

it “knows when someone is NOT following you” do
    …
end

Then the production code for making them pass can be in a method called followed_by?(someone)

Now it’s time to un-pending the original test that led us to here and implement the dm method to make it pass. Part of the dm method for making the test pass could be like the following:

def dm(receiver)
    …
    return if !followed_by?(receiver)
    …
end

Perfect. Now think about the design of the class under test and how its interface is gonna be to the outside world (other classes/modules of the system)! The followed_by? method should not be visible to the outside world. This is a method that is being used internally in the class and we defined it cause we didn’t want our dm method to do more than one thing! Remember Uncle Bob definition of clean method: “A method should do one thing and only one thing and do it well!”

So what are we gonna do now? If we make the followed_by? method private then those couple tests will complain about it cause they can’t see it anymore and we have to remove them to shut them up! It’s A OK to do that in this case. Sometimes you can remove the test after they served you. We TDD-ed the functionality of followed_by? in our code and we’re using it with no problem in the dm method. And it’s completely a good choice to make that method private according to the circumstances and how it’s being used in the class. And followd_by? method is still being tested indirectly through the tests for the dm method, so we didn’t ruin the coverage of our automated tests.

And I know that we don’t have that ONE reason for failure anymore but again “Programming is the world of trade-off” and IMHO this time it’s a better idea to make the method private and remove the tests related to it cause they served their purposes during the development of that part. I learned this interesting technique and idea from Kent Beck TDD screencasts by Pragmatic Programmer which I highly recommend you to check them out! This technique came handy and very interesting in a bunch of places while I was writing code and I thought it’s a good idea to share it with you.

Hope that’ll be useful!

[1] When we’re doing Red-Green-Refactor cycle in this case we just define the dm method and the code will pass cause we’re not calling the dm method on the twitter_client (writing only enough production code to make the failure pass)! And then with more tests we drive the design and development of the dm method towards this whole idea of checking whether the receiver is following us or not! But again this is a blog post and I wanted to make another point here so I cut through the path a little bit 🙂

Just because you can, does not mean you should!

I tweeted about this thing a couple times and now I want to elaborate a little more on it with an example! A lot of times I hear people talking about the fact that something is much easier to do in a specific language and we can do it that way! And a lot of times that makes sense and is a good way to do that specific task in that specific language! But sometimes it’s just a violation of a principle that we’ve learned the hard way that it matters and we should be careful about it in our code base! For example imagine we want to write a user authenticator which determines whether user’s credentials are correct or not! You can see something similar to the following in a lot of code bases! (Remember that it’s just a very simple thing just for the sake of this post and by no means this is a user authenticator :D)

describe UserAuthenticator do
    it “does not authenticate with wrong password” do
        user = stub(:username => “bob”, :password => “pwd”)
        user_rep = stub(:get_user_by_username => user)
        DatabaseUserRepository.stub(:new => user_rep)
        authenticated = subject.authenticate(“bob”, “wrong password”)
        authenticated.should be_false
    end
end

class UserAuthenticator
    def initialize
        @user_rep = DatabaseUserRepository.new
    end

    def authenticate(username, password)
        user = @user_rep.get_user_by_username(username)
        password == user.password
    end
end

It’s not this easy to do such a thing in a static-type language like Java or C# but since ruby is a dynamic language (duck-type) so ruby and rspec let you do something like the above code! As long as it responds to “new” message you can stub it in any way you like so a lot of people just because they CAN do this they do something similar in their code! But we forgot the main point here! We are violating the Dependency Inversion Principle and instead of depending on abstraction we are tightly coupling ourselves to a concrete thing (DatabaseUserRepository)! I know that Dependency Inversion Principle in a dynamic language such as Ruby has a slightly different meaning and it’s not exactly as its meaning in Java or C#! But what matters for us here is something that can responds to the message “get_user_by_username”! That’s all that matters here! But we are making ourselves more dependent than what matters! Imagine we want to also support storing user information in flat files or some other mechanism! How are we gonna handle those parts of code for the same logic and functionality?! Are we gonna have another UserAuthenticator that instantiate FileUserRepository in its constructor and the rest will be the exact same?! Of course not and I can see you’re cringing right now! WTF?!

We have something which is exactly helpful in this kind of situation and it’s called Dependency Injection! (Of course there are other ways for handling this, e.g. Strategy Pattern, but I go with Dependency Injection for this post just for fun :D)

We inject the dependency and the only thing that matters to us is that it can respond to “get_user_by_username” message! So when it comes to support different kinds of storage mechanism and their appropriate repositories the code will be totally flexible in that scenario and all we need is a new instance of our UserAuthenticator with a FileUserRepository injected to it! So the code for such a design will be like the following:

describe UserAuthenticator do
    let(:user_rep) { stub }
    let(:authenticator) { UserAuthenticator.new(user_rep) }
    it “does not authenticate user with wrong password” do
        user = stub(:username => “bob”, :password => “pwd”)
        user_rep.stub(:get_user_by_username =>; user)
        authenticated = authenticator.authenticate(“bob”, “wrong password”)
        authenticated.should be_false
    end
end

class UserAuthenticator
    def  initialize(user_rep)
        @user_rep = user_rep
    end

    def authenticate(username, password)
        user = @user_rep.get_user_by_username(username)
        password == user.password
    end
end

The point is not just being able to write test for the code! The whole point of TDD is driving a better design for us so in a static-type language it’s not easy to do something like the first example hence it’s hard to write tests for it this way so it will drive us towards depending to abstraction and also injecting that dependency through constructor so we can easily replace the actual collaborator with a stub and test the unit of work that we want at the moment completely isolation! But because we can write a test like the first example in ruby and rspec it doesn’t mean we should do that! The point is having a better design not being able to write tests for the code!

Remember again, just because you can does NOT mean you SHOULD! Power brings responsibility so use the power in a right way and at the right place!

Something worth a try!

I hope you enjoyed it!

Refactor till Drop!

Sometimes we think that our code is good enough and clean! It’s totally fine to continue developing instead of looking it from one step back and clean it up before we move one! After all it is called Red-Green-Refactor! It’s not Red-Green-[Move immediately after Green to the next Red]

Currently I’m working on rails application for maintaining banking transactions and generating some special reports for the data on a frequent basis and do a lot of stuff with that data, alerting, grouping, etc. which doesn’t matter for now!

I’m trying to a have good and SOLID design in my application and separate the logic from rails framework for having a better separation of concerns, fast-isolated stuff and some other reasons that I talked/tweeted about them many times!

One of the features for this application is doing several operations on transactions of the current month. So we need to get the transactions of the current month first then do the operations, render views, etc.

This is a good candidate for a class/module which does this specific task and nothing more! Single Responsibility Principle is our friend! I went through the Red-Green-[ Move immediately after Green to the next Red] cycle for this piece of code and I ended up with the following two rspec examples:

describe CurrentMonthTransactions do
    it “gets the transactions for the current month which are in the current year too” do
        calendar = stub(:today => Date.new(2012, 10, 5))
        tr1 = stub(:date => Date.new(2012,  9, 1))
        tr2 = stub(:date => Date.new(2012, 10, 1))
        Transaction.stub(:all => [tr1, tr2])
        transactions = CurrentMonthTransactions.get_transactions(calendar) #this is for dependency injection
        transactions.should == [tr2]
    end

    it “does not get the transactions for the current month which are in the current year” do
        calendar = stub(:today => Date.new(2012, 10, 5))
        tr1 = stub(:date => Date.new(2012,  9, 1))
        tr2 = stub(:date => Date.new(2011, 10, 1))
        Transaction.stub(:all => [tr1, tr2])
        transactions = CurrentMonthTransactions.get_transactions(calendar)
        transactions.should == []
    end
end

First thing I like to mention here cause I want to be a positive person is that it’s a good idea that I have 2 tests here and they are testing this “year thing” specifically! Because in this case my tests have better granularity and more focused and since they are supposed to be documentation of the system they are shouting this tricky point about the year in getting the current month transactions that you should not just check the month!

Ok enough with the positivity! This code is terrible, horrible, awful and ugly 😀 and the reason for it is exactly the third step in my cycle! (look above)

So I tried to tackle this code one step at a time and I show you the steps here. I started with removing duplications and rspec will help you to do such a thing perfectly through different constructs!

Here is the first attempt for both removing the duplication for the 10 magic number that appeared a lot in my tests and also express its meaning with an expressive name:

let(:october) { 10 }
# Then I replaced all those 10s with “october” variable
…

After that I removed the duplication in stubbing the calendar concept like the following:

let(:calendar) { stub(:today => Date.new(2012, october, 5)) }
# Then I removed this line from both tests
…

After that I removed the duplication of declaring that stub “tr1” in exact same way in both tests:

let(:tr1)  { stub(:date => Date.new(2012, 9, 1)) }
# Then I removed its declaration from both tests
…

Then I attacked the duplication for getting current month transactions which was again the exact same line in both tests:

subject do
    @transactions = CurrentMonthTransactions.get_transactions(calendar)
end
# Then I replaced this line with subject
…

We eliminate a lot of mess! So far we made the code look like the following:

describe CurrentMonthTransactions do
    let(:october) { 10 }
    let(:calendar) { stub(:today => Date.new(2012, october, 5)) }
    let(:tr1) { stub(:date => Date.new(2012, 9, 1)) }

    subject do
        @transactions = CurrentMonthTransactions.get_transactions(calendar)
    end

    it “gets the transactions for the current month which are in the current year too” do
        tr2 = stub(:date => Date.new(2012, october, 1))
        Transaction.stub(:all => [tr1, tr2])
        Subject
transactions.should == [tr2]
    end

    it “does not get the transactions for the current month which are in the current year” do
tr2 = stub(:date => Date.new(2011, october, 1))
        Transaction.stub(:all => [tr1, tr2])
        subject
        transactions.should == []
    end
end

A lot better! But still there are some duplications and it can be better. That’s why I say “Refactor till Drop!” because you should not say: “Ok, it’s clean let’s move to the next failing test!” you should make it REALLY clean! Otherwise these little messes inside the code will gather and bite you usually sooner than you think and sometimes it’s too late to do something about them and they make the development really painful and hard! So I’ll do another change for eliminating even more duplication and this is the final result:

describe CurrentMonthTransactions do
    let(:october) { 10 }
    let(:calendar) { stub(:today => Date.new(2012, october, 5)) }
    let(:tr1) { stub(:date => Date.new(2012, 9, 1)) }
    let(:tr2) { stub }

    subject do
        @transactions = CurrentMonthTransactions.get_transactions(calendar)
    end

    before do
        Transaction.stub(:all => [tr1, tr2])
    end

    it “gets transactions in current month and year” do
        tr2.stub(:date => Date.new(2012, october, 1))
        subject
        transactions.should == [tr2]
    end

    it “does not get transactions in current month but other years” do
        tr2.stub(:date => Date.new(2011, october, 1))
        subject
        transactions.should == []
    end
end

As you can see I even changed the name of my tests for being more succinct and at the same time expressive enough! Now you compare the first version and this one then you will realize why I called that one terrible, horrible, awful and ugly! I was not mean! That is the absolute truth!

Now you might argue that we can even make it better (first thing is better names for tr1 and tr2) and I totally agree with you! That’s why you can never say my code is cleanest! There is always a way for making it better! But you should make it clean enough for having a smooth development with no roadblocks! And this code from my point of view for the thing that I’m currently doing is innocent enough and makes me happy!

And I’m not scared at all! Why? Because even if the next reader of this code will be a serial killer and knows where I live, I’m sure he’s not gonna come after me cause this code won’t make him angry! It’s clean enough! I encourage you any time you’re writing a piece of code, imagine its next reader will be a serial killer and he knows where you live! That will help, believe me! 😀

Something worth trying!

Details are Evil

Recently one of my friends asked me to help him for refactoring and improving the design of his rails application and its unit and integration tests. It was a really great experience because working on other people’s codes always teaches you great stuff. You get to see how someone with different point of view and different skills writes a piece of code and solves a problem. I personally learned a lot from reading other’s codes and watching people writing code and explaining their thoughts while they were doing it. I believe it was Picasso who once said: “the best thing that inspires me is watching other painters’ works.” If you want to see how important this is and how it can affect you, I recommend you to watch this great talk by Geoffrey Grosenbach about watching people code.

My friend told me one of the nightmares in the application was maintenance of the scenarios. Apparently they were too fragile and whenever they changed something in the application code they had to change the scenarios accordingly. And because of the fact that those scenarios were the live documentation of the application (which they should be) this frequency of changing them was driving them crazy. So I was reading some of the cucumber scenarios and I saw the following in the signup feature of the application:

Given I am on the homepage
When I follow “Sign up”
And I fill in “Email” with “john@exmaple.com”
And I fill in “Password” with “foobar”
And I fill in “Confirmation” with “foobar “
And I press “Submit”
Then I should see “User has been created.”
And new user should be added.

Evil was found. As you can see in this scenario, there is too much information in it. Not useful information about the domain of the application but details of how to accomplish something in it. (signing up for this specific case)

A scenario should talk about the domain not about moving from one page to another and clicking some links or filling in some text boxes in the page. Imagine customers are talking to you about this kind of feature in the application they want you to produce for them. How would they describe it to you? They would say: “users can sign up through the website” or they would say: “a user should click on the Sign up link and then fill in text boxes with the appropriate information and after that click on the button. And when he/she successfully signed up there should be a message about it”? Of course it’s going to be the former when they are describing the feature. Most of the times they will tell you something like the latter expression when they see the system functioning. Then maybe they will tell you about some modifications they want you to make in the details of the feature. But normally they don’t do it when they are describing the feature. And this is the job for scenarios. They should DESCRIBE the feature not the details of it.

This is one of the numerous cases that testers can be really helpful. Testers have a different point of view from programmers and they tend more to look at the “WHAT” not the “HOW”. When a tester sees a scenario like this she will immediately say: “this is not a good scenario. It contains too much detail and distracts the reader from the essence of the feature”. This is why Liz Keough emphasizes that BDD scenarios should be born from conversations between user, tester, programmer, etc. and the conversation is the most important ingredient cause it’ll embrace different viewpoints.

Cucumber perfectly supports the right way of doing this. What you see here should be in the step definitions not in the feature and corresponding scenarios. So we changed this scenario to something like this:

Given a user is on the homepage
When she signs up
Then she should be included in the users of the website

That’s it. All those detail information moved into the step definitions of signing up. You should not talk about HOW in the scenario. Scenario is a place for a WHAT of a feature. HOW should be handled in the step definitions (but as little as possible).

Specially in Rails

You can see this kind of problem more in cucumber scenarios for rails applications. The reason for that is because when you run the generator command for cucumber (rails generate cucumber:install) it creates a web_steps.rb file which contains some pre-defined steps in it. People tend to use them and don’t introduce their own steps as much as possible. So they write their scenarios in a way that they can use from the steps already existed the in web_steps. And that is the main reason my friend and his team wrote their scenarios in that way too. (e.g I follow “link” or I press “button text” etc.)

Fortunately new versions of cucumber-rails gem and cucumber generator removed that pre-defined web_steps file completely for this reason. But at the same time unfortunately some people are used to the old version style and still define their scenarios and steps like that. You can find more about this removal here.

Remember, just because something exists it doesn’t mean you have to use it or you have to adapt yourself to it. You should try to write high quality, maintainable, flexible code and test and anything that impedes this, should be eliminated and removed from your arsenal.

I hope you like this and I appreciate your feedback.

Notes

Thanks a lot to Lisa Crispin for motivating me to share this.

I know that the example here is really classic and educational but I could get permission to share only this one scenario from my friend and his team.

Fast Rails Tests, Smoother Development

If you’re a rails developer or had an experience of writing a rails application you know that running your tests is a really painful and long process because it loads the rails environment at first. I’m assuming you write unit-tests for your rails application because it’s really offensive not to write tests in kind of an application that creates a test/spec folder for you from the very first beginning of its life. It assumes that you write tests for your app because you care about your code and software. Once again I remind you Uncle Bob’s famous quote about writing tests and TDD: “you can’t call yourself a professional developer if you’re not doing TDD.” Anyway, running tests in rails is really painful and too slow. When you are doing your Red-Green-Refactor cycle whenever you run your tests you have to wait several seconds until you get a feedback from your tests. This is not the way how it should be done. This latency and delay is breaking the cycle and is really overwhelming. Pretty soon you won’t run your tests as frequent as you should, therefore you won’t be able to get quick feedback about what you’ve done and you miss a lot of TDD and unit-testing benefits in general.

I personally write my code with TDD approach and the quick feedback is really critical during my development cycle. So this kind of slow test won’t let me to be as productive as I should. Whenever I run my tests I am like: “Son of a Gun, why is this happening?!”

Recently I watched Corey Haines talk at Arrrrr Camp about fast-rails-tests. I can’t emphasize on watching this talk enough if you care about your craft and productivity as a developer. Note that I didn’t say rails developer because there are lots of good ideas that you can take from that talk as a developer and software craftsman in general. In that talk Corey mentions about separating your domain and business logic modules in a way that don’t depend on rails. Not surprisingly, isolation and separation of concerns is a golden solution here, AGAIN. According to Corey’s example imagine you have a ShoppingCart model which has many products in it (ActiveRecord model of course) and at some point you need to calculate the total price of the products in your ShoppingCart. By default when people face this kind of problem they write a method in the ShoppingCart model which calculates the total price of the products like this:

def total_price
    products.map(&:price).inject(0, &:+)
end

BTW this is one of the reasons we will end up with “Fat Models”. Let’s ask a question from ourselves at this point: “does this really have anything to do with rails and ActiveRecord?” No, it’s obvious. So, why we put it in the model? Why we made this logic depend on the ActiveRecord and rails in general? We shouldn’t do this. This is a calculation and logic and it should be separated. There are different ways for doing this according to Corey’s talk and I definitely recommend you to watch it and learn about them because in each situation you should pick the best solution. For instance, we can separate this logic and include it in the model so it has this functionality, but the functionality implementation itself is in the right place and separated rather than being mixed with the model. Here’s the test and code for it:

it “returns sum of price for multiple items” do
    TotalPrice.of(stub(:price => 10), stub(:price => 20)).should == 30
end

module TotalPrice
    def of(items)
        items.map(&:price).inject(0, &:+)
    end
end

Then we can use it like the following:

class ShoppingCart < ActiveRecord::Base
    has_many :products

    def total_price
        TotalPrice.of(products)
    end
end

Now you can have your tests for the total price calculation separately from the model and it has no dependency to rails which is the right thing to do. Your tests for this logic will run extremely fast with no latency and delay because of good separation of concerns. I love this idea and ever since I watched this talk by Corey I’m looking for potential Refactorings in my rails apps for separating my domain and business logics and calculations from my models. One of the perfect candidates for this was in my User model. I have some password machinery in my User model which encrypts the password before the user is saved in the database using the before_save ActiveRecord call back. It kind of looks like this: (I made it so much simpler for focusing on the purpose of this post):

class User < ActiveRecord::Base
    attr_accessible :password
    before_save :encrypt_password

    def encrypt_passowrd
        #some password encryption machinery here
    end
end

My tests for this encryption are really painful like all the rails dependent tests because they take too long to run. But then it hit me that this machinery has nothing to do with rails, it’s an encryption algorithm that I use for my password security issues. So I can SEPARATE this logic from the model perfectly. So I ended up writing the following module:

module PasswordEncryption
    def encrypt(str)
        #some encryption machinery here
    end
end

Then I changed my model accordingly:

class User < ActiveRecord::Base
    include PasswordEncryption

    attr_accessible :password
    before_save :encrypt_password

    def encrypt_password
        encrypt password
    end
end

Now I can run my tests during my development cycle very quickly and get fast feedback which is one of the main purposes of TDD and unit-testing in general. My development process for the business logic and calculations part of the application which BTW are most important parts, became so much smoother and faster and I can completely see the improvement in my productivity when I’m working on a rails app. Here are screenshots from running password encryption related tests in my app before and after this Refactoring and design change:

Look how much difference that design change made. I loved this point by Corey during his talk: “most of the times when people have troubles testing something in their code either about running time or testing the functionality itself they change their tests but it’s Test-DRIVEN-Development and it means that we should change our design because that difficulty in testing shouts about design problem which needs to be solved. So, we should change our design NOT our tests.”

I found this really simple trick which helps me a lot while I’m hunting for this kind of Refactoring and design candidates in my rails apps, whenever you face something in your code ask yourself this simple question: “does this have anything to do with rails?” when the answer is “No” go ahead and separate that logic. Try this and you’ll be surprised and thrilled about the fact that you will develop your app so much SMOOTHER and more FLUENT!

Hope that helps, it helped me a lot.

 

Related Posts:

Running Rails Rspec Tests – Without Rails

Making ActiveRecord Models Thin

Architecture The Lost Years (Uncle Bob)