current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite
1

I'm doing an archived MOOC at the moment which introduces Ruby programming. There are some assingments, yet no solutions and since the MOOC is over no one will correct those (+ the forum is down)

I'm totally new to Ruby (background in Java) and asking for some hints on my solution to the following task (my solution under the task):

Define a method sum which takes an array of integers as an argument and returns the sum of its elements. For an empty array it should return zero.

def sum(array)
  array[0] = array.pop + array[0];
  sum(array) until array.length == 1
  array.first
end

Define a method max_2_sum which takes an array of integers as an argument and returns the sum of its two largest elements. For an empty array it should return zero. For an array with just one element, it should return that element.

def max_2_sum(a)
  case a.length
    when 0
      return 0
    when 1
      return a.first
    else
      return (a.sort!.pop)+(a.sort!.pop)
  end
end

I think this is pretty verbose, yet I do not know how to solve that better

Define a method sum_to_n? which takes an array of integers and an additional integer, n, as arguments and returns true if any two distinct elements in the array of integers sum to n. An empty array or single element array should both return false.

def sum_to_n?(array, val)
  cartesian = array.product(array).select! { |c| c[0]+c[1] == val }
  not cartesian.empty?
end

I like this :)

share|improve this question
    
Do you have any particular concerns with your code? It's a bit unclear what it is that you're asking here. –  RubberDuck 20 hours ago
1  
I'm not sure if this is "the ruby way" - never worked with a dynamic language before & not so expirenced with programming at all –  ruby000 20 hours ago
    
Better do not call methods right after !, because ! methods can return nil. –  Nakilon 10 hours ago

3 Answers 3

up vote 8 down vote accepted

Ruby can be an extremely concise language, where your programs are almost already written for you:

def sum(array)
    array.inject(0, :+)
end

Where the docs say:

inject { |memo, obj| block } → obj

Combines all elements of enum by applying a binary operation, specified by a block or a symbol that names a method or operator.

In fact reading the docs can go a long way in shortening your programs.

Reading the docs for Array tells us that sorting is built-in and last (the twin of first) are too, also, we just wrote a handy sum function.

def max_2_sum(array)
    sum(array.sort.last(2))
end

This is slower than needed, that is O log n instead of O n but it may be optimized iff it becomes a performance bottleneck (improbable).

Your last function is indeed nice, but the assignment was not necessary and you did not use the handy built-in any?

def sum_to_n?(array, val)
    array.product(array).any? {|couple| sum(couple) == val}
end
share|improve this answer
1  
Thanks, this is really helpful. Ruby has a lot of sophisticated methods, thanks for pointig me there. Yet your max_2_sum does not return "0" if the array is empty :) –  ruby000 19 hours ago
1  
You should use 0 as the initial memo for inject: [].inject(0, :+) # => 0 whereas [].inject(:+) # => nil. –  toro2k 17 hours ago
    
@toro2k done :) –  Caridorc 14 hours ago
1  
Also note that you shouldn't use the ! versions of functions when you're doing an inline operation with them -- they only return the new value if it was changed, in most cases. –  QPaysTaxes 13 hours ago
up vote 1 down vote

Define a method sum which takes an array of integers as an argument and returns the sum of its elements. For an empty array it should return zero.

The accepted answer's array.inject(:+) returns nil, not 0, when given an empty array. A simple change fixes it:

def sum(array)
  array.inject(0, :+)
end

Define a method max_2_sum which takes an array of integers as an argument and returns the sum of its two largest elements. For an empty array it should return zero. For an array with just one element, it should return that element.

Given the fix to the sum method above the accepted answer should work.

def max_2_sum(array)
  sum(array.sort.last(2))
end

Define a method sum_to_n? which takes an array of integers and an additional integer, n, as arguments and returns true if any two distinct elements in the array of integers sum to n. An empty array or single element array should both return false.

By 'distinct elements' do you mean elements with different values, or different indices? For example, in the array [1, 1, 2], can 1 be paired with 1?

share|improve this answer
up vote 1 down vote

Instead of providing solutions, I'll review the code by stating what not to do.

Your sum function, while technically correct, has three shortcomings:

  • It's inefficient, in that it manipulates the array more than necessary.
  • By calling Array#pop, it mutilates the input array in the process. While the problem statement doesn't explicitly forbid you from modifying the array, it is good practice to refrain from doing so.

  • It uses recursion. What's more, it does so in a weird way that also involves an until loop, and it also discards the result of the recursive call. If you did want to write it recursively (I'm not saying it's a good idea), it should look like

    def sum(array)
  array.empty? ? 0 : array.last + sum(array[0, array.size - 1])
end

Your max_2_sum also mutilates the input array in two ways: sorting it in place and removing elements from it.

The (a.sort!.pop) + (a.sort!.pop) line is a bit deceptive, in that the first a and the second a refer to the same array, but in different states. There is no sense in sorting the same array twice — you can just sort it once and take the last two elements.

Your sum_to_n? solution is incorrect, in that it fails to take into account the word distinct in the problem statement. A proper solution would probably involve Array#combination.

Also, sum_to_n?([3], 6) crashes with NoMethodError: undefined method `empty?' for nil:NilClass, due to the way Array#select! works:

If changes were made, it will return self, otherwise it returns nil.

As a rule of thumb, methods whose name ends with ! tend to be a bit "weird", and you should probably read the docs thoroughly. In many cases, especially if you're a beginner, the version without ! is preferable.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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