8
\$\begingroup\$

I learned to program in Java and C# and in my free time I am using Ruby because it is fun. Unfortunately I have the feeling that I write Ruby code like I am making Java or C# code. I have learned to use regex instead of strings for comparison, using each instead of for-loops, keeping method names lowercase, and how to use code blocks (which are quite similar to lambda expressions in C#).

I would love to improve the Rubiness of my code and I hope you would be willing to give me one or more pointers on a piece of code I made. It answers Project Euler problem 27.

class Integer
  def prime?
    return false if self < 1
    2.upto(Math.sqrt(self)) do |i|
      return false if self % i == 0
    end
    true
  end
end

def get_amount_of_primes_from_quadratic_formula(a,b)
  primes = []
  still_all_primes = true
  n = 0
  while still_all_primes
    result = n**2 + a*n + b
    if result.prime? then
      primes << result
    else
      still_all_primes = false
    end
    n += 1
  end
  primes.size
end

def get_product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values()
  max_product = 0
  max_primes = 0
  -999.upto(1000) do |a|
    -999.upto(1000) do |b|
      primes = get_amount_of_primes_from_quadratic_formula(a,b)
      if primes > max_primes then
        max_primes = primes
        max_product = a*b
      end
    end
  end
  max_product
end

start = Time.now
answer = get_product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values()

puts "The answer is #{answer} and it took #{Time.now-start} seconds."

I think I can improve on the if-then statement and write it more concise, and also the two "upto-loops" where I first declare the variables max_primes and max_product can be written in a more Ruby-way I am sure.

I would be very grateful if you could let me know how to write more like Ruby!

Links that ask similar questions which I am reading in the meantime:

\$\endgroup\$
6
\$\begingroup\$

Succinctness
As rubyists, we love being succinct, and we love playing with enumerations.

You will see very few literal false and true in ruby code, as well as very few explicit return calls.

For example:

Instead of writing return false if self < 1 we will prefer to compound the condition to self >= 1 && ... which will do the same thing, but we "save" return false.

The power of Enumeration
Ruby has a very powerful Enumerable, and is used widely, often more than once in a line (using method chaining).

For example:

2.upto(Math.sqrt(self)) do |i|
  return false if self % i == 0
end

Here you check if any of the numbers in the range are a divisor for self, and break if there is any. A more ruby way of doing it will be:

return false if 2.upto(Math.sqrt(self)).any? { |i| self % i == 0 }

We'll also prefer to more succinct range syntax (2..Math.sqrt(self)), which is simply shorter...

So now, our def prime? method could be reduced to a one-liner:

class Integer
  def prime?
    self > 1 && !(2..Math.sqrt(self)).any? { |i| self % i == 0 }
  end
end

Mapping
Anywhere in the code I see the following pattern:

result = []
some_loop do
  result << something
end

A red flag is raised, and I look for a way to use map to do the same thing:

result = some_loop.map { something }

Your code goes over all the non-negative integers, and takes counts how many of them result in a prime, until the first non-prime.

"All the non-negative integers" can be expressed in ruby as (0..Float::INFINITY), so we can write:

(0..Float::INFINITY).map { |n| n**2 + a*n + b }.take_while { |result| result.prime? }.count

This code takes each integer, maps it into the result of n**2 + a*n + b, takes all the results until they are no longer prime, and counts how many are there.

Cool! Right? The only problem with the code above, is that it will take infinity to complete it, as it takes all the numbers and maps them, and then checks for how many to take.

To solve this problem ruby now has...

Lazy Enumerables
As of ruby 2.0, lazy enumerables allows you to calculate values in an infinite stream only as needed.

To solve the problem above, all we need to do now is to add the lazy operator on the range:

(0..Float::INFINITY).lazy.map { |n| n**2 + a*n + b }.take_while(&:prime?).count

And we have another one-liner!

Everything is an enumerable
So you want to save on your "upto-loops"? Let's do it!

You want to enumerate over each pair of numbers from -999 to 1000, so what you actually want is to have a long matrix of those pairs:

