solnic.eu

Get Rid of That Code Smell – Duplication

2012-05-12T11:14:05Z

This is a post from the Get Rid of That Code Smell series.

Removing duplication from the code is a seemingly easy task. In many cases it is pretty straight-forward – you look at similar bits of code and you move them to a common method or class that is reusable in other places. Right? No, not really. It is true that code that looks similar might be an indicator that there’s a duplication but it’s not the definitive way of determining the smell.

If we look for duplication in our code we should look for duplicated concepts instead of similarly looking lines of code. Here I want to remind you the short definition of DRY principle that I’m sure most rubyists are familiar with:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

Andrew Hunt and David Thomas

The Pragmatic Programmer

There is a fantastic presentation by David Chelimsky from RubyConf 2010 titled “Maintaining Balance While Reducing Duplication”. If you missed it you should definitely check it out. There’s a beautiful quote in this talk:

DRY does not mean “don’t type the same characters twice”

David Chelimsky

Maintaining Balance While Reducing Duplication

Detecting Duplication Smell

You can use both reek and flay to aid you with finding duplication in your code but remember that these tools are not smart enough to find similar concepts. Both will find potential duplication but it’s your job to decide if it makes sense to do anything about it.

Here is an example from Virtus project where reducing duplication wasn’t such a good idea:

[crayon-4fae4603e7b90/]

The Equalizer module defines a bunch of methods. Internally it has a few private define_* methods which use @keys ivar to map it to something else. Now the metric tools complained about all these calls to @keys.map. It is true, this is a duplicated code but what’s the risk that it’s going to change? Very, very small. Is this a duplicated concept in the library? Nope.

Nevertheless I went ahead and reduced this duplication by introducing a new method so the code looked like that:

[crayon-4fae4603e7bda/]

Now the metric tools won’t complain. All the calls to @keys.map has been replaced with a call to the new compile_keys method.

This refactor did reduce duplication but with the price of increased complexity. I don’t think it was such a great idea – the risk that these duplicated calls would have to change was so low that I should’ve left this code alone despite the little duplication.

Removing Duplication

Here’s another example where it actually did make sense to reduce duplication to get cleaner code and better encapsulation:

[crayon-4fae4603e7c22/]

I think it’s easy to see duplication here. The duplicated concept is that we want to call a specific method on the value only if it’s actually available. If it’s not then we use an alternative String-based coercion.

This logic was extracted into a new method called coerce_with_method and the module looked like that:

[crayon-4fae4603e7c66/]

In fact this turned out to be so common that later on coerce_with_method was moved to the base Virtus::Coercion::Object class with a few overridden variations in other coercion sub-classes.

Summing Up

Reducing duplication should be based on finding similar concepts, not similarly looking code. The metric tools can only point you to potential duplication but you have to determine yourself if it really makes sense to do anything about it. In some cases you can just complicate your code just for the sake of reducing duplication without any real benefits.

OK! Next episode will be about “Primitive Obsession” which is extremely common in Ruby world.

Share/Bookmark

Get Rid of That Code Smell – Control Couple

2012-04-11T14:18:11Z

This is a post from the Get Rid of That Code Smell series.

If you are serious about Object Oriented Design and respecting Single Responsibility Principle then you definitely want to get rid of Control Couple code smells. In this post I will show you a simple example explaining how to identify and remove control coupling from your code. I like to think about that code smell also in the context of SRP because I like to apply it to every piece of my system – whether it’s a method, a class or a whole library. I like to be able to describe each piece with a simple sentence saying what it does, what’s the responsibility. With control coupling you introduce multiple responsibilities in a single method which is against SRP and against Object Oriented Design.

Detecting Control Couple Smell

Reek can help you with that – detecting Control Couple is turned on by default. I believe that Pelusa’s “Else Clause” and “Case Statement” lints can also be used to find places in your code with potential control coupling.

A dead-simple example of control coupling could like this:

[crayon-4fae4603ed374/]

Which is pretty self-explanatory. The say method puts an upcased sentence if loud parameter is set to true.

Let me show you a real-world example though. Let’s bring it to the class level – here’s a small snippet from Virtus:

[crayon-4fae4603ed3b9/]

DefaultValue is a class responsible for evaluating a default value of an attribute in Virtus. Its #evaluate method, depending on @value ivar, is deciding how to evaluate the value. We have multiple cases here and all of them are handled within one method. You cannot easily describe this method’s responsibility because it does different things depending on what the @value ivar actually is. If it’s a proc-like object responding to #call we call it, if it’s a duplicable object then we dup it, else we just return it as is.

I found that code pretty ugly. The repercussion of Control Couple smell in this example was that every time we were setting the default value we were performing this if/else check on @value.

Removing Control Couple Smell

The DefaultValue class was refactored by splitting the evaluate logic into 3 sub-classes. Every sub-class implements a self.handle? method which checks if its instance can actually handle the given value. This means that the logic inside #evaluate is now performed only once, prior to deciding which DefaultValue sub-class we want to initialize.

Here’s how it looks like:

[crayon-4fae4603ed401/]

Now for example FromCallable class implementation looks like that:

[crayon-4fae4603ed442/]

I liked that refactor because I could remove the if/else clause from #evaluate method, split responsibilities across 3 sub-classes and gain a small performance boost too.

Summing Up

Even if removing Control Couple smell requires writing a bit more code – it’s worth that price. Getting rid of that smell leads to better Object Oriented Design and helps you with respecting Single Responsibility Principle. I also like that it’s easier to understand what the code does because responsibilities are shared across the objects rather than jamming everything into a few methods that couple the logic to the received arguments.

In the next post we’ll see how to deal with Duplication in our code. Stay tuned.

Share/Bookmark

Get Rid of That Code Smell – Attributes

2012-04-04T12:53:20Z

In this post I will show you why using attribute accessors is a code smell in most of the cases. This is a very convenient feature of Ruby but you should only consider using it if you’re implementing a data-like objects which expose, well, data to other parts of your system. A good example is an Active Record object which exposes its attributes. Another good example could be an object which wraps a response from a remote API and through attribute readers gives you access to data returned in that response. Probably every other use case should be considered as a code smell.

Detecting Attribute Smell

It’s pretty easy to find the attribute smell in your code. Just grep for usage of attr_* in your classes. Pelusa does that for you. Reek can also detect this smell although this check is turned off by default.

Here’s an attribute class which you can instantiate with a name and set its reader visibility. This is an actual piece of code that we used to have in Virtus. The information about reader visibility needed to be public. Initially we had a public attr_reader for the reader_visibility instance variable that was set in the constructor. It was an easy and convenient solution. The related piece of code looked like that:

[crayon-4fae4603f1a74/]

Now in other places we were using the value of reader_visibility so we had pieces of code like this:

[crayon-4fae4603f1ab8/]

This means that we had an instance variable exposed to the other objects which were relying on its value. This was a mistake and definitely a code smell.

Removing Attribute Smell

You probably know how to get rid of that smell, right? We should simply implement a predicate method and this is exactly what was done:

[crayon-4fae4603f1afa/]

This is much better because of two reasons. First of all we hide the private state of an object behind a public predicate method which is simply cleaner. Secondly if, for some reason, the logic for calculating the value of reader_visibility becomes more complex we will simply implement that in the predicate method. Well, it’s probably not going to be the case here but you get the idea.

What about “Tell, Don’t Ask”?

This rule is also related to the attribute smell. Whenever you rely on an object’s attribute to make a decision what to do next – you’re violating “Tell, Don’t Ask” and you’re introducing the attribute smell. Here’s a quick example:

[crayon-4fae4603f1b3d/]

This subtle difference is important because the library object should know how to deal with an empty books array and it should not be a concern “outside” of the library object. We also rely on the books property which indicates the attribute smell.

Summing Up

As you can see the attribute smell is easy to find and fix. The important thing to remember is that when you rely on the internal state of the objects you make future refactorings very difficult. So just don’t do that. Objects should expose as little information about their internal state as possible. Make things publicly visible only if you’re certain it’s really really needed. Rely on API that your objects provide instead of their internal state. This will make your life much easier.

In the next post we’ll deal with the “Control Couple” smell.

Share/Bookmark

Get Rid of That Code Smell

2012-05-11T17:43:03Z

While working on DataMapper 2 libraries we are measuring quality of our code with various code metric tools. Dan Kubb has been using this approach successfully for over 2 years in Veritas which resulted in a beautiful and clean code. When I started working on Virtus I decided to embrace all the practices that Dan introduced in his projects. It was a fantastic experience because Virtus was not a green-field project – I extracted a piece of DataMapper 1 and turned it into a standalone library. The code was already pretty awesome and easy to work with but this didn’t change the fact that most of it was completely refactored after we realized how many code smells were there. Based on this experience I’m starting a small series of blog posts about dealing with various common code smells.

There are a lot of great ruby tools for finding smells in your code. In DM2 projects we use metric-fu which incorporates usage of Flog, Flay, Reek and Roodi. Recently I also started experimenting with Pelusa (I’m hoping to replace *all* the metric tools with Pelusa at some point in the future).

Here’s a list of most common code smells that we’ve identified and removed:

Attribute (using attribute getters/setters)
Control Couple
Duplication
Primitive Obsession
Feature Envy
Utility Function
Large Class
Long Method

In the near future I’ll describe each of these code smells with simple code examples showing you how you can deal with them. This will show what we’ve done but I’m not saying that there are no better ways and as always I’m open for discussions.

What’s the result of removing these (and probably a few more that I don’t recall at the moment) code smells? Really clean and well designed codebase that’s a great pleasure to work with. You can actually see a visual result on Virtus’ CodeClimate profile here. Entire codebase has “A” grade. How awesome is that?

If you want to know how we’ve pushed Virtus to the high level of code quality – watch this space.

Share/Bookmark

DataMapper-2 Presentation From wroc_love.rb Conference

2012-03-26T17:59:25Z

Once again thank you for the awesome event that took place in Wrocław last weekend – the wroc_love.rb conference. We’ve had fantastic and inspiring talks and many great discussions. It’s clear to me that a new era in our community has started and people are willing to learn and embrace patterns from other languages without being scared that we’re going to lose the “agile” aspect of programming in Ruby and Rails.

My personal favorite presentations were Piotr Szotkowski’s “Decoupling Persistence” and Jim Gay’s “It’s Business Time”. We also had a very interesting panel discussion about OOP and Rails.

Most of the talks and discussions should be available online soon. If you’re interested in seeing them make sure to follow wroc_love.rb on twitter and/or watch their blog for any future announcements.

Meanwhile here are the slides from the talk I gave about development of DataMapper 2:

Share/Bookmark

New Virtus Release With Truly Awesome Features

2012-03-26T18:00:08Z

Just a quick announcement that I just pushed a new version of Virtus with support for long awaited features: EmbeddedValue, member type coercions for array/set attributes and ValueObject. Current version is 0.2.0, please give it a try and tell me what you think.

Here’s a quick sneak-preview of what you can do with Virtus:

[crayon-4fae460400e34/]

I hope you’re going to enjoy it and find it useful in your projects. In case of any issues please report them on Github.

Huge props go to Dan Kubb, Emmanuel Gomez and Yves Senn for helping me with recent releases! Thanks guys!

Share/Bookmark

Yes, You Should Write Controller Tests!

2012-03-26T18:00:21Z

It really surprises me that there are people arguing that writing controller tests doesn’t make sense. Probably the most common argument is that actions are covered in acceptence tests along with checking if views are properly rendered. Right? Right…well that’s just wrong! Are you trying to say that your slow acceptance tests are covering every possible controller action scenario? Are you trying to say that, for instance, every redirect that should take place is tested within an acceptance test? Are you also checking every invalid request in acceptance tests? Are you telling me that your acceptance test suite takes 27 hours and 13 minutes to finish because you fully test your controllers there?! Oh I’m sure that your acceptance test suite runs faster and you probably cover only ‘the happy scenarios’ there…which basically means you miss A LOT of test coverage.

A Simple Fact About a Controller

Here’s a simple fact about a controller – it’s a class with methods! Yes, it’s a class with methods I repeat. It should have tests. Every method (action) should be covered by a test.

Why? Well, for the same reason that you write tests for any other class in your application. We want to make sure that our code behaves like expected. It’s also much easier to keep your controllers thin when you have tests. If it’s trivial to write a test for a controller then it’s probably a well implemented controller. If you’re drowning in mocks then you probably want to refactor your controller.

