Prime multiplication table

Question

I am new to Ruby (1 year as hobby project on weekends) and solved a code challenge and it was not as good as they wanted. I want to know how i could make it better and how to make the structure better. In summary, how would a mid or senior ruby developer do this challenge?

This is the task:

Write a program that prints a multiplication table of primes numbers. The program should take an argument from the command line that specifies the amount of prime numbers to generate and print out a multiplication table for these prime numbers.

An example of the way the application may run:

executable_script_name --­­count 10

An example of the output (using the terminal­table gem):

| 2 3 5 7 11 13 17 19 23 29

­­­+­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­­

2 | 4 6 10 14 22 26 34 38 46 58

3 | 6 9 15 21 33 39 51 57 69 87

5 | 10 15 25 35 55 65 85 95 115 145

7 | 14 21 35 49 77 91 119 133 161 203

11 | 22 33 55 77 121 143 187 209 253 319

13 | 26 39 65 91 143 169 221 247 299 377

17 | 34 51 85 119 187 221 289 323 391 493

19 | 38 57 95 133 209 247 323 361 437 551

23 | 46 69 115 161 253 299 391 437 529 667

29 | 58 87 145 203 319 377 493 551 667 841

Notes

1. Consider code readability/complexity
2. Consider SOLID principles, but do not over­engineer
3. Consider extensibility
4. Feel free to use any library or gem in both implementation and tests, but please write your own code for the prime number generator.
5. Consider how you can prove the correctness of your application
6. Write it in Ruby

Below starts my code challenge:

require 'rubygems'
require 'bundler/setup'

require 'optparse'
require './calculate_prime'

# Just parsing the options here and
# initialize the Calculater to present
# the table of results.

options = {}
OptionParser.new do |opts|
  opts.banner = 'Usage: prime_multiply.rb [options]'

  opts.on('-v', '--count N', Integer, 'Select amount of primes') do |v|
    options[:count] = v
  end
end.parse!

primes = CalculatePrime.new(options[:count])
puts primes.present

This code above is just for run terminal, check arguments and and call the actual program and return the table.

require 'terminal-table'
require 'optparse'
require './array_addons'

# This program multiplies all primes and
# add them to a multidimensional array and
# present them as a table in the terminal

# This class handle all the logic in the program.
# It use a addition to the Array class in array_addons
# to make it easier to understand the code.
class CalculatePrime
  attr_reader :primes, :counted

  FIXNUM_MAX = (2**(0.size * 8 - 2) - 1)

  # initialize the class and create number of primes picked
  def initialize(count)
    @primes = get_prime_numbers(count)
    multiply
  end

  # gets amount of primes from Prime class
  def get_prime_numbers(size)
    primes = []
    return [] if size < 1
    (2..FIXNUM_MAX).each do |num|
      # want to break it when got all asked primes so it will
      # not go for infinite
      break if primes.size >= size
      # check if num is odd by dividenum and add it
      primes.push(num) if (2..num - 1).all? { |dividenum| num % dividenum > 0 }
    end
    primes
  end

  # multiply the primes with it's values with
  # the multiply_by method in my own array addition.
  def multiply
    @counted = []

    @primes.each do |n|
      # using unshift to add a extra of n to the beginning
      # of the array to show it nice in the table later
      @counted.push(@primes.multiply_by(n).unshift(n))
    end
  end

  # return the data that was multiplied in a table
  def present
    # unshifting a empty string to make a empty column
    # between header and childcolumns
    table = Terminal::Table.new headings: @primes.unshift(''), rows: @counted
    table
  end
end

If you wonder why i am not using Ruby's Prime class it is because they wanted me to make the prime method myself in Ruby.

And then it is just the added method to array:

# This class just add a collect method for multiply_by
# to make it easier to understand the code.
class Array
  def multiply_by(x)
    collect { |n| n * x }
  end
end

I think the biggest no-no in my code is the FIXNUM_MAX constant. But how could I have done it in a better way?

And PS. They didn't even let me explain my reasons why i did some of the code and not even a thank you for the challenge so I feel i did a really bad challenge. I also have rspec tests and can add them if you wonder that could be the reason why they not even let me discuss it.

My Gemfile is:

source 'http://rubygems.org'

# terminal stuff
gem 'terminal-table'

# testing
gem 'rspec'

# code quality checks
gem 'rubocop'

EDIT: Got answer now after some time:

• TDD and the Red/Green/Refactor cycle
• Seperating the concerns of CalculatePrime class as it had way to many responsibilities because it was responsible for calculating the prime numbers, multiplying the numbers and printing the table each one of these could have made into their own classes with one responsibility, I would recommend reading 'Practical Object Oriented design in Ruby' by Sandi Metz as this has some good examples of this.

I guess they was very picky and strict. TDD is mostly because of lack of experience and the other thing well, is also a little bit of lack of experience but also I felt I didn't want too small classes either. But they didn't let me explain that. These stuff are also easy to learn.

