views:

166

answers:

4

I have a large class with lots of methods and it's starting to get a bit unorganized and hard to navigate. I'd like to break it up into modules, where each module is a collection of class and instance methods. Perhaps something like this:

UPDATE: I've now realized that this is a pretty poor example. You probably wouldn't want to move validations or attributes out of the core class.

class Large
  include Validations
  include Attributes
  include BusinessLogic
  include Callbacks
end

After reading Yehuda's post about Better Ruby Idioms, I'm curious how others are tackling this problem. Here's the two methods I can think of.

First Method

module Foo
  module Validations
    module ClassMethods
      def bar
        "bar"
      end
    end

    module InstanceMethods
      def baz
        "baz"
      end
    end
  end

  class Large
    extend Validations::ClassMethods
    include Validations::InstanceMethods
  end
end

Second Method

module Foo
  module Validations
    def self.included(base)
      base.extend ClassMethods
    end

    module ClassMethods
      def bar
        "bar"
      end
    end

    def baz
      "baz"
    end
  end

  class Base
    include Validations
  end
end

My questions are:

  • Is there a better way to do this?
  • How do you get a one-liner module mixin for a set of class/instance methods with the least amount of magic?
  • How do you namespace these modules to the base class without namespacing the class itself?
  • How do you organize these files?
+7  A: 

Breaking a class into modules, while tempting (because it's so easy in Ruby), is rarely the right answer. I usually regard the temptation to break out modules as the code's way of telling me it wants to be split into more tightly-focussed classes. A class that's so big you want to break it into multiple files is pretty much guaranteed to be violating the Single Responsibility Principle.

EDIT: To elaborate a bit on why breaking code into modules is a bad idea: it's confusing to the reader/maintainer. A class should represent a single tightly-focussed concept. It's bad enough when you have to scroll hundreds of lines to find the definition of an instance method used at the other end of a long class file. It's even worse when you come across an instance method call and have to go looking in another file for it.

Avdi
I agree with you that this could be code smell. I also think that the key to readability is having code organized into small, tightly focused sections. This is my motivation for splitting a class into smaller sub-sections. To take an example from Rails, many people separate authentication logic from their other code by including it into their models and controllers from modules.
Ben Marini
If you just want to split your code up into files you don't have to use modules; you can just re-open the class in each file. It's still a bad idea, though. Better to divest functionality into smaller classes, each a tight group of business logic, validations, callbacks, etc. related to a particular part of the larger class. After all, validations are part of the business rules too; it doesn't make sense to move them *farther* from the other business logic.
Avdi
+3  A: 

After doing what Avdi said, these are the things I would do before putting anything into a module:

  1. Whether this module can or will be used in any other class?
  2. Would it make sense to extract the functionality of these modules into a different or base class?

If the answer for 1 is no and 2 is yes then IMHO that indicates to better have a class rather a module.

Also, I think putting attributes in a module is conceptually wrong because classes never share their attributes or instance variables or in other words their internal state with any other class. The attributes of a class belongs to that class only.

Business logics do definitely belong to the class itself and if the business logic of class A has some common responsibilities with class C then that needs to be extracted into a base class to make it clear instead of just putting it into a module.

nas
A: 

The standard idiom seems to be

foo.rb
foo/base.rb
foo/validations.rb
foo/network.rb
foo/bar.rb

and foo.rb would be something like

class Foo
  include Foo::Base
  include Foo::Validations
  include Foo::Network
  include Foo::Bar
end

This is the standard idiom, and it works fairly well for letting you break things up. Don't do class methods vs instance methods. Those are generally pretty arbitrary distinctions, and you're better off putting code that deals with similar subjects together. That will minimize how many files you have to touch for any given change.

BEWARE: Rails can get confused by nesting models like this, at least if everything were classes. I think it'll do better with all the nested files just being modules, but you'll have to see. I'm still suggesting this because it's the normal idiom used by the Ruby community, but you may have to avoid having both a foo.rb and a foo/ directory amongst your Rails models (if that's the kind of class you're talking about).

edebill
A: 

Although including different modules will work, it is generally more troublesome than simply reopening the class in multiple places.

There is a (very simple) gem that you can use to makes this as pretty as can be: concerned_with

Example (from the readme)

# app/models/user.rb
class User < ActiveRecord::Base
  concerned_with :validations,
                 :authentication
end

# app/models/user/validations.rb
class User < ActiveRecord::Base
  validates_presence_of :name
end

#app/models/user/authentication.rb
class User < ActiveRecord::Base
  def self.authenticate(name, password)
    find_by_name_and_password(name, password)
  end
end
Marc-André Lafortune
Thanks, this is very interesting. Looks like it leverages ActiveSupport::Dependencies#require_dependency which is not as simple. Conceptually, pretty straightforward. Reopen the class.
Ben Marini