0
\$\begingroup\$

As a beginning Ruby programmer I implemented a solution for Project Euler, Problem #11. (The problem is described in the comment below.) I'll be grateful for any suggestions for improvement (especially regarding code readability).

# Largest product in a grid
# Problem 11
#
# In the 20×20 grid below, four numbers along a diagonal line have been [marked].
#
# 08 02 22 97 38 15 00 40 00 75 04 05 07 78 52 12 50 77 91 08
# 49 49 99 40 17 81 18 57 60 87 17 40 98 43 69 48 04 56 62 00
# 81 49 31 73 55 79 14 29 93 71 40 67 53 88 30 03 49 13 36 65
# 52 70 95 23 04 60 11 42 69 24 68 56 01 32 56 71 37 02 36 91
# 22 31 16 71 51 67 63 89 41 92 36 54 22 40 40 28 66 33 13 80
# 24 47 32 60 99 03 45 02 44 75 33 53 78 36 84 20 35 17 12 50
# 32 98 81 28 64 23 67 10[26]38 40 67 59 54 70 66 18 38 64 70
# 67 26 20 68 02 62 12 20 95[63]94 39 63 08 40 91 66 49 94 21
# 24 55 58 05 66 73 99 26 97 17[78]78 96 83 14 88 34 89 63 72
# 21 36 23 09 75 00 76 44 20 45 35[14]00 61 33 97 34 31 33 95
# 78 17 53 28 22 75 31 67 15 94 03 80 04 62 16 14 09 53 56 92
# 16 39 05 42 96 35 31 47 55 58 88 24 00 17 54 24 36 29 85 57
# 86 56 00 48 35 71 89 07 05 44 44 37 44 60 21 58 51 54 17 58
# 19 80 81 68 05 94 47 69 28 73 92 13 86 52 17 77 04 89 55 40
# 04 52 08 83 97 35 99 16 07 97 57 32 16 26 26 79 33 27 98 66
# 88 36 68 87 57 62 20 72 03 46 33 67 46 55 12 32 63 93 53 69
# 04 42 16 73 38 25 39 11 24 94 72 18 08 46 29 32 40 62 76 36
# 20 69 36 41 72 30 23 88 34 62 99 69 82 67 59 85 74 04 36 16
# 20 73 35 29 78 31 90 01 74 31 49 71 48 86 81 16 23 57 05 54
# 01 70 54 71 83 51 54 69 16 92 33 48 61 43 52 01 89 19 67 48
#
# The product of these numbers is 26 × 63 × 78 × 14 = 1788696. What is the greatest
# product of four adjacent numbers in the same direction (up, down, left, right, or 
# diagonally) in the 20×20 grid?
#

str = <<-EOS
08 02 22 97 38 15 00 40 00 75 04 05 07 78 52 12 50 77 91 08
49 49 99 40 17 81 18 57 60 87 17 40 98 43 69 48 04 56 62 00
81 49 31 73 55 79 14 29 93 71 40 67 53 88 30 03 49 13 36 65
52 70 95 23 04 60 11 42 69 24 68 56 01 32 56 71 37 02 36 91
22 31 16 71 51 67 63 89 41 92 36 54 22 40 40 28 66 33 13 80
24 47 32 60 99 03 45 02 44 75 33 53 78 36 84 20 35 17 12 50
32 98 81 28 64 23 67 10 26 38 40 67 59 54 70 66 18 38 64 70
67 26 20 68 02 62 12 20 95 63 94 39 63 08 40 91 66 49 94 21
24 55 58 05 66 73 99 26 97 17 78 78 96 83 14 88 34 89 63 72
21 36 23 09 75 00 76 44 20 45 35 14 00 61 33 97 34 31 33 95
78 17 53 28 22 75 31 67 15 94 03 80 04 62 16 14 09 53 56 92
16 39 05 42 96 35 31 47 55 58 88 24 00 17 54 24 36 29 85 57
86 56 00 48 35 71 89 07 05 44 44 37 44 60 21 58 51 54 17 58
19 80 81 68 05 94 47 69 28 73 92 13 86 52 17 77 04 89 55 40
04 52 08 83 97 35 99 16 07 97 57 32 16 26 26 79 33 27 98 66
88 36 68 87 57 62 20 72 03 46 33 67 46 55 12 32 63 93 53 69
04 42 16 73 38 25 39 11 24 94 72 18 08 46 29 32 40 62 76 36
20 69 36 41 72 30 23 88 34 62 99 69 82 67 59 85 74 04 36 16
20 73 35 29 78 31 90 01 74 31 49 71 48 86 81 16 23 57 05 54
01 70 54 71 83 51 54 69 16 92 33 48 61 43 52 01 89 19 67 48
EOS

