Skip to content

Instantly share code, notes, and snippets.

@ahoward
Created May 8, 2012 15:39
Show Gist options
  • Select an option

  • Save ahoward/2636430 to your computer and use it in GitHub Desktop.

Select an option

Save ahoward/2636430 to your computer and use it in GitHub Desktop.
style is the only thing.
# shitty
attributes[:name] = options[:name] unless options[:name].blank?
attributes[:first_name] = options[:first_name] unless options[:first_name].blank?
attributes[:last_name] = options[:last_name] unless options[:last_name].blank?
attributes[:linkedin_id] = options[:linkedin_id] unless options[:linkedin_id].blank?
attributes[:company] = options[:company] unless options[:company].blank?
attributes[:company_id] = options[:company_id] unless options[:company_id].blank?
attributes[:linkedin_picture_url] = options[:linkedin_picture_url] unless options[:linkedin_picture_url].blank?
# shitty, but i can read it...
attributes[:name] = options[:name] unless options[:name].blank?
attributes[:first_name] = options[:first_name] unless options[:first_name].blank?
attributes[:last_name] = options[:last_name] unless options[:last_name].blank?
attributes[:linkedin_id] = options[:linkedin_id] unless options[:linkedin_id].blank?
attributes[:company] = options[:company] unless options[:company].blank?
attributes[:company_id] = options[:company_id] unless options[:company_id].blank?
attributes[:linkedin_picture_url] = options[:linkedin_picture_url] unless options[:linkedin_picture_url].blank?
# at least it's DRY, but fuck it's hard to read...
[:name, :first_name, :last_name, :linkedin_id, :company, :company_id, :linkedin_picture_url].each do |key|
attributes[key] = options[key] unless options[key].blank?
end
# minimum required to be good
#
# * the variable name makes the intention clear
# * the list isn't jammed onto one line hiding typos and confounding quick editing
# * it's primed for configuration (whitelist is an option)
# * stack errors come from *one line*
# * it separates the logic of listing what to whitelist and how to extract a key from options
# * reading a commit diff is actually possible (thanks @drbrain)
#
whitelist = [
:name,
:first_name,
:last_name,
:linkedin_id,
:company,
:company_id,
:linkedin_picture_url
]
whitelist.each do |key|
attributes[key] = options[key] unless options[key].blank?
end
@ahoward

ahoward commented May 8, 2012

Copy link
Copy Markdown
Author

but that is the next best.

@zliang-min

Copy link
Copy Markdown

For one-time-use values, I prefer @steveklabnik 's style. But instead of putting a comment on it, I would give a meaningful name to the block argument, like this:

[ 
  :name,
  :first_name, 
  :last_name,
  :linkedin_id,
  :company,
  :company_id,
  :linkedin_picture_url
].each do |accessable_attr|
  attributes[accessable_attr] = options[accessable_attr] if options[accessable_attr].present?
end

@steveklabnik

Copy link
Copy Markdown

I personally would write this without the comment, but added it here to address your 'variable gives a good name' concern.

In 'real' code, I might actually do

   def whitelist
     %s[
      name
      ...
   end

If I planned on using it elsewhere in the class. Then it'd end up being almost the same. But really, without any context here, it's impossible to say what's 'best.'

Except #1 is clearly terrible.

@ahoward

ahoward commented May 8, 2012

Copy link
Copy Markdown
Author

for sure coding is a process. hopefully people are actively moving from the top of that gist somewhere near the bottom. it's okay to not always start there or to take a different route - the motivation to get there is almost as important as getting there; since it's unlikely we ever will...

@steveklabnik

Copy link
Copy Markdown

👍

@ahoward

ahoward commented May 8, 2012

Copy link
Copy Markdown
Author

@drbrain pointed out that an additional advantage is that the last example diffs well in a commit

@rab

rab commented May 9, 2012

Copy link
Copy Markdown

I will also point out that the Ruby parser allows a comma after the last item in an Array literal (or a Hash) such as:

    whitelist = [ 
      :name,
      :first_name, 
      :last_name,
      :linkedin_id,
      :company,
      :company_id,
      :linkedin_picture_url,
    ]

This has the advantage of having a single-line diff when the next whitelist item is added. The alternative is to have a patch like:

       :company_id,
-      :linkedin_picture_url
+      :linkedin_picture_url,
+      :pet_name
     ]

Rather than simply:

       :linkedin_picture_url,
+      :pet_name,
     ]

@ahoward

ahoward commented May 9, 2012

Copy link
Copy Markdown
Author

i am a huge fan of 'trailing comma' for that very reason. however, years ago matz threatened to make it go away in 1.9.ish.... can anyone confirm otherwise so as to advocate resumed usage?

@rab

rab commented May 9, 2012

Copy link
Copy Markdown

Well, for Hash at least, the Ruby Spec project codifies the "hanging comma":

https://github.com/rubyspec/rubyspec/blob/master/language/hash_spec.rb#L48

And similar specs for a hanging comma at the end of an argument list for send:

https://github.com/rubyspec/rubyspec/blob/master/language/versions/send_1.9.rb

I don't see anything equivalent for Array unfortunately. However, there does seem to be support in the grammar parse.y for it:

(excerpted from four locations in the file)

primary     : literal
        | tLBRACK aref_args ']'
            {
            /*%%%*/
            if ($2 == 0) {
                $$ = NEW_ZARRAY(); /* zero length array*/
            }
            else {
                $$ = $2;
            }
            /*%
            $$ = dispatch1(array, escape_Qundef($2));
            %*/
            }
aref_args   : none
        | args trailer
            {
            $$ = $1;
            }
        | args ',' assocs trailer
            {
            /*%%%*/
            $$ = arg_append($1, NEW_HASH($3));
            /*%
            $$ = arg_add_assocs($1, $3);
            %*/
            }
        | assocs trailer
            {
            /*%%%*/
            $$ = NEW_LIST(NEW_HASH($1));
            /*%
            $$ = arg_add_assocs(arg_new(), $1);
            %*/
            }
        ;

trailer     : /* none */
        | '\n'
        | ','
        ;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment