Multiset implementation in Ruby -Take 2

Question

This is a follow up to question. I fixed the issues mentioned in the answer as far as my understanding. The union and intersection now return new object instances with new implementation. I also wrote a test, which is below.

class MultiSet
  include Enumerable

  def initialize enum=[]
    @members = {}
    raise ArgumentError, "enum must include the 'Enumerable' module" unless enum.kind_of? Enumerable
    enum.each do |item|
      add item
    end
  end

  def each &blk
    @members.each &blk
  end

  def == other
    @members.to_h == other.to_h
  end

  def eql? other
    self == other
  end

  def members
    @members.dup
  end  

  def to_a
    a = []
    @members.each do |key, count|
      count.times { a << key }
    end  
    a
  end  

  def to_set
    Set.new members.keys
  end

  def remove item
    if include? item
      @members[item] = @members[item] - 1
      @members.delete item if @members[item] < 1
    end
    self
  end

  def add item
    if include? item
      @members[item] += 1
    else
      @members[item] = 1
    end
    self
  end

  def clear
    @members.clear
  end

  def multiplicity item
    include?(item) ? @members[item] : 0
  end

  def include? item
    @members.include? item
  end

  def cardinality
    @members.values.reduce(0,:+)
  end

  def | other
    union = self.class.new(self.to_a)
    other.each do |key, count|
      count.times { union.add key }
    end
    union
  end

  def & other
    intersection = self.class.new
    @members.each do |key, count|
      if other.include? key
        max_count = [count, other.multiplicity(key)].min
        max_count.times { intersection.add key }
      end  
    end
    intersection
  end

  alias_method :to_h, :members

end

The test

require 'minitest/autorun'
require_relative 'multiset'
require 'set'

class MultiSetTest < MiniTest::Unit::TestCase

  def test_equal
    assert_equal MultiSet.new([1, 3]), MultiSet.new([3, 1])
  end

  def test_no_duplicates
    assert_equal MultiSet.new([1, 1]).to_set, Set.new([1])
  end  

  def test_remove
    assert_equal MultiSet.new([1,3]), MultiSet.new([3,2,1]).remove(2)
    assert_equal MultiSet.new([1,2,3]), MultiSet.new([3,2,1]).remove(4)
    assert_equal MultiSet.new([1,2,3]), MultiSet.new([3,2,1]).remove(2.0)
    assert_equal MultiSet.new([1,3]), MultiSet.new([3,2.0,1]).remove(2.0)
  end

  def test_empty
    assert_equal MultiSet.new, MultiSet.new([1,2]).clear
    assert_equal MultiSet.new, MultiSet.new.clear
  end

  def test_multiplicity
    assert_equal 2,MultiSet.new([1, 1, 2]).multiplicity(1)
    assert_equal 0,MultiSet.new([1, 1, 2]).multiplicity(4)
    assert_equal 0,MultiSet.new([1, 1, 2]).multiplicity(1.0)
    assert_equal 1,MultiSet.new([1, 1, 2]).multiplicity(2)
  end

  def test_membership
    assert MultiSet.new([1,2,3]).include?(2)
    assert MultiSet.new(1..3).include?(2)
    refute MultiSet.new(1..3).include?(2.0)
    refute MultiSet.new(1..3).include?(4)
  end

  def test_add
    assert_equal MultiSet.new([1,2,3,4]),
      MultiSet.new([1,2,4]).add(3)

    expected = {1 => 1, 2 =>1, 3 => 2}
    assert_equal expected, MultiSet.new([1,2,3]).add(3).to_h

    assert_equal MultiSet.new([1,2,3,3.0]),
      MultiSet.new([1,2,3]).add(3.0)
  end

  def test_to_set
    assert_equal Set.new, MultiSet.new.to_set
    assert_equal Set.new([1,2,3]), MultiSet.new([3,1,2]).to_set
  end

  def test_cardinality
    assert_equal 3,MultiSet.new([1, 1, 2]).cardinality
    assert_equal 0,MultiSet.new.cardinality
  end

  def test_union
    assert_equal MultiSet.new([3.0,3,2,1]), MultiSet.new([1,3]) | (MultiSet.new([2,3.0]))
    assert_equal MultiSet.new([3,1]), MultiSet.new([1,3]) | (MultiSet.new)
    assert_equal MultiSet.new([2]), MultiSet.new([2]) | (MultiSet.new)
    assert_equal MultiSet.new([]), MultiSet.new | (MultiSet.new)
  end

  def test_intersection
    assert_equal MultiSet.new([1,1]), MultiSet.new([1,1,1,3]) & (MultiSet.new([1,1,2]))
    assert_equal MultiSet.new([3,1]), MultiSet.new([1,3]) | (MultiSet.new)
    assert_equal MultiSet.new([2]), MultiSet.new([2]) | (MultiSet.new)
    assert_equal MultiSet.new([]), MultiSet.new | (MultiSet.new)
  end


