class Position
attr_accessor :x, :y
#example of inputs: "12", "0 2", "1,1"
def parse input
coords = input.scan(/[0-9]/)
raise StandardError, "Invalid input! Please enter 2 coordinates. Example: 1 0" if coords.size != 2
@x = coords[0].to_i
@y = coords[1].to_i
end
end
Refactorings
No refactoring yet !
Adam
January 15, 2009, January 15, 2009 22:03, permalink
class Position
attr_reader :x, :y
def self.parse(digits)
new(*digits.scan(/(\d).*(\d)/).first)
end
def initialize(x, y)
@x, @y = x.to_i, y.to_i
end
def inspect
"<Position x:#{x}, y:#{y}>"
end
end
Martin Carel
January 15, 2009, January 15, 2009 23:29, permalink
I much prefer that. The regex reflects the intention. Plus, the whole thing is definitely more OO.
If I want to keep the error handling (e.g. if user enters "abc"), I'd suggest:
def self.parse(digits) new(*digits.scan(/(\d).*(\d)/).flatten) rescue raise StandardError, "invalid input! Please enter 2 coordinates. Example: 2 0" end
Tj Holowaychuk
January 23, 2009, January 23, 2009 20:39, permalink
My useless 2 cents. I like to have at least one base exception per class so developers can rescue it as needed (if at all)
class Position
class Error < StandardError; end
class ParseError < Error; end
attr_reader :x, :y
def self.parse digits
new *digits.scan(/(\d).*(\d)/).first
rescue
raise ParseError, "Please pass a string containing 2 coordinates. Example: '2 0'", caller
end
def initialize x, y
@x, @y = x.to_i, y.to_i
end
def inspect
"<Position:#{__id__} x:#{x}, y:#{y}>"
end
end
Tj Holowaychuk
January 23, 2009, January 23, 2009 20:54, permalink
Also I should mention (although in this case it does not matter at all really) but generally something [^\d]* would be faster than .*
Martin Carel
January 23, 2009, January 23, 2009 21:07, permalink
@ Tj Holowaychuk: it was not useless. I put StandardError for simplicity sake. See for yourself: http://tinyurl.com/cdq2dm :)
But I will improve with:
- defining my custom Error class inside Position class
- change the error message (like you subtlety did) to be more friendly
Tj Holowaychuk
January 23, 2009, January 23, 2009 21:31, permalink
Sounds good. I like to have all other exception classes inherit from Error so it can be generically rescued like below but totally up to you. I find passing the callstack (caller) to raise helps a lot too as far as usability goes, it can be annoying when something blows up and its referencing far lower in the stack
... blah blah rescue Position::ParseError # Do something rescue Position:Error # All other errors raised from Position ...
Martin Carel
January 23, 2009, January 23, 2009 21:58, permalink
I like that!
Thanks for the trick.
Is there a better way to do this?
I wish I could do something along the line of:
@x, @y = input.scan(/([0-9]).*([0-9])/)