crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.34k stars 1.61k forks source link

each_with_object behaves differently than in Ruby #4135

Closed daveyarwood closed 7 years ago

daveyarwood commented 7 years ago

I noticed this issue when solving the "Raindrops" problem (similar to the classic FizzBuzz) on http://exercism.io.

I had previously solved the same problem in Ruby using each_with_object("") to iterate through the rules and build up a string of the applicable sounds. So, I tried to reuse the same code in Crystal and I found that each_with_object appears to behave differently.

Here is my code:

class Raindrops
  RULES = [
    {3, "Pling"},
    {5, "Plang"},
    {7, "Plong"}
  ]

  def Raindrops.drops(drop)
    sounds = RULES.each_with_object "" do |(n, sound), result|
      drop % n == 0 ? result + sound : result
    end

    sounds.empty? ? drop.to_s : sounds
  end
end

# Raindrops.drops(35) => ""

This code compiles, but it appears that the result is not being accumulated, and the block returns "".

I was able to get it to work using reduce:

class Raindrops
  RULES = [
    {3, "Pling"},
    {5, "Plang"},
    {7, "Plong"}
  ]

  def Raindrops.drops(drop)
    sounds = RULES.reduce "" do |result, (n, sound)|
      drop % n == 0 ? result + sound : result
    end

    sounds.empty? ? drop.to_s : sounds
  end
end

# Raindrops.drops(35) => "PlangPlong"

In Ruby, reduce and each_with_object both work for the example above. Could this be a bug in the implementation of each_with_object in Crystal?

$ crystal -v
Crystal 0.21.1 [3c6c75e] (2017-03-06) LLVM 3.5.0
Sija commented 7 years ago

Crystal behaves here in the same way as Ruby does.

# from Ruby `Enumerable#each_with_index` docs
# works as expected

evens = (1..10).each_with_object([] of Int32) { |i, a| a << i * 2 }
# => [2, 4, 6, 8, 10, 12, 14, 16, 18, 20]

Passed String as an initial value indeed gets returned at the end, because #each_with_index does not accumulate value, it just passes it as a reference. You could rewrite it to use String::Builder, like so:

class Raindrops
  RULES = [
    {3, "Pling"},
    {5, "Plang"},
    {7, "Plong"}
  ]

  def self.drops(drop)
    sounds = RULES.each_with_object(String::Builder.new) do |(n, sound), result|
      result << sound if drop % n == 0
    end
    sounds.empty? ? drop.to_s : sounds.to_s
  end
end

# Raindrops.drops(35) # => "PlangPlong"
refi64 commented 7 years ago

Hey, you're the guy who works on Alda, right?

daveyarwood commented 7 years ago

@kirbyfan64 yep, hello!

@Sija That makes sense. The String::Builder semantics are the piece I was missing. Thanks!