This is not the first post cautioning the use of modules on our blog. However, while I do mostly agree with the points raised in Josh's blog, this is a different set of common mistakes I see when using modules.
The use of include SomeModule leads to obfuscated code. Rather than being explicit with the boundaries between your behavior, you are creating "misdirection masquerading as abstraction."[1]
What's wrong with mixins?
They are a violation of the Single Responsibility Principle
The use of include SomeModule is almost a violation of the SRP by definition. We have one class with its own behavior, and we are including additional behavior contained in a module. By my count, this is at least two reasons to change, meaning two responsibilities. Not only that, but you still have duplicated code if a module is included in more than one class.
By pulling duplicated code into a shared mixin, you haven't really removed any duplication. Files are just a convenient means for serializing code, but the methods still exist on both classes. From the outside observer, the code is duplicated, which includes your tests.
Both of these violations make the code much harder to test, which brings me to my next point.
They are hard to test.
When including a module on more than one object, you put yourself in a paradox with unit tests. On one hand, there is the desire to not stub the class under test, while on the other hand, there is a desire to remove duplication in your tests.
Why not stub the class under test you say? Stubbing the class under test can lead to hard to read tests and will make it harder to refactor the class, since something as simple as a method rename may break tests stubbing that method.
This leads us to testing the same code in multiple places, one for each place the module is included. When that code changes, you will have to update tests in each place the module is used, and likely an additional location for the module itself.
While there are tools like RSpec's shared examples that can be used to remove test duplication, more often than not, I find removing the duplication in this way leads to complicated, hard to understand tests. The fact that you have code that is hard to cleanly test is an indication of poor design and not that your testing framework needs new features.
They make it harder to debug code.
Including methods via a module hides where those methods come from. If you include n modules on an object, you have at minimum n + 1 places to look to find the method. It could be in any of the included modules or in the class itself.
It gets even worse if you have different modules that define the same method. You won't get an "AlreadyDefinedMethod" error. At best, you will likely get cryptic errors that show themselves as strange behavior. No one is going to get confused if you use Foo.bar instead of including Foo and calling self.bar, however I've seen plenty of confusion and time lost created by unclear code. In this regard, it is very similar to monkey patching a class in that you may not know where behavior is coming from and may end up overwriting your own methods.
The use of the self.included hook can also lead to code that is hard to reason about, but Josh's blog already covered that in detail, so I will just point you to that. The short version is that proper use of a module can sometimes depend on you knowing details about its implementation.
They are often used as a crutch.
I have worked on a variety of large applications, some which contain God Objects. Oftentimes, I will see what I call a partial-refactoring done using modules. This happens when you take the time to organize a class' methods into module, and include them on the original God Object. Sometimes the tests get moved too. While this does have some benefit, it doesn't really fix the problem. You still have one God object with the same number of public methods. The methods have just been reorganized. The biggest flaw with this problem is that people will still continue to add to the God Object.
That is not to say that pulling methods into a module is not a good first step, but you should finish the job. Make the module it's own class, and definitely make sure to move the tests as well.
Alternatives to Modules as Mixins
Class methods defined on a class
class Foo
def self.bar
end
end
Class methods defined on a module
module Foo
def self.bar
end
end
Instance methods
class Foo
def bar
end
end
The key difference between accessing methods via included modules and these alternatives is the explicitness. With mixins, you have unclear boundaries between your objects, making it harder to create or even see the correct abstractions. You might even had a hard time seeing what your current abstractions are. Every time you call a method on a dependency, you should be able to immediately know where that method lives.
By using any of these alternatives to modules, you are informing the next person reading the code where the called code lives. If you separate responsibilities in your code and make those boundaries explicit, you will be doing that person a big favor.
