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

Flambino · Accepted Answer · 2014-10-19 17:15:51Z

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.

1. How would the #members accessor be made alias to to_h, if I made members private — aarti
– aarti, Commented Oct 19, 2014 at 18:28
@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. — Flambino
– Flambino, Commented Oct 19, 2014 at 18:34
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. — aarti
– aarti, Commented Oct 19, 2014 at 19:15
@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
– Flambino, Commented Oct 19, 2014 at 19:18
@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) — Flambino
– Flambino, Commented Oct 19, 2014 at 19:32

Stack Exchange Network

Multiset implementation in Ruby

1 Answer 1

You must log in to answer this question.

Linked

Hot Network Questions

Multiset implementation in Ruby

1 Answer 1

You must log in to answer this question.

Linked

Related

Hot Network Questions