to_proc_icky! shouldn't be a 10 on ruby 1.9+ #21

Closed
JohnAmican opened this Issue Feb 26, 2013 · 4 comments

Comments

Projects
None yet
3 participants
@JohnAmican

I'm assuming this score was given because of the performance hit you used to get in 1.8 for doing something like ["1", "2", "3", "4"].map(&:to_i) vs. ["1", "2", "3", "4"].map { |n| n.to_i }.

Can we at least make this score conditional based on what version of Ruby is running flog?

@ghost ghost assigned zenspider Mar 5, 2013

@zenspider

This comment has been minimized.

Show comment Hide comment
@zenspider

zenspider Mar 5, 2013

Owner

Hah! I forgot about that.

Looking into it, no... I'm recommenting the case statement to make it more clear:

    case arg.first
    when :lvar, :dvar, :ivar, :cvar, :self, :const, :colon2, :nil then # f(&b)
      # do nothing
    when :lit, :call then                                              # f(&:b)
      add_to_score :to_proc_normal
    when :lasgn then                                                   # f(&l=b)
      add_to_score :to_proc_lasgn
    when :iter, :dsym, :dstr, *BRANCHING then                          # below
      # f(&proc { ... })
      # f(&"#{...}")
      # f(&:"#{...}")
      # f(&if ... then ... end") and all other branching forms
      add_to_score :to_proc_icky!
    else
      raise({:block_pass_even_ickier! => arg}.inspect)
    end

so the symbol to_proc is only 5 points. Makes me want to increase icky even more as DAMN those are gross. (and yes, they were all found in the wild).

Owner

zenspider commented Mar 5, 2013

Hah! I forgot about that.

Looking into it, no... I'm recommenting the case statement to make it more clear:

    case arg.first
    when :lvar, :dvar, :ivar, :cvar, :self, :const, :colon2, :nil then # f(&b)
      # do nothing
    when :lit, :call then                                              # f(&:b)
      add_to_score :to_proc_normal
    when :lasgn then                                                   # f(&l=b)
      add_to_score :to_proc_lasgn
    when :iter, :dsym, :dstr, *BRANCHING then                          # below
      # f(&proc { ... })
      # f(&"#{...}")
      # f(&:"#{...}")
      # f(&if ... then ... end") and all other branching forms
      add_to_score :to_proc_icky!
    else
      raise({:block_pass_even_ickier! => arg}.inspect)
    end

so the symbol to_proc is only 5 points. Makes me want to increase icky even more as DAMN those are gross. (and yes, they were all found in the wild).

@grantovich

This comment has been minimized.

Show comment Hide comment
@grantovich

grantovich Mar 5, 2013

Thanks for the clarification -- for some reason we were under the impression that f(&:b) was being assigned 10 points. Still, I'm not sure why it even deserves 5 points... this is quite high relative to other constructs. Consider:

['1', '2', '3', '4'].map{ |n| n.to_i } => 2.3 points
['1', '2', '3', '4'].map(&:to_i)       => 8.2 points

Given that the second is little more than an alternate syntax of the first, why the large disparity in points?

Thanks for the clarification -- for some reason we were under the impression that f(&:b) was being assigned 10 points. Still, I'm not sure why it even deserves 5 points... this is quite high relative to other constructs. Consider:

['1', '2', '3', '4'].map{ |n| n.to_i } => 2.3 points
['1', '2', '3', '4'].map(&:to_i)       => 8.2 points

Given that the second is little more than an alternate syntax of the first, why the large disparity in points?

@zenspider

This comment has been minimized.

Show comment Hide comment
@zenspider

zenspider Apr 8, 2013

Owner

The numbers are only useful relative to each other. You shouldn't be worrying about the actual score. Only the score for a single project relative to itself over time is important.

Owner

zenspider commented Apr 8, 2013

The numbers are only useful relative to each other. You shouldn't be worrying about the actual score. Only the score for a single project relative to itself over time is important.

@zenspider zenspider closed this Apr 8, 2013

@grantovich

This comment has been minimized.

Show comment Hide comment
@grantovich

grantovich Apr 8, 2013

I'm not sure how that resolves this issue... consider the two styles in my above example, and a hypothetical project that replaced several instances of the first style (map{ |n| n.to_i }) with instances of the second style (map(&:to_i)). The score of that single project, relative to itself over time, would dramatically increase. Our point is that this is not a desirable outcome, because the complexity of the code has not dramatically increased.

I'm not sure how that resolves this issue... consider the two styles in my above example, and a hypothetical project that replaced several instances of the first style (map{ |n| n.to_i }) with instances of the second style (map(&:to_i)). The score of that single project, relative to itself over time, would dramatically increase. Our point is that this is not a desirable outcome, because the complexity of the code has not dramatically increased.

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