Single Level of Abstraction (Don’t mix things in the wrong place)

This is going to be a short post on a very interesting and important programming concept that IMHO is really helpful and make a nice difference in the code! It’s about not mixing things that have different levels of abstraction in a method! The name of this technique is Single Level of Abstraction (not surprising really) by Kent Beck and it’s about not putting things that are in different levels of abstraction and details in the same method! It’s better to have same level of abstraction for the statements inside a method! It’s one of those somewhat fuzzy technique/principles in programming so I think an example would be good right now. For making the point lets imagine a VERY simple and almost-unreal piece of code! Lest say we store a matrix in a file and we want to compare two different matrices that are sitting in two files and check whether they’re equal or not. At one point, our compare method looks like the following:

class MatrixFileComparer:
…
def compare(self):
  with open(self.file1) as f1:
    lines1 = f1.readlines()
  with open(self.file2) as f2:
    lines2 = f2.readlines()
  return self.compare_lines(lines1, lines2)
…

As I said it’s a very simple piece of code and obviously it reads lines of each file and compare those lines together one by one! If you pay attention to the content of compare method, it’s more like an integration and delegator point of the MatrixFileComparer class! That means it is delegating different responsibilities to appropriate methods inside/outside of this class each of which doing one thing and do it well, then integrates results/effects of them! Such a method in a class should not have any duplication and any low level task or implementation detail and also it’s mostly invoking/delegating to other methods as I mentioned before! But if you look at our compare method, it’s also doing a lower-level task which is opening a file and reading all of its lines via the file handler object! Right there you can notice the different levels of abstraction in this method! One task is reading the content of a file, which is obviously more fine-grained and lower-level than the other thing, which is JUST delegating to compare_lines method!

Now lets fix this mixture of abstraction levels and see what it looks like:

…

def compare(self):
  lines1 = self.read_all_lines(self.file1)
  lines2 = self.read_all_lines(self.file2)
  return self.compare_lines(lines1, lines2)

def read_all_lines(self, file_name):
  with open(file_name) as f:
  return f.readlines()
…

Now if you look at the compare method, ALL it does is just method invocation and delegating the job to the appropriate method and getting its result in order to use in another step! The interesting thing is that all the steps in compare method are at the SAME LEVEL of ABSTRACTION now, unlike before that one step was doing something lower-level and the other step was doing something in a higher level of abstraction! It happened to eliminate the duplication that we had in our previous version of compare method as well but that’s just a bonus and it could have happened the other way around!

IMHO it’s a really interesting principle and helps to improve the code further! And the nice thing about it is that you don’t have to jump between different levels of abstraction and details in your mind while reading the code inside a method! Because when you have to do that and all of a sudden the method goes into more details and lower-level implementation tasks, it’s just a distraction from understanding WHAT that method is doing in each step and show you HOW it’s doing something in the middle of your higher level view! That’s a different level of abstraction and belongs to another method of the class!

Anyway, I should stop preaching now! I hope you find it interesting and helpful as well!

Happy Hacking!

Advertisements

Nested Stubbing => Shouts for Refactoring

A lot of programmers write unit tests during the development and also a lot of programmers do Test Driven Development. One thing that we usually forget while programming is Listening to our Tests. If we listen carefully to our tests they will give us a lot of interesting hints and information and frankly that’s why a lot of people call it Test-Driven-Design because we can find useful points in our tests that will help us to have a better design.

I’m going to talk about one of those points that we can find out very easily by listening to our tests and will help us to have a better design and having a piece of information in its right place in our program. That certain point is Nested Stubbing which BTW happens a lot in our tests.

Lets say we are writing a twitter client app and all the communication through the network, OAuth related parts, calling Twitter API, fetching and storing tweets, etc. is being done in separated modules (Separation of Concerns and Single Responsibility Principle etc.)

We have also different kinds of presentations (views if you will) for this application. One of them is a Console Based presentation for taking a look at the tweets in terminal. One part of this presentation is taking the latest 10 tweets and rendering them (in whatever way you like) to the user. Obviously the rendering related code lives in its own place separated from the logic, data storage etc. Imagine we have a Console class which gets those latest tweets and give them to the renderer object in order to show them to the user (for brevity I remove some parts):

def show_latest_tweets
    last_ten = Tweet.order(‘created_at DESC’).limit(10)
    renderer.render_tweets(last_ten)
end

and lets say we have a test for this piece to make sure that this method is calling the right things:

it “fetches last 10 tweets and renders them” do
    latest_tweets = [t1, …, t10]
    ordered_tweets = stub(:limit).with(10) { latest_tweets }
    Tweet.stub(:order).with(‘created_at DESC’) { ordered_tweets }
    renderer.should_receive(:render).with(latest_tweets)
    console.show_latest_tweets
end

**

If you look at this code there is an obvious violation of  Law of Demeter happening. We know that when we call order on Tweet what is being returned has a limit method that we can call to limit the results. How can you easily detect this? BECAUSE WE’RE DOING A NESTED STUBBING IN OUR TEST. We are setting up a nested stub cause we setup ordered_tweets as a result of calling order on Tweet which is a stub itself. This tells us that we know TOO MUCH about the inside of Tweet class and its details at this level (Console class) which we should NOT!

Right now we’re using something like ActiveRecord as the ORM for storage part of the application but what if we change that later to something else? There’s a good chance in that new ORM the mechanism for doing the same thing (getting the latest 10 tweets) will be different and we need to change our code appropriately. But with this code that we wrote here we have to change Console class for changing our storage mechanism which does not make any sense. Console SHOULD NOT know anything about the details of storage and storage should NOT be a REASON of change for Console.

We need to have a layer which hides this information from Console and give him what he wants instead of Console reaching for that information through method chains (Tell, Don’t Ask). As you can see Tweet is like a model (if you will) in this application. And he’s the one who should know about the storage mechanism in this app (how to be stored, retrieved etc.) [Or even taking it further there can be a TweetRepository class which is specifically handling storage-related stuff for Tweet, kind of like a Façade Pattern between Tweet and DB/File/etc.]

We can add a method to Tweet class like the following:

def self.last_n_tweets(n)
    Tweet.order(‘created_at DESC’).limit(n)
end

So now lets rewrite our test based on this change:

it “fetches last 10 tweets and renders them” do
    latest_tweets = [t1, …, t10]
    Tweet.stub(:last_n_tweets).with(10) { latest_tweets }
    renderer.should_receive(:render).with(latest_tweets)
    console.show_latest_tweets
end

And now the code for it:

def show_latest_tweets
    last_ten = Tweet.last_n_tweets(10)
    renderer.render_tweets(last_ten)
end

First of all, we don’t have that nested stubbing in our test method but that’s not the point, if you pay attention you see that we eliminated the coupling and dependency of Console to the Storage mechanism and it doesn’t have any knowledge about that part anymore which is a big advantage. If we decide to change the data storage part of this application or change our ORM, that change will be hidden from Console or any client of this functionality (Tweet retrieving stuff etc.)! Console will still call Tweet.last_n_tweets and how’s that being implemented is none of its business and it doesn’t care.

As you saw listening to our test can have interesting results and nice design hints. Whenever you feel that your test is more work than it should be, or it doesn’t seem to be right or it’s hard to write the test for the target piece of code, that means there’s a design problem in our code (MOST OF THE TIMES). So we should fix the problem in the right place not struggle with making that test happen anyway. It’s time to stop preaching.

Hope that helps and happy hacking.

** This test contains more than one assertion which is usually not a good idea (there are exceptions depending on the situation like anything else in programming)!  I wrote those here to show how things are supposed to work in that piece of code!

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)

Intermediate Variables, more Communicative Code

As you know one of the four rules of simple design is that the code should clearly express the intent of its author. So any attempt for making the code more communicative and expressive will pay off later. As Martin Fowler says: “Any fool can write a program that computer understands but the art is writing a program that humans can understand perfectly.”

Kent Beck has a book called Implementation Patterns this whole book is about the techniques and patterns for making the code as communicative  as possible. I strongly recommend reading this book at least twice because you will see how dramatically your code becomes more expressive as a result of these patterns.

We should try to write a clear code which communicates its intent perfectly. Because when someone will read it in future (doesn’t matter close or far) can understand what it does and what was our intent when we wrote it at the first place. Remember, most of the times we will be that someone so it is better to write our code in a way which won’t make us to curse ourselves.

One of the simplest and at the same time great ways for making the code more communicative is using expressive intermediate variables. Unfortunately we forget about this little technique a lot and the main reason for that is the attraction to making things succinct. It is nice to write succinct code but remember succinct means short AND useful. It doesn’t just mean short. But sometimes we forget that and we are so happy while we are making things done in ONE line. Sometimes this one line thing comes at a price and that is less readability and understandability of the code. We fade in this fake beauty so much that we forget about the main goal which is making things clear and expressive. There’s an old saying: “don’t make much at once.” We should consider this continuously during our programming.

Few days ago at work, we found a problem in our filtering process. When in the UI we set some filtering for data retrieving it worked correct but when we removed that filter and retrieved data again that same filtering was considered as before. For instance when I wanted to get users that their first name was John, I wrote John in the FirstName filter text area and it worked as it should but when I removed John from that filter text area again it gave me the users with first name John.

Finally I found out that the filter mechanism didn’t clear itself in some special circumstances and I tried to write a unit-test for it which of course failed and then I made it pass. Sure enough the problem was solved and the filtering process worked like a charm since then. (Sometimes when you are doing TDD you cannot think of all possible cases, so later you will face a problem in your system. The best reaction to that is writing an automated unit-test for it and reproducing the problem in that test and then making it pass. With this approach you will be sure that this situation will be covered all the time through your test suite after that.)

I did this process while our new employee sat next to me. The test was almost something like this:

[Test]
public void ShouldRetrieveAllDataWhenThereIsNoNeedForFiltering()
{
    var searchEngine = new SearchEngine();
    searchEngine.AddFilter(new Filter(“FirstName”, “John”));
    var users = searchEngine.GetUsers();
    Assert.AreEqual(10, users.Count);
    Assert.IsTrue(AllUsersNamesAre(“John”));
    searchEngine.AddFilter(new Filter(“FirstName”, null));
    users = searchEngine.GetUsers();
    Assert.AreEqual(50, users.Count);
}

When I asked her how she feels about this, she said: “what does it mean when you say new Filter(“FirstName”, null)? what is the meaning of null in this context? Does that mean users that their first name is null?” At that point it hit me. I was so excited about making the filtering process in one line that I forgot about the most important goal. I should make the code communicative so the reader can understand the intent perfectly. But apparently I screwed up. So I sat back and looked at the code and thought with myself: “why is this so obvious to me but not her?” and then I found out. I knew that when the search engine gets a filter it will only use it when that filter is effective. Somewhere in the GetUsers implementation the search engine will check this à filter.IsEffective() and IsEffective will return false when the value part of the filter is null etc. And when the filter is effective the search engine will use it otherwise it will ignore it. Because I knew about the implementation details of this code that test was clear to me. But that is the problem. The unit-test is supposed to be the live and executable documentation of the system and everyone should be able to find out about how the system works in general by looking at it not by knowing about the implementation details of the system.

So I made this one line change and everything turned out differently:

public void ShouldRetrieveAllDataWhenThereIsNoNeedForFiltering()
{
    …
    var ineffectiveFilter = new Filter(“FirstName”, null);
    searchEngine.AddFilter(ineffectiveFilter);
    …
}

After this she said: “oh, it is an ineffective filter so it will do nothing and then the search engine should return all the users.”

With that one line and introducing an expressive intermediate variable with appropriate name the whole intent of the code became obvious even to someone who had no idea of the implementation details of the production code.

Conclusion

The main goal is communicating the intent through the code as much as possible. Even simple techniques like expressive intermediate variables can change things dramatically. Remember that these little things make the difference between a crap and a well-crafted code. Do not forget about the goal along the way. Always keep in mind that by expressing the intent clearly the reader will thank you and be able to understand the code perfectly. MOST OF THE TIMES THE READER WILL BE YOURSELF.

Thanks for reading, hope that helps.

Expressive Hard-Coded values in tests

This is just a quick thing about making unit tests (or micro tests, you can call it whatever you want the important things is the concept) more readable and less confusing. In TDD by Example Kent Beck says that we should be careful about the data we use in our unit tests. If we use a data for a specific purpose we should always use that data for that specific usage, because it makes our tests more consistent and readable. Also it doesn’t cause any confusion for the reader. For instance, if we want to have one sample student entity in some of the unit tests, set its Id to 1 for all the places, do not set it 1 for a unit test and 2 for another one (of course this depends upon the goal of the test and sometimes we should use different values but clearly I’m not talking about those situations and I mean situations that using different values not only have no benefit but it also makes inconsistency and confusion).

We can go further than this and try to make our unit tests even more communicative and expressive by just a little thinking about the values of the data in tests. If we care about the hard-coded values in our tests like the way we care about naming of the things in our code then we will end up with a great improvement in the readability of our unit tests.

Imagine we are writing unit tests for a log in system. One of the cases is that if username was nil and password was whatever (it can be anything, doesn’t matter) system should not validate this combination of username and password:

it “should not validate when username is nil” do
    result  = @user_validator.validate(nil, “blabla”)
    result.should == false
end

By reading the content of this unit test we cannot understand about the point that when username is nil then it doesn’t matter what the password is because its value is “blabla”, something that is not expressive at all. For solving this issues there are two solutions a) express this fact in the description of the unit test b) use more communicative and expressive value for password. I’m gonna focus on the second solution here while the first one is important equally. But sometimes you cannot say every little thing in the description or name of the unit test because they should be more story-like and talk about scenarios. In this example if we use “any password” instead of “blabla” anyone can understand the point by reading it.

In another scenario we will have something like this:

it “should not validate when password is wrong” do
    result = @user_validator.validate(“john”, “blabla”)
    result.should == false
end

It is a good unit test and you can understand a lot by its description but the combination of the username and password is not very communicative and can be a little confusing. Now what about this one:

result = @user_validator.validate(“correct username”,  “wrong password”)

At first when we see the previous test we say it is perfect and completely expressive but when we see this new one, we will be like: “aw, this one is much better”. This is exactly the Corey Hains point about the fact that no code is in its cleanest state and there is always room for more Refactoring. With this little change we made a great impact on the readability and expressiveness of the test and no one will be ever confused by these values. Also we can conform the Kent Beck’s rule that I mentioned at the beginning, perfectly. Whenever we mean wrong password we can use “wrong password” or whenever we mean password doesn’t matter we can use “any password” and so on. This little thing can show its wonderful effects in huge code bases with thousands of unit tests.

Sometimes we think that are these extra efforts worth it? Do they return worthwhile values? But always remember that these little things can make a huge difference. By doing these kinds of improvements a good code or even a great code can become WONDERFUL. No one will ever say: “that is good don’t make it better”!!! There is always room for becoming better and making improvement in EVERYTHING.

Thanks for reading.