Re: Implementing sort/sort!

On Wed, Jul 3, 2013 at 4:39 AM, Kyle Hunter <[e-mail]> wrote:

> Hello,
>
> I was wondering about implementing sort/sort!. Basically, I have a Point
> class, which contains x, y variables of a 2d Cartesian point. It also
> includes the Comparable mixin and thus implements a <=> method.
>
> I also have a Polygon class which is really just a container for an
> array of the aforementioned Point class. I guess the name is a misnomer
> but I'm working on adding functionality. Anyway, I want Polygon.sort to
> return a new Polygon with the points sorted, and Polygon.sort! to modify
> the points inside of the current polygon.
>
> I've attempted it, and it seems to work, but I was wondering if anyone
> could recommend a more canonical or idiomatic way to do it, or point out
> any flaws with my approach.
>
> class Point
>   include Comparable
>   attr_accessor :x, :y
>
>   def initialize(x, y)
>     @x = x
>     @y = y
>   end
>
>   def <=>(other)
>     x_cmp = @x <=> other.x
>     x_cmp != 0 ? x_cmp : @y <=> other.y
>   end
> end
>
> class Polygon
>   include Enumerable
>   attr_accessor :points
>

Usually a bad idea because it allows someone to mess with the internal
state of your class Polygon.  You can even update @points to point to a
single Fixnum which will break a lot of logic of your class.  You violate
encapsulation here.


>   alias_method :_sort, :sort
>

  def initialize(points)
>     @points = points
>   end
>

You could add a check which ensures points does actually contain what it is
supposed to contain.  Example

def initialize(points)
  @points = points.each {|x| raise "Type error" unless Point === x}
end

Note: this will also flag an error if points does not have Each - an
additional safety against instances of wrong types passed in.

  def each(&blk)
>     @points.each(&blk)
>   end
>

Conventionally #each returns self. That is missing here.


>   def sort
>     Polygon.new _sort
>   end
>

Usually with pairs of bang and non bang methods one can do

def meth!
  # logic
  self
end

def meth
  dup.tap {|copy| copy.meth!}
end


>   def sort!
>     @points = _sort
>   end
>

Wrong return value.  Should be self.


> end
>
> # Example usage
> poly = Polygon.new [Point.new(3,4), Point.new(1,2)]
> poly.sort
> poly.sort!
>

I think there is one design issue here: you include Enumerable and hence
Enumerable#sort and #sort_by.  Those methods always return an Array.  You
are returning Polygon here which is a change in interface.  Code like this
will break when handed a Polygon:

def sorted_size(x)
  case x
  when Enumerable
    x.sort.size
  when ...
  else
   ...
  end
end

Changing an interface in this way is usually a bad thing to do.  If you
want to include Enumerable but also provide a means to sort Points you
could provide a separate method to sort Points, e.g.

def sort_points
  @points.sort!
  self
end

Kind regards

robert

-- 
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/