One important note regarding naming in ruby:
The syntax in ruby is that constants (including class and module names) have to start with capital letters and local variables have to start with lower case letters. Instance, class and global variables must start with their respective sigil followed by a letter in any case. Method names must start with a letter in any case, but if they start with a capital letters, they must be called with ()
when there are no arguments.
The naming convention in ruby is to use snake_case for method names and variables, PascalCase for class and module names and ALL_CAPS for constants other than class and module names.
For method names the convention is to use snake_case
for all methods except "constructor"-style functions (e.g. Integer()
, Array()
, Hash()
) where the name of the function is the same as that of the constructed class.
Generally using PascalCase for method names is a bad idea because:
- It's against convention and will make people think it's a constructor-style function
- If you define a method without arguments, you'll trip yourself up because you'll try to call it as
MyMethod
instead of MyMethod()
which does not work if the method name starts with a capital letter.
General notes on your code:
Using a while loop is usually unidiomatic ruby. Using a while loop to iterate from one number to another number with a constant step-size is always unidiomatic ruby. If the step-size is 1, you can just use each
on a range and if it is something else you can use step
. So your code would just be written as:
def display_multples(multiples_of, count)
(multiples_of .. count).each do |i|
if i % multiples_of == 0
puts i
end
end
end
Now as pdr points out, this is not the optimal algorithm to do this. Instead of iterating between all integers between multiples_of
and count
and printing those which are divisible by multiples_of
, you can just iterate between the numbers between multiples_of
and count
with a step size of multiples_of
, i.e. only iterate over the multiples of multiples_of
. However his code contains an unnecessary each
(which will also make the code not work on ruby 1.8.6, though that's thankfully becoming less of an issue these days). So the code should just be:
def display_multiples(multiples_of, count)
(multiplesOf..count).step(multiples_of) do |i|
puts i
end
end
However this code still contains one major, non-ruby-specific design-smell: You're mixing the logic to generate the multiples with the code printing it. Generally IO and logic should be separated as much as possible. So what you should do is define a method multiples
which generates the multiples and a method print_multiples
which prints them.
In ruby the best way to write a method which generates a sequence of values is to write an "iterator method", i.e. a method which yields the elements. That would look like this:
def multiples(multiples_of, count)
(multiples_of .. count).step(multiples_of) do |i|
yield i
end
end
def print_multiples(multiples_of, count)
multiples(multiples_of, count) do |i|
puts i
end
end
You can also define multiples as:
def multiples(multiples_of, count, &blk)
(multiples_of .. count).step(multiples_of, &blk)
end
In 1.8.7+ this also has the benefit of returning an Enumerator if no block is given. so if you don't just want to print it, you can also use Enumerable methods like map
, select
etc. on it.