Is it okay to call a private method of a parent class's subclass from a module which is included in the parent class in rails?

Question

Is it okay to call a private method of a parent class's subclass from a module which is included in the parent class especially when it concerns ApplicationController, Controllers and lib modules in Rails?

Consider if required to change the controller name the method name to reflect the model name(to Article) change.

I feel this is really bad coding and wanted to know what community thinks about this

Example from a Rails Application:

/lib/some_module.rb

module SomeModule
  include SomeModuleResource

  def filtering_method
    calling_method
  end

  def calling_method
    fetch_object
  end
end

/lib/some_module_resource.rb

module SomeModuleResource
  def fetch_object
    note
  end
end

/app/controllers/application_controller.rb

class ApplicationController < ActionController::Base
  include SomeModule

  before_action :filtering_method

end

/app/controllers/notes_controller.rb

class NotesController < ApplicationController

  def show
  end

  private

  def note
    @note ||= Note.find(param[:id]))
  end
end

I wonder what you try to achieve? Why don't you just call `note` in the `show` method or define a `before_action :note` in the controller. IMHO that would more readable and much easier to understand. — spickermann, Oct 15 '17 at 08:34
I am asking this because i have seen this code in an application that i am working on and i need to justify to others that this is wrong way of coding. — Satya, Oct 15 '17 at 08:39
Did you ask the other developers why they did it that way? There must be a reason why they thought it is a good idea to replace a single line of code with two modules and several steps of indirection. I agree that this example looks ridiculous complex at first glance but there must be a reason or a history behind the decision to introduce this complexity. — spickermann, Oct 15 '17 at 08:46
They did this to avoid re-writing note method again in the module (to be precise rubocop saying module too long). This happens to access all the controllers similarly. So if i change something esp. rename the method in such large codebase, we will be inducing code failures which was my point. BTW methods here are representation and not the actual code. As i can't share the exact code as i am bound by NDA. — Satya, Oct 15 '17 at 08:51

3limin4t0r · Accepted Answer · 2017-10-15T22:19:24.410

I'm of the opinion that this is not necessary bad, although when you expect a certain interface (methods, variables, etc.) from the class that includes the module I would add the following:

module SomeModuleResource
  def fetch_object
    note
  end

  private

  def note
    raise NotImplementedError
  end
end

This way, when #note is called without implementing it (because you forgot it was needed or whatever) a NotImplementedError is raised.

Another option is to work around it and create a more general solution. For example, if all controllers behave the same way you described above you can do the following:

module SomeModuleResource
  def fetch_object
    note
  end

  private

  def note
    klass = params[:controller].classify.constantize
    instance = klass.find(params[:id])

    var_name = "@#{klass.underscore}"
    instance_variable_set(var_name, instance) unless instance_variable_get(var_name)
  end
end

You could also create a class helper method like before_action so that you can pass your own implementation.

module SomeModule
  include SomeModuleResource

  def self.included(base)
    base.extend(ClassMethods)
  end

  def filtering_method
    calling_method
  end

  def calling_method
    fetch_object
  end

  module ClassMethods
    def custom_before_action(&block)
      define_method(:note, &block)
      private :note

      before_action :filtering_method
    end
  end
end

Now you can use custom_before_filter { @note ||= Note.find(params[:id]) } in every controller (after including).

The above is just to present you with ideas. I'm sure you could find better solution to the problem, but this hopefully points you in the right direction.

See: Alternatives to abstract classes in Ruby?. Or search for abstract classes in Ruby and you'll find more on this subject.

I agree with the 1st code sample. If a method from sub class is expected to be implemented then we should make sure it is implemented. If it is not forced to do so and on later changes the code would break. — Satya, Oct 16 '17 at 05:37

Is it okay to call a private method of a parent class's subclass from a module which is included in the parent class in rails?

1 Answers1