end

Answer 1

Note: This is written using modern Ruby. I'm well aware that some things I used here were not available, and some things you used no longer are.

Let's kick off with some personal preference things. These are all totally your choice to put in, but I prefer to do them for various reasons, and my later tips assume you followed them.

1. Wrap method arguments in parentheses, both in method invocations and method definitions. Note that this doesn't apply to operators.
2. Put a space after your commas, and on either side of every binary operator.

Now, on to the meatier advice:

In multiset.rb:

1. Define @members as a Hash.new(0) instead of just {}. That way, you can remove all of your nil checks. For example,

   def add(item)
     if include? item
       @members[item] += 1
     else
       @members[item] = 1
     end
     self
   end
   

   can become just

   def add(item)
     @members[item] += 1
     self
   end
   

   There are a bunch of other places that this affects; see the final code for them all.

2. Instead of defining a method as a wrapper around another -- i.e. your eql?:

   def eql?(other)
     self == other
   end
   

   use alias_method:

   alias_method :eql?, :==
   

   It makes the intention much clearer -- you want to make eql? and == equivalent.

3. I'd recommend making two new aliases: union to | and intersection to &, since the meanings aren't immediately clear from the names.

Your tests all look pretty thorough, and I don't really have anything to say about them that I didn't say in the first section. However, I do tend to miss edge cases, so please give it another look if you're concerned about that.

Put together, this is your final code:

class MultiSet
  include Enumerable

  def initialize(enum=[])
    @members = Hash.new(0)
    raise ArgumentError, "enum must include the 'Enumerable' module" unless enum.kind_of?(Enumerable)
    enum.each do |item|
      add(item)
    end
  end

  def each(&blk)
    @members.each(&blk)
  end

  def ==(other)
    @members.to_h == other.to_h
  end

  def members
    @members.dup
  end

  def to_a
    a = []
    @members.each do |key, count|
      count.times { a << key }
    end
    a
  end

  def to_set
    Set.new(members.keys)
  end

  def remove(item)
    @members[item] -= 1
    @members.remove(item) if @members[item] < 1
    self
  end

  def add(item)
    @members[item] += 1
    self
  end

  def clear
    @members.clear
  end

  def multiplicity(item)
    @members[item]
  end

  def include?(item)
    @members.include?(item)
  end

  def cardinality
    @members.values.reduce(0, :+)
  end

  def |(other)
    union = self.class.new(self.to_a)
    other.each do |key, count|
      count.times { union.add(key) }
    end
    union
  end

  def &(other)
    intersection = self.class.new
    @members.each do |key, count|
      if other.include?(key)
        max_count = [count, other.multiplicity(key)].min
        max_count.times { intersection.add(key) }
      end
    end
    intersection
  end

  alias_method :to_h, :members
  alias_method :union, :|
  alias_method :intersection, :&
  alias_method :eql?, :==
end