11
\$\begingroup\$

Here's a method I made that's supposed to print out multiples of any numbers up to any quantity:

def DisplayMultiples(multiplesOf, count)
  i = multiplesOf
  while i <= count
    if i % multiplesOf == 0
      puts i
    end
    i += 1
  end
end

Any suggestions on how to improve the code to something more fitting to the Ruby style? I'm coming from a C# background so I'd like to switch things up a bit.

\$\endgroup\$
32
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Good, thorough, answer. Supercedes mine which I will delete, so no need to reference my answer here \$\endgroup\$ – pdr Feb 16 '11 at 16:31
  • 1
    \$\begingroup\$ By the way: literally everything you write, except for the specific naming conventions, has really nothing specifically to do with Ruby, it's just general good OO, or even just general good programming. Pretty much everything would apply 100% identically to C# as well, for example. \$\endgroup\$ – Jörg W Mittag Feb 17 '11 at 3:56
7
\$\begingroup\$

This is not actually an answer to your question, but since you mentioned that you come from C# to Ruby, I just wanted to point out that the idiomatic Ruby solution that @sepp2k presented is pretty much indentical to what an idiomatic C# solution would have looked like:

class Multiples : IEnumerable<int>
{
    private readonly int _multiplesOf;
    private readonly int _limit;

    public static Multiples Of(int multiplesOf = 1, int limit = int.MaxValue)
    {
        return new Multiples(multiplesOf, limit);
    }

    Multiples(int multiplesOf = 1, int limit = int.MaxValue)
    {
        _multiplesOf = multiplesOf;
        _limit = limit;
    }

    public IEnumerator<int> GetEnumerator()
    {
        var i = 0;
        while ((i += _multiplesOf) <= _limit) yield return i;
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

It might not be immediately obvious that those two examples are the same, but they mostly are. The main difference is that .NET's libraries are much weaker than Ruby's, so we had to implement some stuff manually that was provided for us by the Ruby libraries.

You would use both versions pretty much the same way:

// C#
foreach (var n in Multiples.Of(7, 50)) Console.WriteLine(n);
Multiples.Of(7, 50).ToList().ForEach(Console.WriteLine);
Console.WriteLine(Multiples.Of(7, 50).ToArray());

# Ruby
multiples(7, 50) {|n| puts n }
multiples(7, 50, &method(:puts))
puts *multiples(7, 50)

In fact, the C# version I presented here is slightly more powerful, since it allows the client to choose the termination condition more freely. For example, what if I don't want the multiples of 7 below 50, but rather the first 50 multiples? Easy:

Multiples.Of(7).Take(50)

Implementing this for the Ruby version is left as an exercise to the reader :-)

\$\endgroup\$
  • 2
    \$\begingroup\$ I'd say that to be similar to my ruby version the C# code should use Enumerable.Range, not an iterative while-loop. Also I don't get your last point. You can do multiples(7, Float::INFINITY).take(n) in the ruby version as well. \$\endgroup\$ – sepp2k Feb 17 '11 at 18:12
5
\$\begingroup\$

Inline way: p (7..49).step(7).to_a

Invocable way:

def multiples(of, through)
  (of..through).step of
end
p multiples(7, 49).to_a

#=> [7, 14, 21, 28, 35, 42, 49]

The biggest difference from your code is due to Ruby's library. Smalltalk was a major influence on Ruby.

\$\endgroup\$
2
\$\begingroup\$

As Ruby, like Perl, emphasizes that there is more than one way to do something, let me suggest my own. This is close to what I'd do in a functional programming language:

def multiples(n, count)
    1.upto(count).map {|i| n * i}
end

multiples(23, 5)
=> [23, 46, 69, 92, 115]

It could also be added (via monkeypatching) to the Fixnum class, if you're willing to engage in that sort of thing. That would allow you to invoke it like this:

23.multiples(5)
=> [23, 46, 69, 92, 115]

This will do the same thing as the step methods provided in other answers. I find it a bit more readable (though I wasn't familiar with the step function until today, so take that as you will).

Edit

While we're at it, let's look at something neat you can do with Ruby 2.0. With the new Enumerable.lazy class, you can easily express this as an infinite sequence, from which you can take small subsets:

def multiples(n)
    (1..Float::INFINITY).lazy.map {|x| x * n}
end

# Now we can get an infinite list, and defer operating on it until later
multiples(23).take(5).to_a
=> [23, 46, 69, 92, 115]
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy