Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take a position on indentation of private/protected/public #31

Closed
burke opened this issue Sep 19, 2011 · 8 comments
Closed

Take a position on indentation of private/protected/public #31

burke opened this issue Sep 19, 2011 · 8 comments

Comments

@burke
Copy link

burke commented Sep 19, 2011

eg:

class Foo
  def bar  
  end
private # this 
  def baz
  end
  protected # versus this
  def quux
  end
end

I'm not sure which is the most widely-used. I know Rails uses the former, but I don't have a strong opinion. I feel like a style guide that suggests using private where applicable should also dictate how to indent it.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 19, 2011

I've generally favoured the latter style (it's the one used in TRPL book and in the ruby-mode authored by Matz), but I'm not quite fond of it, so I've decided to merge the Rails style indentation you've suggested. We'll see how the community would react - I personally think that on some level it improves the readability of the code.

@bbatsov bbatsov closed this as completed Sep 19, 2011
@mitio
Copy link
Contributor

mitio commented Sep 19, 2011

I've seen and use the latter style of indenting access modifiers (with the appropriate blank lines around the modifier).

However, AFAIK Rails uses a modification of the first version and also the second version (again showing lack of consistency). They use the "first" version in modules and the "second" one in classes, but I can't say that this is consistent and I also don't like it. This is the modification of the first version it uses:

class SomeClass
  def public_method
  end

  private

    def private_methods
    end
end

I haven't seen @burke's first option used in Rails.

Again, I would vote for the indented access modifier, i.e. the latter version.

@burke
Copy link
Author

burke commented Sep 19, 2011

Yeah, I definitely want to avoid deferring to Rails here. This is one part of the project's coding style that absolutely baffles me.

This is a good article discussing the options http://fabiokung.com/2010/04/05/ruby-indentation-for-access-modifiers-and-their-sections/

I can get behind either style 2 or style 3 (The ones in my earlier comment). I don't think 1 and 4 should be considered, nor should Rails' style.

I just skimmed my gems directory (sans rails) and found a pretty even split between 2 and 3.

Maybe it would be a good idea to model after ARel's style here? https://gist.github.com/1227301

@burke
Copy link
Author

burke commented Sep 19, 2011

Aaron is a pretty inconsistent with his spacing around access modifiers, but I would write that out as something along the lines of:

  • Do not indent access modifiers (public/private/protected).
  • Leave a blank line above and below access modifiers, except where the modifier appears at the top of a class context. In this case, omit the blank line above.

[examples]

@mitio
Copy link
Contributor

mitio commented Sep 19, 2011

+1 for "style 2" (no identation, ARel style, etc :)

@DAddYE
Copy link

DAddYE commented Sep 19, 2011

In padrino we use this:

class SomeClass
  def public_method
  end

  private

    def private_methods
    end
end

@burke
Copy link
Author

burke commented Sep 19, 2011

I get the appeal of the rails/padrino style, but to me it feels even more wrong than out-denting the access modifiers, since it implies a scope has been opened. It makes it very difficult to trace a method back up to its enclosing context, since there are now either 2 or 4 spaces of indentation between the class line and the def line. This gets to be a real challenge in files like active_record/base.rb (not to imply that a 2200-line file isn't a code smell in and of itself).

After thinking about this some more, I think I'm going to have to re-cast my vote for style 2. It may not stand out as much, but it is semantically much more consistent and clear.

EDIT: It also does have the benefit of being more widely-published.

@DAddYE
Copy link

DAddYE commented Sep 19, 2011

Im pretty open to this, I tried all 3 versions and I can't say what is better, I prefer the 3° way only for esthetic reason, btw yep, you are right.

nilbus added a commit to nilbus/ruby-style-guide that referenced this issue Nov 11, 2011
This has been discussed previously in rubocop#31, but I much disagree with the
stance that was decided on. I want to open this up again for discussion.

The problem with having private in line with the method definitions is
that it goes unnoticed. There's no clear distinction between public and
private methods. It may not be so obvious here, but it's a lot more
important when your classes are hundreds of lines long.

    class class Foo
      def bar
      end

      private
      def baz
      end
    end

Outdent style, on the other hand, is quite visible and easy to read.
It's not semantically correct, but it's very similar to other similar
constructs that make sections of code. While `private` is technically a
method call, it acts to section off code, just like if/else and
begin/rescue. @burke is correct in rubocop#31 that it does not open a new
scope, but that hardly matters in the context of a class definition.
Similarly with a begin/rescue block, code in the rescue block can still
reference variables created in the begin section.

    if
      foo
    else
      bar
    end

    begin
      foo
    rescue
      bar
    end

    class Foo
      def bar
      end

    private
      def baz
      end
    end

Finally, although no project's style stands as law, the Rails source has
certainly been the most influential in the Ruby community for setting
standards for code style. Much of their new code now uses the style I'm
suggesting.

For reference, this article talks about the pros and cons of each style.
http://fabiokung.com/2010/04/05/ruby-indentation-for-access-modifiers-and-their-sections/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants