Discussion: the `private def` idiom.

[johana-star]

At my company we have begun using the idiom

private def quux(foo, bar)
  foo <= bar
end

Instead of:

private
def quux(foo, bar)
  foo <= bar
end

This makes private methods more explicit and makes it easier to understand which methods are private, as sometimes the private declaration is somewhere in the middle of a large class.

What do folks think of this idiom? Can we integrate a recommendation in the guide, or at least specify this as an acceptable alternative to the private declaration making all methods below it private?

[scudelletti]

IMHO:

If you use a consistent structure in your class definitions and your classes are no longer than one hundred lines of code. I don't think it is hard to see if a method is private or not.

If you have a large class it does not mean that adding verbosity will solve the issue.

In my opinion this:

class XPTO
  def hallo
    #...
  end

  private def hello
    #...
  end

  private def hello_again
    #...
  end
end

Is not so nice as this:

class XPTO
  def hallo
    #...
  end

  private

  def hello
    #...
  end

  def hello_again
    #...
  end
end

It remembers me java. public static void main(String[] args)

[eventualbuddha]

I agree with @strand. I think it is much clearer what's public and what's private for the simple reason that I don't have to (at best) scan upwards until I find–or fail to find–a private line or (at worst) mentally evaluate the code to determine method visibility.

The comparison to Java I don't really take as a negative. Java's static nature allows for some pretty awesome static analysis and refactoring.

[hammerdr]

I personally started on the proper structuring and small classes make this a moot point train, but I actually think that this has a benefit to how people approach writing Ruby code.

This adds some friction to using private methods, which is generally a positive thing. Private methods are lies in Ruby anyway -and- they tend to indicate that there is a concept missing in the code that could be extracted. By more explicitly calling out private methods, we could have the effect of encouraging people to design public-by-default and to extract more classes.

[mikegee]

I agree that the new style should be considered acceptable:

private def quux(foo, bar)
  foo <= bar
end

I don't think this style guide should recommend its usage (yet). See the introductory paragraph of the README:

This Ruby style guide recommends best practices so that real-world Ruby programmers can write code that can be maintained by other real-world Ruby programmers. A style guide that reflects real-world usage gets used, while a style guide that holds to an ideal that has been rejected by the people it is supposed to help risks not getting used at all—no matter how good it is.

private def is nice and clear, but not common. I suspect its usage is still rare in Ruby code written in 2018, let alone all the code we deal with that has been written years ago.

That's not stopping a particular organization from having their own style guide recommending its usage of course.

[hammerdr]

Probably a minor thing, but I definitely recommend against companies forking style guides and trying to maintain their own. Better to just adopt community standards because it makes onboarding easier, context switching less costly, and updating the style guide is "free", especially when you use static code analysis tools like Rubocop :)

[johana-star]

@hammerdr The happy pattern I have seen is "follow the Ruby Style Guide, except in the following cases…"

[johana-star]

Thanks for the discussion, while I think this practice increases readability, it is still uncommon. Closing, if this becomes a more common practice, please re-open this issue.

[kwstannard]

deleted an off topic suggestion

I am in favor of allowing private def as long as I can also ignore putting all private methods together at the bottom as it would make it easier working in legacy Rails applications if private methods were immediately below where they were used.

[marcandre]

I reopened this issue as I think it is a worthwhile discussion and it might change.

I personally like private def. This is the style I am using in the matrix std lib, for example.

The fact that private does not affect constant declarations (nor def self.foo, if you use those) make it even less convenient. I hope that this will change though.

[bbatsov]

Yeah, that'd be awesome. I've always hated the need to use private_const and private_class_method. Based on my observation this gets newcomers every single time and that's why eventually we added cops in RuboCop to flag this.

I think the backward compatibility impact of the change you propose will be a lot less pronounced that most people would think.

[pirj]

In either case, I hope we'll settle on an agreement to put all definitions of methods/attributes/constants in public protected private order.

[marcandre]

@pirj I don't think the order is in question.

Note that private and al. are now even nicer idioms in Ruby 3.0 because we can now write:

protected attr_accessor :a, :b

[bbatsov]

What do folks think of this idiom? Can we integrate a recommendation in the guide, or at least specify this as an acceptable alternative to the private declaration making all methods below it private?

I guess it's fine to mention it as an alternative acceptable style at least, but I'd also advise to avoid mixing the two styles, especially in a single class. It's clear that since this became feasible the approach has been gaining some traction.

[pirj]

@marcandre Yes, I have seen this PR was merged. Awesome.

From the description:

makes it easier to understand which methods are private

It should be easy enough if the code of the class is structured into public and private parts. If you exaggerate far enough, the same argument can be used to explicitly specify the class around method definition to "easier understand in what class the method is defined":

module API
  module V2
    class Authorization
      # 1000 lines of methods here
    end

    class ::API::V2::Authorization
      def can_access_internals?
      end
    end

    # 1000 more lines
  end
end

just a funny note, surely don't take this as a serious proposal. Surely, this code makes it super-clear on what class defines can_access_internals?. But it's excessively verbose at the same time.

And so is an explicit private.

If you use private def and separate public and private parts with private in the middle of the class, it's easy to get tricked by a forgotten private before def and take the method as public, even though it was below the private mark.

as sometimes the private declaration is somewhere in the middle of a large class

For me, there are additional signs if a method is private or public in a large class:

• if it's not documented, it's private
• if it is documented, but flagged as @private

This is the style I am using

I know your code, it's always super-clean and easy to read. But I would not recommend applying the same principles that you use to the general public in isolation from other principles that you apply to the same codebases.

[marcandre]

easier understand in what class the method is defined

Actually, that's a valid point, but that's also why we typically have a rule for this: one (big) class per file.

you use private def and separate public and private parts with private in the middle of the class, it's easy to get tricked by a forgotten private before def and take the method as public, even though it was below the private mark

Absolutely. Unless you use RuboCop and it enforces an order public / protected / private...

I think we agree that:

• either style are viable; be consistent
• be careful about singleton methods

As far as grouping visibility and ordering them public / protected / private, that's what I'm doing, but I am sometimes tempted to follow @kwstannard and have private methods closer to where they are used. Singleton methods are tricky.