4d1c9dad17af98e55cb65b4efce27c42

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:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
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 !

4d1c9dad17af98e55cb65b4efce27c42

Ben Burkert

September 26, 2007, September 26, 2007 17:05, permalink

1 rating. Login to rate!

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.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
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
Bfec5f7d1a4aaafc5a2451be8c42d26a

macournoyer

September 26, 2007, September 26, 2007 17:18, permalink

1 rating. Login to rate!

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.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
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
Ca4eea2627fcfb4a6af4d05cf0cd634e

Christoffer Hammarström

September 28, 2007, September 28, 2007 00:38, permalink

No rating. Login to rate!

Just invert the if's?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
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
Ca4eea2627fcfb4a6af4d05cf0cd634e

Christoffer Hammarström

September 28, 2007, September 28, 2007 00:39, permalink

1 rating. Login to rate!

Just invert the if's?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
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
Ca4eea2627fcfb4a6af4d05cf0cd634e

Christoffer Hammarström

September 28, 2007, September 28, 2007 01:07, permalink

No rating. Login to rate!

That last if should have been

1
  if line_is_attribute?
Fd1769776698da69ffd5bdda094d8581

Jon Evans

September 28, 2007, September 28, 2007 04:08, permalink

1 rating. Login to rate!

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'.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
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
Avatar

Garth Smedley

September 28, 2007, September 28, 2007 04:21, permalink

1 rating. Login to rate!

I like the inverted if idea, I think you can eliminate the returns using elsif's

1
2
3
4
5
6
7
8
9
10
11
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
Avatar

Anon

September 28, 2007, September 28, 2007 05:07, permalink

1 rating. Login to rate!

You can also obfuscate it using case :)

1
2
3
4
5
6
7
8
9
10
11
12
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
B51ddb971f0e250f1cd68781f31b739b

Frahugo

September 29, 2007, September 29, 2007 08:00, permalink

4 ratings. Login to rate!

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.

1
2
3
4
5
6
7
8
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

Your refactoring





Format Copy from initial code

or Cancel