3
\$\begingroup\$

I have the following code that takes an array, and groups it by the sums of its elements:

class Array
  def in_sums_of size, &block
    block = -> el { el } unless block
    container = []

    each_with_index.inject([]) do |tmp, group|
      element, idx = group
      value = block.call(element)

      sum = tmp.inject(0) { |sum,x| sum + block.call(x) }

      if sum + value <= size
        tmp << element
      else
        container << tmp if tmp.any?
        tmp = [element]
      end
      container << tmp if idx == length - 1

      tmp
    end

    container
  end
end

[].in_sums_of(0)
# => []

[1].in_sums_of(1)
# => [[1]]

[1,1].in_sums_of(1)
# => [[1], [1]]

[1,1].in_sums_of(2)
# => [[1, 1]]

[3,2,1].in_sums_of(3)
# => [[3], [2, 1]]

[3,3].in_sums_of(3)
# => [[3], [3]]

[4,3,1].in_sums_of(3)
# => [[4], [3], [1]]

[{v: 1},{v: 2},{v: 2},{v: 3}].in_sums_of(3) do |el|
  el[:v]
end
# => [[{:v=>1}, {:v=>2}], [{:v=>2}], [{:v=>3}]]

It takes an array and a limit, and puts elements into sub arrays, as long as there is space left, which is limited by size var (3 in the example above).

The code works, and is already in production, but I was curious if this is the best approach. I haven't found a Rails helper or Ruby method that would help with this approach. So any ideas on this?

EDIT I've updated the code and added some examples.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you please add several more examples of inputs and outputs of your methods, including some edge cases. I'm confident it's possible to refactor (since initializing variables and temp variables can almost always be avoided Ruby), but I'm not confident I understand what the method is supposed to do. Also, what is the result supposed to be if no block is included? \$\endgroup\$ – Dan Kohn Jul 4 '17 at 14:34
  • \$\begingroup\$ I've updated the code. Do you still see some edge cases that I've missed? \$\endgroup\$ – 23tux Jul 5 '17 at 9:14
4
\$\begingroup\$

You can use chunk, generating a new "chunk id" each time your accumulating sum overflows its max. chunk abstracts away all the details of the inner arrays, so you don't have to worry about when to create a new one, or manually push items onto them. This greatly simplifies the code:

class Array
  def in_sums_of(size, &block)
    block ||= -> x { x }
    sum, id  = 0, 0

    chunk do |x|
      sum += block.call(x)
      sum = block.call(x) and id += 1 if sum > size
      id
    end.map {|x| x.last}

  end
end

As an aside, I'd prefer to create this as a utility method, rather than monkey-patching the Array class, but that change doesn't affect the code much.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ That is brilliant! Exactly "the ruby way" I was looking for. Thanks! \$\endgroup\$ – 23tux Jul 6 '17 at 6:38
  • 1
    \$\begingroup\$ You may be able to use chunk_while (Ruby >=2.3 I believe) which would eliminate the final map. \$\endgroup\$ – Mark Thomas Jul 7 '17 at 19:08
0
\$\begingroup\$

I would consider simplifying this:

block = -> el { el } unless block
container = []
each_with_index.inject([]) do |tmp, group|
  element, idx = group
  value = block.call(element)
  sum = tmp.inject(0) { |sum,x| sum + block.call(x) }

to:

container = []
each_with_index.inject([]) do |tmp, (element, index)|
  value = block_given? ? yield(element) : element
  sum = tmp.sum(&block)

Its arguable about what is better but I would write this as:

class Array
  def in_sums_of size

    sum = 0
    each_with_object([]) do |element, container|
      value = block_given? ? yield(element) : element
      if container.empty? || sum + value > size
        container << [ element ]
        sum = value
      else
        container.last << element
        sum += value
      end
    end

  end
end
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.