# Prime multiplication table

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?

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

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'

# 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.
# to make it easier to understand the code.
class CalculatePrime

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
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.

• When finding multiple prime numbers, would be more efficient to use en.wikipedia.org/wiki/Sieve_of_Eratosthenes instead of computing primality of each number individually. Even if you do want to compute primality individually, you only need to check to sqrt(n) not n-1.
– user1149
Mar 9, 2016 at 3:20
• @BarryCarter Ah, yea. Math.sqrt does make sense. But it must be something more than that miss. How would you structure and do this challenge? Mar 9, 2016 at 7:22
• why do you think using a constant is a bad thing? Also the fact that they didn't respond almost certainly isn't a dig at your code, they probably had a large number of applicants or maybe they are already moving forward with someone. Don't take it personally. Mar 9, 2016 at 9:57
• @maxpleaner got answer now and they say this: - 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. Mar 9, 2016 at 11:23

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|
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.

• 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 } }
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 |
+----+----+----+----+----+-----+