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

Empty line before each control structure #82

Closed
volontarian opened this issue Mar 3, 2012 · 5 comments
Closed

Empty line before each control structure #82

volontarian opened this issue Mar 3, 2012 · 5 comments

Comments

@volontarian
Copy link

Regarding breaking up a method into logical paragraphs how about an empty line before control structures like if, while, return / return values, break, maybe also the ternary operator, etc. UNLESS it's the first line at the current "block".

Code sample:

def enforce_ssl?(req)
  enforce = false
  keys_by_type = { 
    :hosts => [:only_hosts, :except_hosts], :path => [:only, :except], 
    :methods => [:only_methods, :except_methods] 
  }

  if !keys_by_type.values.flatten.compact.any? { |option| @options[option] }
    return true
  end

  keys_by_type.keys.each do |type|
    enforce = enforce_ssl_for?(keys_by_type[type], req)

    next unless enforce

    keys_by_type.each do |other_type,sub_keys|
      next if type == other_type || !sub_keys.any? { |option| @options[option] }

      enforce = enforce_ssl_for?(sub_keys, req)

      break unless enforce
    end

    break if enforce
  end

  enforce   
end
@Confusion
Copy link

I think that is a bit too much. I removed some newlines and the resulting version seems more coherent to me:

def enforce_ssl?(req)
  enforce = false
  keys_by_type = { 
    :hosts => [:only_hosts, :except_hosts], :path => [:only, :except], 
    :methods => [:only_methods, :except_methods] 
  }

  if !keys_by_type.values.flatten.compact.any? { |option| @options[option] }
    return true
  end

  keys_by_type.keys.each do |type|
    enforce = enforce_ssl_for?(keys_by_type[type], req)
    next unless enforce

    keys_by_type.each do |other_type,sub_keys|
      next if type == other_type || !sub_keys.any? { |option| @options[option] }
      enforce = enforce_ssl_for?(sub_keys, req)
      break unless enforce
    end

    break if enforce
  end

  enforce   
end

@volontarian
Copy link
Author

Looks good for me, too :-)

@calebhearth
Copy link

I like to organize code into logical blocks like @Confusion's example. Lines can still belong together even if they aren't in the same control block.

@bbatsov bbatsov closed this as completed Apr 9, 2012
@mikecmpbll
Copy link

this is the single biggest problem with code style consistency across our codebases and we don't have the power to do anything about it with rubocop atm. i know this is closed but it's a huge +1 from me :).

@pirj
Copy link
Member

pirj commented Mar 5, 2021

@mikecmpbll I suggest you contribute a cop to RuboCop that would take care of this.
There doesn't seem to be enough feedback to add this as a guideline to the style guide yet.

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

6 participants