Change for loops with method each

[woarewe]

Hello) I have changed 2 parts in current code: 1) Replaced for loops with each method I don't know any ruby developer, who uses for, everywhere only each, for this I have two arguments(code lines with 'for' were committed in 2011, I think ruby is modern and cool language) :

• Each looks more readable and cleaner
• For uses each under hood, so from functional side I don't change anything 2) Replaced defined? for respond_to?, respond_to? looks more readable, cleaner, it returns true or false, we are checking can our object respond to method each, we are not comparing defined result with all possible values of defined?('local-variable', 'class'), so I think, using defined? doesn't make any sense and only does code unreadable. Sorry for my English.

[yuki24]

I don't believe this PR dramatically improves the code base because of the following reasons:

• defined? makes total sense because it's checking if methods.each is a legit operation.
• While it's true it's not common, the for loop looks perfectly readable to me and there doesn't seem like a competing issue here.
• Accepting changes like this would open the can of warms for other style changes and we definitely don't want our focus to be on that unless the proposed change clearly brings value, which in this case seems very little.

[ioquatix]

I'm surprised by this result:

defined?(thing.each): "method"
thing.respond_to?(:each): true
Warming up --------------------------------------
            defined?   416.887k i/100ms
         respond_to?   350.220k i/100ms
Calculating -------------------------------------
            defined?     71.080M (± 3.8%) i/s -    354.771M in   4.999402s
         respond_to?     20.175M (± 5.5%) i/s -    100.863M in   5.015579s

Comparison:
            defined?: 71079878.2 i/s
         respond_to?: 20174872.5 i/s - 3.52x  slower

Here is the repro code:

#!/usr/bin/env ruby

require 'benchmark/ips'

class Thing
    def each
        puts "each"
    end
end

thing = Thing.new
puts "defined?(thing.each): #{defined?(thing.each).inspect}"
puts "thing.respond_to?(:each): #{thing.respond_to?(:each).inspect}"

Benchmark.ips do |x|
    x.report("defined?") do |times|
        while times > 0
            defined?(thing.each)

            times -= 1
        end
    end

    x.report("respond_to?") do |times|
        while times > 0
            thing.respond_to?(:each)

            times -= 1
        end
    end

    # Compare the iterations per second of the various reports!
    x.compare!
end

Is there some reason why defined? is so much faster than respond_to??

[ioquatix]

(You can comment out def each to try different variations.)

[ioquatix]

Here is comparison of .each vs for ...:

items = 100.times.to_a

Benchmark.ips do |x|
    x.report("for x in items") do |times|
        while times > 0
            for x in items
                -x
            end

            times -= 1
        end
    end

    x.report("items.each{|x|") do |times|
        while times > 0
            items.each{|x| -x}

            times -= 1
        end
    end

    # Compare the iterations per second of the various reports!
    x.compare!
end

Warming up --------------------------------------
      for x in items    17.722k i/100ms
      items.each{|x|    18.391k i/100ms
Calculating -------------------------------------
      for x in items    184.345k (± 4.6%) i/s -    921.544k in   5.013109s
      items.each{|x|    195.077k (± 3.5%) i/s -    974.723k in   5.003383s

Comparison:
      items.each{|x|:   195077.1 i/s
      for x in items:   184345.2 i/s - same-ish: difference falls within error

It looks like for x in items is a bit slower, and I agree it's not used that much. items.each is more common IMHO.

[ioquatix]

Accepting changes like this would open the can of warms for other style changes and we definitely don't want our focus to be on that unless the proposed change clearly brings value, which in this case seems very little.

I can see both sides of this argument. I think there are some valuable points in this PR, but there are also some parts which are less useful. That being said @woarewe is a first time contributor so I think we can help guide them in the right direction, and they will be able to make a useful contribution. I don't mind to accept stylistic and cosmetic improvements along with tangible improvements to consistency and performance.

[ioquatix]

@woarewe these actually do different things:

require 'forwardable'

class Items
    extend Forwardable
    # I think, it's not very good write so :@object
    def_delegators :@a, :first 

    # Better using access methods like :object, via attr_accessor(or reader)
    def_delegators :b, :last

    def initialize
        @a = [1,2,3]
    end

    def b
        [4, 5, 6]
    end
end

items = Items.new

puts items.first # 1
puts items.last # 6

[ioquatix]

It's okay if the end result is that we don't merge this PR, but look at it as a learning experience for everyone. If people are enthusiastic to contribute, that's a great first step. It definitely seems like there is something worth improving here, but as I've tried to show, I think it should be guided by performance improvements where possible. I don't mind changing syntax if it's more maintainable/clearer what's going on. Removing the each and delegating to an existing method makes a lot of sense, as it makes the code semantically simpler with less moving parts. This is not a performance critical code path so an extra allocation doesn't matter that much, but it's really great to be aware of this and also if semantics of the code are changed somehow that might break existing systems.

[woarewe]

@woarewe these actually do different things:

require 'forwardable'

class Items
  extend Forwardable
  # I think, it's not very good write so :@object
  def_delegators :@a, :first 

  # Better using access methods like :object, via attr_accessor(or reader)
  def_delegators :b, :last

  def initialize
      @a = [1,2,3]
  end

  def b
      [4, 5, 6]
  end
end

items = Items.new

puts items.first # 1
puts items.last # 6

@ioquatix , thank you, I understand the difference between these things(instance variables and methods), I just mean, that access to instance variables should be via method, IMHO, instance variables should be used only for initialization. By the way, in current realization of forwardable calls eval, so if you have attr_accessor or attr_reader for instance variable then there is no difference which style using, def_delegator :@a or def_delegaor :a

[ioquatix]

That seems like low hanging fruit for performance improvement

[woarewe]

That seems like low hanging fruit for performance improvement

@ioquatix, Are you about PR or replacing of eval?

[ioquatix]

Removing eval

[woarewe]

Oh, that's cool, I will fix it with huge pleasure.

[ioquatix]

Sorry I have a cold this ends communication for the day

[woarewe]

Hello I tried to remove 'eval' and found very interesting case I debug test failures and error using gem 'pry-byebug' and found next ruby code in source of gem 'byebug' https://github.com/deivid-rodriguez/byebug/blob/master/lib/byebug/subcommands.rb#L18 Code from link works just because of current version of "forwardable" which build new delegating method via eval. I don't know, what I have to do, because many gems can use this "eval feature"

[woarewe]

I think, I have to close this pull request. Thanks everyone for help and review.