Fraction class with documentation

Question

I have never worked with Ruby before, and thought I'd start working on learning it. For my first little implementation, I have created a simple fraction class. It would be nice if someone would check what I did wrong or could improve upon. I am coming from a JavaScript background which is classless for the most part which is why I might have made some mistakes. Also please rate my documentation, I'm not too familiar with yard just yet.

Here's my code so far:

class Fraction

    @top
    @bot


    # top is the x in (x/n)
    # bot is the n in (x/n)
    def initialize top, bot
        @top = top
        @bot = bot
    end

    # Add two fractions to eachother
    #
    # ==== Example
    #  Fraction.new(1, 5) + Fraction.new(2, 3)
    #
    #
    # @param [Fraction] the other fraction
    #
    # @return [Fraction] the sum of the fractions
    #
    # * other must be a fraction
    # * other must be valid as a fraction
    def +(other)
        lcm = Fraction.lcm(@bot, other.bot);
        min = (@bot<other.bot) ? @bot : other.bot

        Fraction.new(@top * other.bot + other.top * @bot, min * lcm).simplified;
    end

    # Access @top
    # @return [Number]
    def top
        @top
    end

    # Access @bot
    # @return [Number]
    def bot
        @bot
    end

    # factors to a number
    #
    #
    # ==== Example
    #
    #  Fraction.factors 12 # => [1, 2, 3, 4, 6]
    #
    # @param [Number] the number to get the factors from
    #
    # Will return a list of factors including 1
    def self.factors number
        return (1..number).to_a.select do |i|
            number % i == 0
        end
    end


    # GCD (Greatest Common Divisor)
    #
    # ==== Example
    #
    #  Fraction.gcd 24, 36 # => 12
    #
    # this works because both 24 and 36 are divisible by 12,
    # but no higher number
    #
    # @param [Number, Number] two numbers to get the GCD from
    # @return [Number] GCD
    #

    def self.gcd a, b
        ((factors a) & (factors b)).pop
    end

    # LCM (Least Common Multiple)
    # ==== Example
    #
    #  Fraction.lcm 3, 5 # => 15
    #  Fraction.lcm 5, 5 # => 5
    #  Fraction.
    #
    # @param [Number, Number] two numbers to get the LCM from
    # @return [Number] LCM
    #

    def self.lcm a, b
        min = (a>b)?a:b

        while true do
            if min % a == 0 && min % b == 0 then
                return min
            end

            min += 1;
        end
    end


    # Simplified
    #
    # ==== Example
    #
    #  Fraction.new(2, 4).simplified # => (1 / 2):Fraction
    #
    # @return [Fraction] a simplified fraction instance
    #
    def simplified
        g = Fraction.gcd @top, @bot

        Fraction.new @top/g, @bot/g
    end

    # Simplified?
    #
    # ==== Example
    #
    #  Fraction.new(2, 4).simplified?            # => False
    #  Fraction.new(1, 2).simplified?            # => True
    #  Fraction.new(2, 4).simplified.simplified? # => True
    #
    # @return [bool] wether simplified or not
    def simplified?
        (Fraction.gcd @top, @bot) <= 1
    end

    # to_s (To String)
    #
    # ==== Example
    #
    #  Fraction.new(2, 4).to_s # => (1 / 2)
    #
    # ==== Format
    #  (#{@top} / #{@bot})
    #
    # @return [String] A formatted string of the *simplified* fraction
    #
    def to_s
        if simplified? then
            sprintf "(#{@top} / #{@bot})"
        else
            simplified.to_s
        end
    end

end




#
# Specs
#
describe Fraction, '+factors' do
    it "should be a list" do
        expect(Fraction.factors(6)).to eq([1, 2, 3, 6])
    end
end

describe Fraction, '+initialize' do
    it "should initialize" do
        expect(Fraction.new(1, 5))
    end
end

describe Fraction, '-to_s' do
    it "should patternize" do
        expect(Fraction.new(1, 5).to_s).to eq("(1 / 5)");
        expect(Fraction.new(6, 5).to_s).to eq("(6 / 5)");
    end
end

describe Fraction, "+lcm" do
    it "should get lcm" do
        expect(Fraction.lcm(5, 3)).to eq(15);
        expect(Fraction.lcm(5, 5)).to eq(5);
    end
end

describe Fraction, '#+' do
    it "should add them together" do
        expect((Fraction.new(1, 3) + Fraction.new(2, 3)).to_s).to eq(Fraction.new(1, 1).to_s)
    end
end

Answer 1

Mathematical simplification

The GCD can be calculated with the Euclidean algorithm that is both simpler and faster than your method:

function gcd(a, b)
    a, b = b, a % b until b == 0
    return a

This also does not need the additional factors function.

---

lcm can be defined in terms of GCD:

def self.lcm a, b
    (a * b).abs / gcd(a, b)
end

Built-in

Both gcd and lcm are built-in see: https://ruby-doc.org/core-2.2.0/Integer.html#method-i-gcd and https://ruby-doc.org/core-2.2.0/Integer.html#method-i-lcm . Maybe you re-implemented them on purpose as part of the exercise but it is good to know how extensive the Ruby Standard library is.

Incomplete

The code misses the other operations on fractions (*/-) and some useful helpers like negate (x -> -x) and invert (x -> 1/x), to become truly useful.

Good

Apart from these Math / Completeness matters, the code is very clear (arguably also because it is very simple) and the documentation is comprehensive so good job overall.

Answer 2

1. Unnecessary stuff: @top, @bot after the declaration of the class.. well, basically you can remove them, what should they do? :O
2. Clarity: @top, @bot - this is mainly matter of taste but... @bot: it's not immediate to understand what it is about (yup, I know it's written in the docs but... couldn't you just have called it bottom?) - or the "real names" numerator / denominator ?
3. Test cases: honestly... unless you do something more than just assigning values, I'd skip the unit test on the initialization.. otherwise, strictly speaking, you'd have to test also top & bot methods, wouldn't you? Makes no sense, doesn't it? ;)
4. Accessors: in ruby we have a nice construct not to have to write all boilerplate code like you did.

We have the equivalence between

def top
   @top
end

def bot
   @bot
end

and

 attr_reader :top, :bot

Have a look at attr_writer & attr_accessor as well :)