def parse_line
if line_is_parsable?
if line_is_nested?
if line_is_attribute?
parse_attribute
else
# do parsing
end
else
@line_stack.pop
end
else
skip_line
end
end
Refactorings
No refactoring yet !
Ben Burkert
September 26, 2007, September 26, 2007 17:05, permalink
So I started breaking it up using the :alias_method_chain from rails.
It's definitely not the dry approach, so I'm still looking for a better way to do this.
def parse_line
#do parsing
end
def parse_line_with_attribute_check
if line_is_attribute?
parse_attribute
else
parselinewithoutattributecheck
end
end
alias_method_chain :parse_line, :attribute_check
def parse_line_with_nested_check
if line_is_nested?
parse_line_without_nested_check
else
@line_stack.pop
end
end
alias_method_chain :parse_line, :nested_check
def parse_line_with_parsable_check
if line_is_parsable?
parse_line_without_parsable_check
else
skip_line
end
end
aliasmethodchain :parseline, :parsablecheck
macournoyer
September 26, 2007, September 26, 2007 17:18, permalink
Why not restructure everything in a more OO way ? Everything seems to be around analysing a line so why not make a Line class with specialized subclasses.
class Line
def parse
skip_line
end
end
class Parsable < Line
def parse
@line_stack.pop
end
end
class Nested < Parsable
def parse
# do parsing
end
end
class Attribute < Nested
def parse
parse_attribute
end
end
Christoffer Hammarström
September 28, 2007, September 28, 2007 00:38, permalink
Just invert the if's?
def parse_line
if !line_is_parsable?
skip_line
return
end
if !line_is_nested?
@line_stack.pop
return
end
if !line_is_attribute?
parse_attribute
return
end
# do parsing
end
Christoffer Hammarström
September 28, 2007, September 28, 2007 00:39, permalink
Just invert the if's?
def parse_line
if !line_is_parsable?
skip_line
return
end
if !line_is_nested?
@line_stack.pop
return
end
if line_is_attribute?
parse_attribute
return
end
# do parsing
end
Christoffer Hammarström
September 28, 2007, September 28, 2007 01:07, permalink
That last if should have been
if line_is_attribute?
Jon Evans
September 28, 2007, September 28, 2007 04:08, permalink
Making it more OO looks like the way to go, but referring to Christoffer Hammarström's post, rather than invert the tests like that I'd use 'unless'.
def parse_line
unless line_is_parsable?
skip_line
return
end
unless line_is_nested?
@line_stack.pop
return
end
unless line_is_attribute?
parse_attribute
return
end
# do parsing
end
Garth Smedley
September 28, 2007, September 28, 2007 04:21, permalink
I like the inverted if idea, I think you can eliminate the returns using elsif's
def parse_line
if !line_is_parsable?
skip_line
elsif !line_is_nested?
@line_stack.pop
elsif !line_is_attribute?
parse_attribute
else
# do parsing
end
end
Anon
September 28, 2007, September 28, 2007 05:07, permalink
You can also obfuscate it using case :)
def parse_line
case
when !line_is_parsable?
skip_line
when !line_is_nested?
@line_stack.pop
when line_is_attribute?
parse_attribute
else
# do parsing
end
end
Frahugo
September 29, 2007, September 29, 2007 08:00, permalink
I usually don't like to have multiple return point in a method since if might complicate maintenance. But when it is a very small method (which it should be), I allow me to put more than one return point.
So here's how I would do it... easy to read... easy to maintain.
def parse_line2 return skip_line unless line_is_parsable? return @line_stack.pop unless line_is_nested? return parse_attribute if line_is_attribute? # do parsing # ... end
I hate arrow code. It's ugly.
And it's also hard to debug, which really sucks for me because I have the most bugs per line in arrow code.
Here's what I'm talking about: