# Code refactoring: request for comment

**URL:** <https://www.ruby-forum.com/t/code-refactoring-request-for-comment/187823>\
**Category:** Ruby\
**Created:** [April 29, 2010, 11:17pm UTC](https://www.ruby-forum.com/t/code-refactoring-request-for-comment/187823 "2010-04-29T23:17:54Z")\
**Posts on this page:** 2\
**Page:** 1

<div class="post-metadata">

**Author:** ![trevoke](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/ee7513/32.png) [@trevoke](https://www.ruby-forum.com/u/trevoke)\
**Post date:** [April 29, 2010, 11:17pm UTC](https://www.ruby-forum.com/t/code-refactoring-request-for-comment/187823/1 "2010-04-29T23:17:54Z")

</div>

Hi all,  
I’ve just finished doing some pretty heavy refactoring on a method. I’d  
like to know if anyone can offer any suggestions or feedback.

This is the old one:

> <https://github.com/Trevoke/SGFParser/blob/2684c78738a0cc8cc8de0ad73f6a96efa7aa1101/lib/sgf/parser/tree_parse.rb>

This is the new one:

> <https://github.com/Trevoke/SGFParser/blob/45dfc8649cbf021c0472d985f8fe9e837e151579/lib/sgf/parser/tree_parse.rb>

In short, I am parsing an SGF file.  
The basic important characters which delimit the nodes are (, ), ;, [  
and ].

Thanks in advance.

---

<div class="post-metadata">

**Author:** ![trevoke](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/t/ee7513/32.png) [@trevoke](https://www.ruby-forum.com/u/trevoke)\
**Post date:** [April 30, 2010, 12:26am UTC](https://www.ruby-forum.com/t/code-refactoring-request-for-comment/187823/2 "2010-04-30T00:26:26Z")

</div>

Hi Aldric –

On Fri, 30 Apr 2010, Aldric G. wrote:

> In short, I am parsing an SGF file.  
> The basic important characters which delimit the nodes are (, ), ;, [  
> and ].

In looking it over, the one place where it was hard to see what was  
going on was:

```
       when '['
         get_property
         @content[@identity] ||= ""
         @content[@identity] << @property
         @identity = ""

```

It’s not clear that get\_property does anything to @property. I mean,  
it’s clear once you look at get\_property, but not from the calling  
context.

Could you do this instead…

```
       when '['
         @content[@identity] ||= ""
         @content[@identity] << get_property
         @identity = ""

```

and then retool get\_property to use a temporary buffer instead of  
@property? If so, you could probably get rid of @property entirely.

Now, just for fun, and because I’m a bit of a compulsive refactorer,  
I’ve created this: [Refactoring in response to Aldric · GitHub](http://gist.github.com/384362) which probably uses  
totally wrong method names but which paints a pretty good picture of  
what would happen if you wanted to push all the busy work out of the  
case statement entirely and let the instance variables and so on talk  
to themselves elsewhere. (You’ll see what I mean 🙂 I don’t know the  
SGF format myself, and haven’t subjected the code to any reality  
checks beyond making sure your specs pass.

David

–  
David A. Black, Senior Developer, Cyrus Innovation Inc.

THE Ruby training with Black/Brown/McAnally  
COMPLEAT Coming to Chicago area, June 18-19, 2010!  
RUBYIST [http://www.compleatrubyist.com](http://www.compleatrubyist.com)