class Grid

  class << self
    def parse str
      Grid.new str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
    end
  end

  def initialize matrix
    @matrix = matrix
    @dim = @matrix.size
  end

  def horizontals
    @matrix
  end

  def verticals
    @matrix.transpose
  end

  def diagonals
    (0..@dim-1).map { |i| diag(0, i) }.reverse + (1..@dim-1).map { |i| diag(i, 0) }
  end

  def inv_diagonals
    reverse.diagonals
  end

  private

  def diag x, y
    result = []
    while x < @dim && y < @dim
      result << @matrix[x][y]
      x += 1
      y += 1
    end
    result
  end

  def reverse
    Grid.new(@matrix.map { |row| row.reverse })
  end
end

def max_product lines
  lines.flat_map { |line| line.each_cons(4).to_a }.map { |cons| cons.inject { |prd, x| prd * x} }.max
end

grid = Grid.parse str
p [:horizontals, :verticals, :diagonals, :inv_diagonals].map { |method| max_product(grid.send(method)) }.max
\$\endgroup\$
3
  • \$\begingroup\$ @CarySwoveland, I'm not sure what to think about your comment, as it seems sarcastic, but anyway ... I really liked your solution, particularly the use of the cool array functions like max_by and reduce, which really make the code much cleaner and which I didn't know before. I gave you an upvote right away because to my understanding that is the way to express gratitude here on SE. The upvotes are anonymous, though, so I'd like to thank you once more for the time you've spent answering my question and for the new stuff that you've taught me. :) \$\endgroup\$ Dec 12, 2014 at 9:44
  • \$\begingroup\$ Dušan, it appears that someone with a master key deleted my comment. I have no idea why. (Whoever did that, please step out of the shadow and defend your action). My comment was not intended to be sarcastic. I complimented you on your code and tried to explain why I gave what admittedly is an unacceptable answer. You must take that at face value. I apologize for not fixing my answer yet, but I will do so this evening. \$\endgroup\$ Dec 13, 2014 at 0:41
  • \$\begingroup\$ After looking at your code more closely I see that my earlier unqualified praise of it was unwarranted, but it still contains flashes of inspiration, especially considering that you are a self-declared newbie. I hope you find my comments helpful. \$\endgroup\$ Dec 13, 2014 at 6:26

1 Answer 1

3
\$\begingroup\$

Your Code

Class method

You created the class method parse with this snippet:

class << self
  def parse str
    Grid.new str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
  end
end

That works, but a more conventional definition is just:

def self.parse str
  Grid.new str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
end

Further, since self equals Grid when this method is created, it is better to write:

def self.parse str
  new str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
end

so that this code needn't be changed if you rename the class. (Note that new str.split... is the same as self.new str.split....)

However, you don't need a class method to create the instance. Instead, just write:

arr = str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
grid = Grid.new(arr)

Consolidate all calculations in the class

You want to move:

def max_product lines
  lines.flat_map { |line| line.each_cons(4).to_a }.map { |cons| cons.inject {
   |prd, x| prd * x} }.max
end

and

[:horizontals, :verticals, :diagonals, :inv_diagonals].map { |method|
   max_product(grid.send(method)) }.max

into the class definition and make the latter a method:

def max_product_all_ways
  [:horizontals, :verticals, :diagonals, :inv_diagonals].map { |method|
    max_product(grid.send(method)) }.max
end

but I think it would be clearer to write:

def max_product_all_ways
  [max_product(horizontals),
   max_product(verticals),
   max_product(diagonals),
   max_product(inv_diagonals)].max
end

You would then invoke max_product_all_ways as follows:

arr = str.split("\n").map { |row| row.split(" ").map { |val| val.to_i } }
grid = Grid.new(arr)
grid.max_product_wall_ways

Diagonal detail

It's a small thing, but in diagonals you don't need to compute the diagonals of length less than four if only arrays of size four are to be considered. In fact, you should not, as it is conceivable that a diagonal of length less than four could have the greatest product!

