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.
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
Now as pdr points out, this is not the optimal algorithm to do this. Instead of iterating between all integers between
count and printing those which are divisible by
multiples_of, you can just iterate between the numbers between
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|
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|
def print_multiples(multiples_of, count)
multiples(multiples_of, count) do |i|
You can also define multiples as:
def multiples(multiples_of, count, &blk)
(multiples_of .. count).step(multiples_of, &blk)
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
select etc. on it.