[[-999, -999], [-999, -998],...,[1000, 1000]].do_something_smart

To do that, you can use product:

(-999..1000).to_a.product((-999..1000).to_a)

But since both a and b have the same range, we can even DRY this up, and use repeated_permutation:

(-999..1000).to_a.repeated_permutation(2)

Both of these solutions will give you the needed matrix, so we can move on the see what we should do with it...

We want to get the coeffiecients that produce the number of primes, so let's do just that:

a, b = (-999..1000).to_a.repeated_permutation(2).max_by { |a, b| get_amount_of_primes_from_quadratic_formula(a,b) }

Now all we need to do is multiply them with each other!

Method naming
Your names are very verbose, which is a good thing, but ruby idiom frowns upon get_ prefixes. Also, prefer using verbs already in the language (count) over those which are not in the language (amount_of)

So now the code will look like:

class Integer
  def prime?
    self > 1 && !(2..Math.sqrt(self)).any? { |i| self % i == 0 }
  end
end

def count_quadratic_formula_primes(a,b)
  (0..Float::INFINITY).lazy.map { |n| n**2 + a*n + b }.take_while(&:prime?).count
end

def product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values()
  a, b = (-999..1000).to_a.repeated_permutation(2).max_by { |a, b| count_quadratic_formula_primes(a,b) }
  a * b
end

start = Time.now
answer = product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values

puts "The answer is #{answer} and it took #{Time.now-start} seconds."

15 lines of hard-core ruby-style code!

Enjoy!


Update
It seems that lazy adds considerable overhead to the performance of the code. So it is not advisable to use it.

Fortunately this works:

(0..Float::INFINITY).take_while { |n| (n**2 + a*n + b).prime? }.count

My code still runs ~2 times slower than the original (ends in 18 seconds), but it is more reasonable than with lazy...

\$\endgroup\$
  • 1
    \$\begingroup\$ also, you can inject your a*b :) \$\endgroup\$ – gaussblurinc Apr 20 '14 at 7:37
  • \$\begingroup\$ I like your code better, but it takes 130 seconds, while the original code takes 10. The prime function is a beauty. I suspect that the count_quadratic_formula_primes method is slow. \$\endgroup\$ – Erwin Rooijakkers Apr 20 '14 at 14:58
  • \$\begingroup\$ @user2609980 - yes, apparently lazy add a lot of overhead... see my update \$\endgroup\$ – Uri Agassi Apr 20 '14 at 18:08
  • 3
    \$\begingroup\$ user2609980, I'm surprised at the difference in execution times, but I'd pay no heed to that at this stage of your Ruby education. Uri has covered a wide swath of ground in his answer, acquainting you with typical Ruby coding style, the addition of a method to an existing Ruby class (prime?), the use of powerful enumerators map, product, permutation, any? and take_while from the Enumerable module, and up_to from the Integer class, and Ruby's new lazy operator. He has also nicely explained his reasons for coding it the way he has. Great answer, Uri. \$\endgroup\$ – Cary Swoveland Apr 20 '14 at 18:24
  • 1
    \$\begingroup\$ Lovely stuff. Can I suggest instead of something like !(1..10).any? {...} that the OP use (1..10).none? {...}. \$\endgroup\$ – David Aldridge Apr 11 at 7:04
0
\$\begingroup\$

Rubocop Report

There are a couple of additional offenses against Ruby style guide and conventions that haven't been addressed in the excellent answer.

  • Use # frozen_string_literal: true top level comment (why?).
  • Insert an empty line after guard clauses: return false if self < 1.
  • Prefer the zero predicate over 0 check: self % i == 0 -> (self % i).zero?.
  • Try to keep the number of lines of each method below 10. get_amount_of_primes_from_quadratic_formula has 13 lines.
  • Parameter names should be communicative. a, b should be refactored.
  • Do not use then for multi-line if. if result.prime? then -> remove then.
  • Omit parentheses when the method or def does not define arguments.
  • Keep line lengths below 80 characters.
\$\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.