Revised class definition

So this is what we now have (more or less):

class Grid

  def initialize arr
    @matrix = arr
    @dim = @matrix.size
  end

  def max_product_all_ways
    [max_product(horizontals),
     max_product(verticals),
     max_product(diagonals),
     # max_product(inv_diagonals)
     ].max
  end

  private

  def max_product lines
    lines.flat_map { |line| line.each_cons(4).to_a }.
          map { |cons| cons.inject { |prd, x| prd * x} }.max
  end

  def horizontals
    @matrix
  end

  def verticals
    @matrix.transpose
  end

  def diagonals
    (0..@dim-1).map { |i| diag(0, i) }.reverse +
    (1..@dim-1).map { |i| diag(i, 0) }
  end

  def inv_diagonals
    rev.diagonals
  end

  def diag x, y
    result = []
    while x < @dim && y < @dim
      result << @matrix[x][y]
      x += 1
      y += 1
    end
    result
  end

  def rev
    @matrix.map { |row| row.reverse }
  end
end

Well, this is still not quite right. The calculation of inv_diagonals is not correct, but it needs a fair bit of work to fix. I've therefore commented it out in max_product_all_ways. I also changed the name of the method reverse because it's easily confused with the built-in methods for reverse for strings and arrays.

I'll leave it to you to fix the rest and of course others may offer additional suggestions.

Alternative Solution

Here is one way you could write the program more compactly.

Code

class Grid

  def max_product(a, n)
    [array_max(a, n),
     array_max(a.transpose, n),
     array_max(diagonals(a, n), n),
     array_max(diagonals(rotate(a), n), n)
    ].max
  end

  def array_max(a,n)
    row_max(a.max_by { |r| row_max(r,n) }, n)
  end

  def row_max(r,n)
    r.each_cons(n).max_by { |e| e.reduce(:*) }.reduce(:*)
  end

  def diagonals(a, n)
    (0..a.size-n).each_with_object([]) do |i,b|
      b << (0...a.size-i).map { |j| a[i+j][j] }
      b << (0...a.size-i).map { |j| a[j][i+j] } if i > 0
    end
  end

  def rotate(a)
    a.transpose.reverse
  end
end

Examples

Given 20x20 array

a = str.lines.map { |l| l.split.map(&:to_i) }

g.max_product(a,4) #=>    51,267,216
g.max_product(a,3) #=>       776,776
g.max_product(a,5) #=> 3,143,734,308

3x3 array

a = [[1,2,3],
     [4,5,6],
     [7,8,9]]
g.max_product(a,1) #=>   9
g.max_product(a,2) #=>  72
g.max_product(a,3) #=> 504

a = [[1,2,3],
     [4,5,8],
     [7,6,9]]
g.max_product(a,2) #=> 72

a = [[1,7,3],
     [4,8,6],
     [5,2,9]]
g.max_product(a,2) #=> 72

a = [[1,2,3],
     [4,8,6],
     [9,5,7]]
g.max_product(a,2) #=> 72

In the last example, you can see the effects of rotate and diagonals:

b = rotate(a)
  #=> [[3, 6, 7],
  #    [2, 8, 5],
  #    [1, 4, 9]]

c = diagonals(b, 2)
  #=> [[3, 8, 9], [2, 4], [6, 5]]

array_max(c, 2)
  #=> [72, 8, 30].max => 72

Incidentally, I made the number of elements whose product is to be maximized a variable because it wasn't really any more difficult to do that and it facilitated debugging.

\$\endgroup\$
2
  • \$\begingroup\$ @CarySwoveland, thank you for the great review! :) One question - I introduced the static parse method because I needed to be able to instantiate Grid objects from both a string and a two-dimensional array representation (especially for the inv_diagonals method). I thought that it was a reasonable solution, given that Ruby lacks support for overloaded methods. What do you think about this approach? Thanks \$\endgroup\$ Dec 13, 2014 at 11:01
  • \$\begingroup\$ Yes, it's better to convert the string to an array outside the class, as I did the alternative approach. I did an edit. You don't need a new Grid instance for inv_diagonals; just create an array that contains the contents of @matrix rearranged as desired. \$\endgroup\$ Dec 13, 2014 at 19:15

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.