From: Robert Klemme Date: 2013-07-03T16:30:33+09:00 Subject: Re: Implementing sort/sort! --047d7b6da782a17fb504e0966fa1 Content-Type: text/plain; charset=ISO-8859-1 On Wed, Jul 3, 2013 at 4:39 AM, Kyle Hunter 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/ --047d7b6da782a17fb504e0966fa1 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable



On Wed, Jul 3, 2013 at 4:39 AM, Kyle Hunter <<= a href=3D"mailto:lists@ruby-forum.com" target=3D"_blank">lists@ruby-forum.c= om> 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 <=3D> 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<= br> could recommend a more canonical or idiomatic way to do it, or point out any flaws with my approach.

class Point
=A0 include Comparable
=A0 attr_accessor :x, :y

=A0 def initialize(x, y)
=A0 =A0 @x =3D x
=A0 =A0 @y =3D y
=A0 end

=A0 def <=3D>(other)
=A0 =A0 x_cmp =3D @x <=3D> other.x
=A0 =A0 x_cmp !=3D 0 ? x_cmp : @y <=3D> other.y
=A0 end
end

class Polygon
=A0 include Enumerable
=A0 attr_accessor :points

Usually= a bad idea because it allows someone to mess with the internal state of yo= ur class Polygon. =A0You can even update @points to point to a single Fixnu= m which will break a lot of logic of your class. =A0You violate encapsulati= on here.
=A0
=A0 alias_method :_sort, :sort

=A0 def initialize(points)
=A0 =A0 @points =3D points
=A0 end

You could add a check whi= ch ensures points does actually contain what it is supposed to contain. =A0= Example

def initialize(points)
=A0 @points =3D points.each {|x| raise "Type error" unless Point = =3D=3D=3D x}
end
=A0
Note: = this will also flag an error if points does not have Each - an additional s= afety against instances of wrong types passed in.

=A0 def each(&blk)
=A0 =A0 @points.each(&blk)
=A0 end

Conventionally #each retu= rns self. That is missing here.
=A0
=A0 def sort
=A0 =A0 Polygon.new _sort
=A0 end

Usually with pairs of ban= g and non bang methods one can do

def = meth!
=A0 # logic
=A0 self
= end

def meth
=A0 dup.tap {|copy= | copy.meth!}
end
=A0
=A0 def sort!
=A0 =A0 @points =3D _sort
=A0 end

Wrong return value. =A0Sh= ould be self.
=A0
end

# Example usage
poly =3D 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. =A0Those m= ethods always return an Array. =A0You are returning Polygon here which is a= change in interface. =A0Code like this will break when handed a Polygon:

def sorted_= size(x)
=A0 case x
=A0 when Enumerable
=A0 =A0 x.sort.size
=A0 when ...
=A0 else
=A0 =A0...
=A0 end
end
Changing an interface in this way is usually a bad thing to do. =A0If= 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_po= ints
=A0 @points.sort!
=A0 self
end
<= div class=3D"gmail_extra">
Kind regards

robert
<= div class=3D"gmail_extra">
--
remem= ber.guy do |as, often| as.you_can - without end
http://blog.rubybestpractice= s.com/
--047d7b6da782a17fb504e0966fa1--