# Implementing sort/sort!

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
alias_method :_sort, :sort

def initialize(points)
@points = points
end

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

def sort
Polygon.new _sort
end

def sort!
@points = _sort
end
end

# Example usage

poly = Polygon.new [Point.new(3,4), Point.new(1,2)]
poly.sort
poly.sort!

On Wed, Jul 3, 2013 at 4:39 AM, Kyle H. [email protected]
wrote:

the points inside of the current polygon.
@x = x
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

I very much appreciate the thorough review and subsequent suggestions
you’ve provided on the subject of my code.

Many thanks to you, Robert. It makes a lot of sense.

Am 03.07.2013 09:30, schrieb Robert K.:

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

Hi Robert,

I’m probably missing something obvious, but why not simply use

def meth
dup.meth!
end

?

On Fri, Jul 5, 2013 at 8:22 PM, [email protected] wrote:

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

?

Duh. No, you’re not missing anything here. The only defense I have for
the other form I have is that it is independent of the return value of
#meth! - if you think of String#gsub! for example then your short
version
will not work because sometimes that method will return nil. That’s
probably the reason I made my form a habit.

Kind regards

robert

Am 05.07.2013 23:21, schrieb Robert K.:

Duh. No, you’re not missing anything here. The only defense I have for
the other form I have is that it is independent of the return value of
#meth! - if you think of String#gsub! for example then your short
version will not work because sometimes that method will return nil.
That’s probably the reason I made my form a habit.

A very good point. The `self` in the bang method is probably not
too unlikely to be forgotten.

Thanks for sharing!

On Sat, Jul 6, 2013 at 12:26 AM, [email protected] wrote:

too unlikely to be forgotten.

Or it is used to signal something (like with String#gsub!). For those
pairs where the non bang method returns a copy and the bang method
changes
the current object you depend on the copy being returned from the non
bang
method as the copy is only known inside the method.

Thanks for sharing!

You’re welcome!

Kind regards

robert