1

as a new Rubyist, I'm running into a recurring problem when it comes to structure my models.

When a method is too long:

  1. I try to refactor to a better/shorter syntax
  2. I try to split some parts into "sub methods"

PROBLEM: I don't know how to split the method properly + whith which tool (private method, modules etc.)

For example:

I need to run Foo.main_class_method

My model looks like this:

class Foo < Applicationrecord
  def self.main_class_method
    [...] # way too long method with nasty iterations
  end
end

I try to split my method to improve lisibility. It becomes :

class Foo < Applicationrecord
  def self.main_class_method
    [...] # fewer code
    self.first_splitted_class_method
    self.second_splitted_class_method
  end

  private
  def self.first_splitted_class_method
    [...] # some code
  end

  def self.second_splitted_class_method
    [...] # some code
  end
end

Result: It works, but I fell like this is not the proper way to do it + I have side effects

  • expected: splitted_methods are not accessible, except inside main_class_method
  • got: I can call Foo.first_splitted_class_method since class methods "ignore" Private. splitted_class_methods under Private are not private

Question: Is it an acceptable way to split main_class_method or is it a complete misuse of private method ?

qrew
  • 31
  • 5
  • 1
    There are no magic rules that make code good or bad. Is splitting a big method into smaller private method accepted? Yes. But what you really need here is a code review and stackoverflow is not the site for that. https://codereview.stackexchange.com/ – max Feb 12 '20 at 15:02
  • 1
    Adding to what @max said: You can create private class methods, it just works a little differently, see: https://stackoverflow.com/questions/4952980/how-to-create-a-private-class-method – murb Feb 12 '20 at 15:04
  • 1
    Visibility is really just one small part of the question. If you have a fat model you might want to ask yourself if what you are doing belongs inside the model. Models have tons of responsibilities (validations, database bindings, i18n, assocations) etc. So if you are doing the typical stuff that makes a model fat like importing data from an API, CSV or somewhere you should consider moving that into an object that just does that job instead of making the model into more of a god object. Just because the class is named User does not mean that it has to do everything user related. – max Feb 12 '20 at 15:09
  • 1
    Thanks for the quick answer. I fixed the not private "side effect" with private_class_method :method_name. I know visibility is more a consequence than a problem itself. Little by little, I'm trying to use the right tools to comply with DRY, SOLID etc. principles. Thanks again ! – qrew Feb 12 '20 at 15:20

1 Answers1

0

  1. Using private method to split your code:

    • Possible but not the real solution if the code belongs somewhere else
    • It's rather about "does it belongs here?" than "does it look nicer?"

  2. To fix the "not private" private class method (original post) :

    • use private_class_method :your_method_name after you defined it
    • or right before
private_class_method def your_method_name
  [...] # your code
end
  1. If your splitting a class/instance method:
    • the splitted_method must be the same type(class/instance) as the main_class_method calling it
    • In the main_method you can call the splitted_method with or without using self.method syntax
class Foo < Applicationrecord
  def self.main_class_method
    # Here, self == Foo class
    # first_splitted_class == class method, I can call self.first_splitted_class_method 
    self.first_splitted_class_method
    # I can also call directly without self because self is implicit
    second_splitted_class_method
  end

  def self.first_splitted_class_method
  end

  def self.second_splitted_class_method
  end

  private_class_method :first_splitted_class_method, :second_splitted_class_method
end
qrew
  • 31
  • 5