Multiset implementation in Ruby

Question

I am trying to implement the Wikipedia definition of a MultiSet and need feedback on the implementation.

class MultiSet
 include Enumerable
 attr_reader :members
 def initialize enum={}
 @members = {}
 raise_error unless enum.class.include? Enumerable
 enum.each do |item|
 if @members.include?(item) 
 @members[item] += 1
 else
 @members[item] = 1
 end 
 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 to_h
 members.dup
 end 
 def to_set
 Set.new @members.keys
 end 
 def remove item
 if @members.include?(item) 
 @members[item] = @members[item] - 1
 if @members[item] < 1
 @members.delete(item)
 end
 end 
 self
 end 
 def add item
 if @members.include?(item) 
 @members[item] += 1 
 else
 @members[item] = 1
 end 
 self
 end 
 def empty! 
 @members.clear 
 end 
 def multiplicity item 
 @members[item] == 0 ? nil : @members[item]
 end
 def include? item
 @members.include? item 
 end
 def cardinality
 return 0 if @members.empty?
 @members.values.reduce(:+)
 end 
 def | other
 other.each do |k,v|
 if members.include? k
 members[k] = multiplicity(k) + other.multiplicity(k)
 else
 add k
 end 
 end
 self
 end 
 def & other
 members.each do |k,v|
 if other.include? k
 members[k] = [multiplicity(k), other.multiplicity(k)].min
 else
 remove k
 end 
 end 
 self
 end 
end

Answer 1

• Sometimes you're skipping parentheses, sometimes you're not. Please be consistent

• Sometimes you're accessing @members directly, sometimes you're using the members reader method. In some cases that might make sense to do one or the other, but here it's random. Again: Be consistent.
  However, be careful about providing a reader. Right now, I could say a_multiset.members[some_item] = -100 or something, and things would get weird because I've messed with an internal data structure. A #members accessor might be better as simply an alias for #to_h

• What is raise_error? I don't see it defined anywhere. Which, funnily enough, means that it does raise an error - an error telling you that raise_error isn't defined.
  If you're going to raise an error, then raise the appropriate one:

  raise ArgumentError, "enum must include the 'Enumerable' module"
  

• enum.class.include?(Enumerable) is a roundabout way of writing enum.kind_of?(Enumerable)

• Your initializer duplicates logic from you #add method; just call #add instead.

• #remove can be cleaned up a little by postfixing the if members[item] < 1, just to get rid of the pyramid indentation.

• Your #empty! method should probably be called #clear. That's the conventional name used by Hash, Array and Set (as you can tell, since that's what your method calls on the @members hash)

• Pretty sure that your #multiplicity method is exactly backwards. You're returning nil if an item's count is zero. I think you want the exact opposite: Return zero if the item doesn't exist:

  include?(item) ? members[item] : 0
  

• Use your own methods when you can. You have an #include? method already, so there's no need to use @members.include? all over the place.

• #cardinality can be written as just

  members.values.reduce(0, :+)
  

  If you provide an initial value to #reduce (the zero), you don't need the extra emptiness-check you have now

• Pretty sure your union method is incorrect too. If a key doesn't exist, you simply add it - meaning it'll have a count of 1. But in fact, it should have the same count as it has in the other set.
  And you can simplify the method, too:

  def |(other)
   other.each do |key, count|
   members[key] = multiplicity(key) + count
   end
   self
  end
  

• Both the union and intersection methods should probably return a new MultiSet instance, rather than modify the receiver.

• aarti: 1. How would the #members accessor be made alias to to_h, if I made members private
• Flambino: @gitarchie I don't know what you're asking there. I'm just saying that a members accessor should return a dup of @members, like to_h does (in other words, it should be an alias) - it should not return the instance variable itself.
• aarti: Ok, I fixed all the issues you mentioned and submitted a new version, I realize my union and intersection were wrong so I changed them and leveraging the add to actually create the multiset.
• Flambino: @gitarchie Please don't update your question with new code - but feel free to post a brand new question, if you want! Please see this meta post: "What you can and cannot do after receiving answers". I'll take the liberty of rolling back your edit
• Flambino: @gitarchie I know what you mean about versioning, but questions must stay in lock-step with their answers - otherwise the Q&A format falls apart. However, if you simply include a link to/from a new question, it's works out just fine. You can also title a follow-up question with "follow-up", "take 2", "updated" or similar to make the distinction clear. Your new code certainly looks better, by the way. Can't really review it fully here in the comments, of course, but again feel free to post a new question, if you'd like (also just to get other opinions than mine)