Here’s an example of a users controller with a sample create action:

[crayon-4fae4604025a3/]

As you can see we load a group object in the before filter and then we use that object to create a new user. Pretty dead-simple. If you just write an acceptance spec for this controller I bet you will not cover the case when the group cannot be found. After all people usually write acceptance tests for optimistic scenarios. If they want to cover every case then I’m sure they will miss the deadline ;)

But it’s not just about covering every path! It’s also about the quality of the code. When you look at the example above you probably won’t see any code smells, right? OK so let me write a spec for the create action:

[crayon-4fae4604025ec/]

What happens there? Because we reach deeper into group object, get its users and then use it to build a new user the spec requires more mocks then it should. This is a trivial example but you can probably imagine how a spec would look like if the action was more complicated, had more branching logic and even more structural coupling.

Let’s quickly make a small refactor of the action:

[crayon-4fae460402634/]

Now the spec will be a bit simpler:

[crayon-4fae460402675/]

We should also add an example checking the case where a group is not found. This is going to be a rather rare case but this doesn’t change the fact that we should have a test for it:

[crayon-4fae4604026b9/]

That’s it! Small test, fast test. Code is covered. Controller is thin. Now it’s probably a good idea to write some views specs for users controller too but I’m not going to write about it here, that’s boring ;)

Summing up

Yes, you should write tests for controllers. Whenever somebody asks you if he/she should write them, your answer should be ‘yes!’. There are no good arguments against writing controller tests. It’s code after all, it should be covered by tests. Let the tests drive your design and you will be happy with your controllers, otherwise you might end up with a mess. Without proper tests you won’t be able to truly verify if your controllers are thin and well designed. So please go and write controller tests!

In case you missed it you might want to read Avdi’s great post about “Law of Demeter” where he writes about structural coupling which is mentioned in this post too.

Share/Bookmark

Ruby DataMapper Status

2012-03-28T18:12:51Z

In my recent post I gave you a brief overview of what I think about the state of Ruby ORMs. Since I’m involved in the development of DataMapper project I want to write a little more lines of text to give you a good overview of the current state of DataMapper project and how the future version 2.0 is going to look like.

DataMapper 1.x

Let’s get this straight once and for all: DataMapper was never a pure implementation of the Data Mapper pattern. It has elements of both Data Mapper and Active Record patterns. It has a mapping layer where you can configure mappings between model properties and database columns but this doesn’t change the fact that your models have direct access to the persistence layer and a lot of persistence-related functionality is mixed into them. This is what makes DataMapper an ‘ActiveRecord-ish’ library. After a few years of development it’s become clear that this approach, despite its advantages over ActiveRecord, is still not good enough.

Probably one of the biggest wins of DataMapper is support for many different kinds of data stores. Apart from supporting most common RDBMS databases there are adapters for various key-value stores, NoSQL databases and more. The flip side is that working with multiple data stores at once is not very stable at the moment and that you can’t easily use the full power of various databases, like MongoDB, due to limitations in DataMapper’s API.

The good news is that while working on DataMapper and many of its adapters we’ve learned our lessons. With all that knowledge and experience the work on DataMapper 2.0 has been started.

DataMapper 2.0

First of all the next major version of DataMapper will implement the Data Mapper pattern as described in the PoEAA:

A layer of Mappers that moves data between objects and a database while keeping them independent of each other and the mapper itself.

Martin Fowler martinfowler.com/eaaCatalog/…

DataMapper 2 won’t be a big monolithic library, it will consist of multiple independent pieces that are glued together. Each piece of DM2 is a standalone
library so that you could use it separately from the DM itself.

Let me describe all the related projects.

Veritas – the new relational algebra engine

The core of DM2 is Veritas – the new relational algebra engine developed by Dan Kubb. Why not ARel? The answer is simple – ARel is designed to generate SQL whereas DataMapper needs something more abstract with adapters that can handle different kinds of query dialects. With that premise Veritas is a full-featured relational algebra implementation and there’s already an SQL generator which can build pretty complex queries. More generators will be written soon.

Here’s a quick sneak-preview of how you can build relations with Veritas:

[crayon-4fae46040abe1/]

What’s great about Veritas is that with its level of abstraction you can use it for various things – not just to generate queries. For example there’s an
idea of using Veritas to build a new “migrations” library for DM2 that will easily handle even very complex scenarios.

Virtus – the new model definition and introspection layer

I’ve already introduced Virtus – it’s an extraction of DataMapper’s Property API. During the last few months we’ve made many significant improvements and more features will be added soon too. Virtus already supports defining attributes on your models and can handle many kinds of coercions. You can also implement your own coersion methods if you need them.
In the near future Virtus will be supporting more complex functionality like EmbeddedValue and ValueObject. It’s also possible that with Virtus you will be able to define relationships between POROs.

The important thing about Virtus is that it’s designed to work with PORO and it has nothing to do with any persistence concerns. For example there were
people asking me about things like support for dirty attributes tracking – it’s not going to be included in Virtus.

I should mention that DataMapper 2.0 will be designed to work with POROs that are not extended by Virtus. Virtus will be optional but since it provides a very common functionality I suspect most people will want to use it.

Aequitas – the new validation library

Developed by Emmanuel Gomez, Aequitas is designed to work with any Ruby object and it also has support for Virtus. The idea is similar to what we have in DataMapper 1.x where validations can be derived from property declarations. Aquitas is based on the current
dm-validations and the API is very similar, if not identical. Aequitas has support for custom error messages, I18n, validation contexts and built-in data types which come with their own set of validation rules.

[crayon-4fae46040ac39/]

Session, Dirty Tracking and Unit of Work

In the Data Mapper world every change made to the models loaded into memory is tracked by a specialized object which works like a session. When you’re done
with making changes you can either commit them or rollback. The important thing is that all the complex logic behind tracking what was changed, what was deleted and what was added is handled by the session object. Your models are dumb in this regard, they don’t care about dirty tracking, it’s not their responsibility. It’s one of the major differences between AR and DM.

The session object will be an implementation of the Unit of Work pattern described in PoEAA as:

Maintains a list of objects affected by a business transaction and coordinates
the writing out of changes and the resolution of concurrency problems.

Martin Fowler martinfowler.com/eaaCatalog/…

More technically speaking a session will hold a DAG of commands sorted by their dependencies so that when you commit the session it will know in what order those commands should be executed.

When you think about it you will realize that this pattern is quite common. That’s why it would probably make sense to come up with a general UoW library
and then create an extended version for DataMapper needs.

Here’s an example of how such a session may look like:

[crayon-4fae46040ac84/]

The Mapper

Probably dm-core, the current core DataMapper library, will be completely rewritten and become the mapper layer with a thin query API that delegates most of the heavy work down to a veritas adapter. For your convenience there will be a veritas mapper class that you can inherit from but this doesn’t mean that you won’t be able to write custom mapper classes. The idea is really simple here, a mapper class defines
mappings between PORO and the database schema. In most of the cases this means a direct 1:1 mapping but the huge advantage is that once you need something
custom – you will be able to define it.

This is really the key aspect of using a data mapper library – you define your domain objects so that they correspond to your real world domain as close as
possible. It makes applying practices like “Fast Rails Tests” suggested by Corey Haines come in a natural way because you’ll be implementing the business logic in POROs and have them unit tested in an isolation without the database access.

You will be able to define a mapper class more or less like this:

[crayon-4fae46040accb/]

Roadmap? ETA?

There’s a work-in-progress roadmap for DM2 available here. Regarding ETA it’s really hard to say. We’re taking our time to build all these libraries, there’s a big focus on code quality and proper test and docs coverage. Once the roadmap is finalized we will be able to come up with some ETA.

Here’s the full list of related projects on Github:

veritas – core library
veritas-sql-generator – SQL generator
veritas-do-adapter – DataObjects adapter
veritas-optimizer – relation optimizer
virtus – attributes/coercions
aequitas – validations

…and more will come soon, so stay tuned!

Make sure to follow @datamapper on twitter too :)

If you’re eager to learn more you can always join #datamapper IRC channel. I also understand that this post doesn’t answer many possible questions – feel free to ask them in the comments.

Share/Bookmark