5
\$\begingroup\$

Task: Implement a function, which takes an array and returns a new array with every second element removed.

My implementation:

def remove_every_other(list)
  i = 0
  results = []

  list.each do |item|
    results << item if i.even?
    i += 1
  end

  results
end
# Provided function-calls / examples
puts remove_every_other([1, 2, 3, 4, 5]).inspect # [1, 3, 5]
puts remove_every_other([5, 1, 2, 4, 1]).inspect # [5, 2, 1]
puts remove_every_other([1]).inspect # [1]

The provided examples return the expected results. So I guess it is formally correct.

But: Is there are more Ruby-idiomatic way solving the described task?

\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

This is really good! Especially the fact that you're testing it.

But yes, there is a way to make it more idiomatic. For starters, we don't have to maintain the index separately. We can use Enumable#each_with_index

def remove_every_other(list)
  results = []

  list.each_with_index do |item, i|
    results << item if i.even?
  end

  results
end

Second, we don't need to explicitly return the results array if we use Enumerable#each_with_object.

def remove_every_other(list)
  list.each_with_index
      .each_with_object([]) do |(item, i), results|
    results << item if i.even?
  end
end

But even this is a bit long winded. We just need to filter out entries where the

def remove_every_other(list)
  list.each_with_index
      .filter do |item, i|
    i.even?
  end.map(&:first)
end

But if we use the suggested Enumerable#each_slice this becomes even more terse because we don't need to map out the index. There's no conditional here.

def remove_every_other(list)
  list.each_slice(2).map(&:first)
end

Naming

You're using Ruby arrays. This makes the generic list name a bit unidiomatic. More idiomatic would be something that reflects that they are arrays like arr.

The word "remove" in the method name seems less applicable that "select".

Testing

When you call puts value.inspect, this can simply be p value.

And you're repeating yourself. Better to have an array of test arrays and iterate over them.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ In fact, both the OP's original method as well as your refactored one work with all finite Enumerables, not just Arrays, so the list name is doubly confusing: the specification says "array" and the implementation says "any enumerable", but the parameter name says "list". \$\endgroup\$ Commented 1 hour ago

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.