--- title: 'Refactoring: Replace Conditional with Polymorphism' teaser: How to work through existing code to add polymorphic behavior. tags: web,good code author: Joe Ferris published_on: 2012-09-17 --- I recently discovered the `validates!` method in ActiveModels' validations API. To paraphrase the [official documentation](http://api.rubyonrails.org/classes/ActiveModel/Validations/ClassMethods.html#method-i-validates-21): This method is used to define a validation that cannot be corrected by the end user and is considered exceptional. Each validator defined with bang or the :strict option set to true will raise ActiveModel::StrictValidationFailed instead of adding an error when the validation fails. This method was exactly what I had been looking for, so I set to using it [with gusto](http://media.tumblr.com/tumblr_macfytlJMl1qzocnw.jpg). Of course, before I gusted out a quick `validates!` call, I went to write a failing test: ```ruby describe AccessRequest do it { should allow_value('Pending', 'Rejected', 'Accepted').for(:status) } it { should_not allow_value('Pancake').for(:status) } end ``` These tests work fine for the usual `validates_inclusion_of` approach, but there was nothing I could add to make them work with the `:strict` option, since `allow_value` doesn't expect exceptions. Since [shoulda-matchers](https://github.com/thoughtbot/shoulda-matchers) is open source, I rolled up my sleeves and [jumped in](https://github.com/thoughtbot/shoulda-matchers/commit/025023d43). My tests now worked with the simple addition of a `strict` call to the matcher: ```ruby describe AccessRequest do it { should allow_value('Pending', 'Rejected', 'Accepted'). for(:status).strict } it { should_not allow_value('Pancake'). for(:status).strict } end ``` ## What's that smell Everything was working great, but my change had introduced a terrible smell: [AllowValueMatcher](https://github.com/thoughtbot/shoulda-matchers/blob/025023d432302159ea03a36442aa070499392d72/lib/shoulda/matchers/active_model/allow_value_matcher.rb) now had six methods that forked their behavior based on whether or not `strict?` was set. This had a number of downsides: * **Maintainability:** we now had two ways of looking for error messages: from the errors collection, and from an exception raised during validation. This commit makes it clear that at least six places need to change if we add any more. * **Maintainability:** this commit revealed that `AllowValueMatcher` had too many concerns: it both finds error messages to check, and parses the options and errors to determine whether or not the errors were expected. * **Readability:** `AllowValueMatcher` is now too large to understand at a glance, weighing in at 26 methods. * **Readability:** having two logical paths for every method resulted in a lot of long method names. In addition, having so many methods makes it more difficult to reduce the vertical distance between a method and the methods that reference it. * **Testability:** making sure that the various forks work as expected requires testing everything in `AllowValueMatcher` twice. There will always be several methods in between the test harness and the method under test, making the test more difficult to write, understand, and debug. I had a fever, and the only cure was more objects. The prescription: a healthy dose of [Replace Conditional with Polymorphism](http://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html). ## The skinny The [full change] I pushed to solve this problem is fairly large: * I moved all of the error finding and description functionality into two new classes: [ValidationMessageFinder] and [ExceptionMessageFinder]. * The matcher is [assigned a message finder factory] based on whether or not it's strict. * Each [method that used to have a conditional] now delegates to its [message finder]. [assigned a message finder factory]: https://github.com/thoughtbot/shoulda-matchers/commit/87327c225a4c9d492a370fe8ee91631433145b69#L1L30 [ExceptionMessageFinder]: https://github.com/thoughtbot/shoulda-matchers/blob/87327c225a4c9d492a370fe8ee91631433145b69/lib/shoulda/matchers/active_model/exception_message_finder.rb [message finder]: https://github.com/thoughtbot/shoulda-matchers/commit/87327c225a4c9d492a370fe8ee91631433145b69#L1R153 [method that used to have a conditional]: https://github.com/thoughtbot/shoulda-matchers/commit/87327c225a4c9d492a370fe8ee91631433145b69#L1R137 [ValidationMessageFinder]: https://github.com/thoughtbot/shoulda-matchers/blob/3dbf5d4f0d5943ae11f7c770a172c9c71f24ac87/lib/shoulda/matchers/active_model/validation_message_finder.rb [full change]: https://github.com/thoughtbot/shoulda-matchers/compare/96d32c2...87327c2 This improves the code in a few ways: * **Maintainability:** adding another way to find error messages just requires implementing the implicit MessageFinder interface. * **Readability:** the logic for finding messages of a particular type is now encapsulated in a small class per type, reducing the logical distance between methods and references and giving each method a clearer logical context. * **Testability:** the method under test can now be called directly from the test harness. The problem and results are fairly easy to understand, but let's dig into the process of cleaning up this mess. ## One branch at a time, one method at a time This is a higher level refactoring that ideally consists of many, small refactorings. I started with one logical branch: finding errors from the errors collection. I created a new method just for the logical branch I was changing: ```ruby def description if strict? "strictly #{allow_description(allowed_values)}" else allow_description(allowed_values) end end private def allow_description(allowed_values) "allow #{@attribute} to be set to #{allowed_values}" end ``` This change is known as [Extract Method](http://www.refactoring.com/catalog/extractMethod.html). This refactoring didn't change the API at all and required no test changes. Next, I added a reference to the object I planned on providing: ```ruby def description if strict? "strictly #{message_finder.allow_description(allowed_values)}" else message_finder.allow_description(allowed_values) end end ``` This fails because `message_finder` isn't defined. I defined that as a hard-coded reference to the first implementation I planned to write: ```ruby def message_finder @message_finder ||= ValidationMessageFinder.new(@instance, @attribute) end ``` Now I had a failure because that class was undefined. I added my first test: ```ruby context '#allow_description' do it 'describes its attribute' do model_class = define_model(:example, :attr => :string) instance = model_class.new finder = Shoulda::Matchers::ActiveModel::ValidationMessageFinder.new( instance, :attr ) description = finder.allow_description('allowed values') description.should == 'allow attr to be set to allowed values' end end ``` The failures from this test guided me to define the class and implement the `initialize` method. The last change was to move the original `allow_description` method from `AllowValuesMatcher` to my new class ([Move Method](http://www.refactoring.com/catalog/moveMethod.html)). That change caused my new test to pass. At this point, all tests for `AllowValueMatcher` also passed. The process of extracting and moving a method is known as [Extract Class](http://www.refactoring.com/catalog/extractClass.html). I continued to extract and move methods one at a time until every non-strict branch in a conditional was delegated to the message finder. This change resulted in my [first commit](https://github.com/thoughtbot/shoulda-matchers/commit/3dbf5d4f0d5943ae11f7c770a172c9c71f24ac87) to replace the conditionals. At this point, I had extracted a new class, but all six conditionals remained. I wanted to move the other half of each branch into another new class, so I needed a way to get the new class into the mix. I changed `AllowValueMatcher` to start out with a `ValidationMessageFinder`: ```ruby def initialize(*values) @values_to_match = values @strict = false @message_finder_factory = ValidationMessageFinder @options = {} end ``` I changed `AllowValueMatcher#strict` to replace the finder factory: ```ruby def strict @message_finder_factory = ExceptionMessageFinder @strict = true self end ``` And I changed `AllowValueMatcher#message_finder` to use the factory: ```ruby def message_finder @message_finder ||= @message_finder_factory.new(@instance, @attribute) end ``` Then I repeated this process of extracting a class and moving extracted methods one at a time, this time for the strict branch of each conditional. Each replaced method in this second pass went through three stages: ```ruby # Original method def description if strict? "strictly #{message_finder.allow_description(allowed_values)}" else message_finder.allow_description(allowed_values) end end # Extract positive branch to a method and move it to `ExceptionMessageFinder` def description if strict? message_finder.allow_description(allowed_values) else message_finder.allow_description(allowed_values) end end # Both branches are the same. Simplify! def description message_finder.allow_description(allowed_values) end ``` After removing all six conditionals, the `@strict` attribute and `strict?` method were no longer necessary, so I removed them. At this point, I made my [second commit](https://github.com/thoughtbot/shoulda-matchers/commit/87327c225a4c9d492a370fe8ee91631433145b69), completing my higher-level refactoring. ## Putting it all together This refactoring involved many steps, but each step was small, easy to follow, and left me with a functioning system and passing tests. Improving code using many small steps (refactoring) is faster and less error-prone than trying to change a lot of code in one large step (rewriting). I also made frequent commits during this process so that I could easily step backwards if any of my steps were a mistake. Before pushing this, I rebased and squashed most of my commits together. I finished it up with the inevitable fix for Ruby 1.8.7 and put together a [pull request](https://github.com/thoughtbot/shoulda-matchers/pull/151) to get feedback from other developers. Next time you're unhappy with your code, resist the temptation to rip it out and write it fresh, even if you're only replacing a single class or a few methods. Try using some of the small steps described above for a safer, less stressful coding session. ### Next Steps & Related Reading Detect emerging problems in your codebase with [Ruby Science][ruby-science]. We'll deliver solutions for fixing them, and demonstrate techniques for building a Ruby on Rails application that will be fun to work on for years to come. [Grab a free sample of Ruby Science today!][ruby-science] [ruby-science]: http://rubyscience.com?utm_source=giantrobots&utm_medium=blog&utm_campaign=remarket&utm_term=testing