Answer 1

If you wrote this code as a hobbyist, I'm impressed. You seem to be adept at using Ruby idioms for looping, such as low..high).each {}, Enumerable#all? {}, and Enumerable#collect. Your use of action if condition is also appropriate.

On the other hand, I don't think that your object-oriented design of CalculatePrime is good. Problems include:

• Classes should be named as nouns. PrimeCalculator would be more appropriate than CalculatePrime, but even that isn't quite the right description. PrimeMultiplicationTable, maybe?
• What state is kept in the object? #multiply caches its own result, but #get_prime_numbers does.

• What methods is the caller supposed to call? It's not obvious that the usage is supposed to be

  primes = CalculatePrime.new(…)
  puts primes.present
  

  It would help if you marked #get_prime_numbers and #multiply as private. But then…

• When a class has two functions, one of which is a constructor, and the other one takes no parameters, chances are that you would be better of with just a function, and no class at all.

Primes

Prime numbers constitute an infinite list. An Enumerator would be a good way to represent that. With this enumerator, you can do puts primes.next or puts primes.first(10) — that's better than having primes itself take a size parameter.

def primes
  Enumerator.new do |y|
    n = 2
    loop do
      y.yield n if (2...n).all? { |divisor| n % divisor != 0 }
      n += 1
    end
  end
end

Your return [] if size < 1 special case is superfluous. Instead of (2..num - 1), use (2...num) to exclude the upper bound.

You're right that using FIXNUM_MAX to do unbounded counting is awkward. I've opted to give up on iterating over a range, and used a manual loop instead.

Our prime generators are not optimized at all. I wouldn't worry about performance, since you only need to gather enough prime numbers for a multiplication table — how big could your display possibly be?

Multiplication table

Consider code reuse: don't limit your multiplication table generator to primes. For that matter, you shouldn't restrict it to multiplication, either.

I would avoid patching the Array class with #multiply_by. Patching is dubious software engineering practice: in a large project, such patches can conflict with each other. This usage has too little benefit to justify the risk.

Numbers should be right-justified, by convention.

def arithmetic_table(numbers, &binary_op)
  Terminal::Table.new do |t|
    t.headings = [nil] + numbers
    numbers.each do |r|
      t.add_row([r] + numbers.collect { |c| binary_op.call(r, c) })
    end
    t.style = {:alignment => :right}
  end
end

Driver

You don't need to specify a banner; the default provided by OptionParser is pretty good.

It's weird that -v is the short form of --count.

I would package the option-parsing code in a function that caches its own result.

def options
  @options ||= {}
  OptionParser.new do |opts|
    opts.on('-c', '--count N', Integer, 'Select amount of primes') do |n|
      @options[:count] = n
    end
  end.parse!
  @options
end

puts arithmetic_table(primes.first(options[:count] || 0)) { |a, b| a * b }

Note how the very last line assembles together all the building blocks: "Print a multiplication table of the first count primes." Contrast that with your original

primes = CalculatePrime.new(options[:count])
puts primes.present

… which doesn't tell you much about what the program does, because all the functionality is hard-coded and buried inside CalculatePrime.

Answer 2

Some notes on your code:

• OptionParser is a good choice because it's in the stdlib, but it's pretty cumbersome to use. I'd recommend some other library (i.e: trollop)
• There are hundreds of answers in SO and CR about primes generation, so I am not going to add more noise to it, others users can help you there. For this, I'll use the prime library.
• def multiply_by(x). For such a simply operation, you just write the map where you need it, this does not add any useful abstraction IMO.
• This kind of problems ask for functional solutions. No each, push, shift and so on.
• table = some_expr and then table. That's not necessary, just return some_expr.

I'd write:

require 'prime'
require 'trollop'
require 'terminal-table'

module PrettyPrimes
  def self.multiplication_table(size)
    primes = Prime.first(size)
    header_rows = [[nil] + primes, :separator]
    product_rows = primes.map { |p1| [p1] + primes.map { |p2| p1 * p2 } }
    Terminal::Table.new(rows: header_rows + product_rows)
  end
end

if __FILE__ == $0
  opts = Trollop::options do
    opt(:count, "Select amount of primes", type: :integer, default: 10) 
  end
  $stdout.puts(PrettyPrimes.multiplication_table(opts[:count]))
end

Output:

$ ruby primes_multiplication_table.rb --count=5
+----+----+----+----+----+-----+
|    | 2  | 3  | 5  | 7  | 11  |
+----+----+----+----+----+-----+
| 2  | 4  | 6  | 10 | 14 | 22  |
| 3  | 6  | 9  | 15 | 21 | 33  |
| 5  | 10 | 15 | 25 | 35 | 55  |
| 7  | 14 | 21 | 35 | 49 | 77  |
| 11 | 22 | 33 | 55 | 77 | 121 |
+----+----+----+----+----+-----+