[NotarySojac]

A couple days ago, I decided to add a feature to someone else's gem that I'm using in a production app (it converts BBCode into html). But as I was going over the foreign code, I found that one of the methods was intolerably long. My first reaction is to just split it off into smaller functions, but after some pondering I decided that a class should be designed to support these functions. I'm wondering if I can get some opinions on this kind of refactoring.

I'm also curious as to what the best way is to recommend refactorings to maintainers of open source code. If somebody on the internet sent me a pull request of refactorings, I think I'd probably find it a bit of a bother to go through someone else's code and figure out what they're doing and why. Do you know what I mean? But then again that's never happened, so I don't know first hand what that would be like, and I'm pretty confident of the merits of my refactoring.


Here's the original method that was way too big for my liking:

Note: the method is parsing a file that contains, say "an_id_of_video and sorting out the "tag info" and converting it into proper html code for displaying a youtube video in this case.

(https://github.com/veger/ruby-bbcode/blob/4a491cd315d4ec6c8c12e858b5be83ecad76ec8f/lib/ruby-bbcode.rb)

  def self.parse(text, tags = {})
    tags = @@tags if tags == {}
    tags_list = []
    @bbtree = {:nodes => []}
    bbtree_depth = 0
    bbtree_current_node = @bbtree
    text.scan(/((\[ (\/)? (\w+) ((=[^\[\]]+) | (\s\w+=\w+)* | ([^\]]*))? \]) | ([^\[]+))/ix) do |tag_info|

      ti = find_tag_info(tag_info)   # NOTICE HERE

      if ti[:is_tag] and !tags.include?(ti[:tag].to_sym)
        # Handle as text from now on!
        ti[:is_tag] = false
        ti[:text] = ti[:complete_match]
      end
     
      if !ti[:is_tag] or !ti[:closing_tag]
        if ti[:is_tag]
          tag = tags[ti[:tag].to_sym]
          unless tag[:only_in].nil? or (tags_list.length > 0 and tag[:only_in].include?(tags_list.last.to_sym))
            # Tag does to be put in the last opened tag
            err = "[#{ti[:tag]}] can only be used in [#{tag[:only_in].to_sentence(@@to_sentence_bbcode_tags)}]"
            err += ", so using it in a [#{tags_list.last}] tag is not allowed" if tags_list.length > 0
            return [err]
          end

          if tag[:allow_tag_param] and ti[:params][:tag_param] != nil
            # Test if matches
            return [tag[:tag_param_description].gsub('%param%', ti[:params][:tag_param])] if ti[:params][:tag_param].match(tag[:tag_param]).nil?
          end
        end

        if tags_list.length > 0 and tags[tags_list.last.to_sym][:only_allow] != nil
          # Check if the found tag is allowed
          last_tag = tags[tags_list.last.to_sym]
          allowed_tags = last_tag[:only_allow]
          if (!ti[:is_tag] and last_tag[:require_between] != true and ti[:text].lstrip != "") or (ti[:is_tag] and (allowed_tags.include?(ti[:tag].to_sym) == false))
            # Last opened tag does not allow tag
            err = "[#{tags_list.last}] can only contain [#{allowed_tags.to_sentence(@@to_sentence_bbcode_tags)}] tags, so "
            err += "[#{ti[:tag]}]" if ti[:is_tag]
            err += "\"#{ti[:text]}\"" unless ti[:is_tag]
            err += ' is not allowed'
            return [err]
          end
        end

        # Validation of tag succeeded, add to tags_list and/or bbtree
        if ti[:is_tag]
          tag = tags[ti[:tag].to_sym]
          tags_list.push ti[:tag]
          element = {:is_tag => true, :tag => ti[:tag].to_sym, :nodes => [] }
          element[:params] = {:tag_param => ti[:params][:tag_param]} if tag[:allow_tag_param] and ti[:params][:tag_param] != nil
        else
          text = ti[:text]
          text.gsub!("\r\n", "\n")
          text.gsub!("\n", "<br />\n")
          element = {:is_tag => false, :text => text }
          if bbtree_depth > 0
            tag = tags[bbtree_current_node[:tag]]
            if tag[:require_between] == true
              bbtree_current_node[:between] = ti[:text]
              if tag[:allow_tag_param] and tag[:allow_tag_param_between] and (bbtree_current_node[:params] == nil or bbtree_current_node[:params][:tag_param] == nil)
                # Did not specify tag_param, so use between.
                # Check if valid
                return [tag[:tag_param_description].gsub('%param%', ti[:text])] if ti[:text].match(tag[:tag_param]).nil?
                # Store as tag_param
                bbtree_current_node[:params] = {:tag_param => ti[:text]}
              end
              element = nil
            end
          end
        end
        bbtree_current_node[:nodes] << element unless element == nil
        if ti[:is_tag]
          # Advance to next level (the node we just added)
          bbtree_current_node = element
          bbtree_depth += 1
        end
      end

      if ti[:is_tag] and ti[:closing_tag]
        if ti[:is_tag]
          tag = tags[ti[:tag].to_sym]
          return ["Closing tag [/#{ti[:tag]}] does match [#{tags_list.last}]"] if tags_list.last != ti[:tag]
          return ["No text between [#{ti[:tag]}] and [/#{ti[:tag]}] tags."] if tag[:require_between] == true and bbtree_current_node[:between].blank?
          tags_list.pop

          # Find parent node (kinda hard since no link to parent node is available...)
          bbtree_depth -= 1
          bbtree_current_node = @bbtree
          bbtree_depth.times { bbtree_current_node = bbtree_current_node[:nodes].last }
        end
      end
    end
    return ["[#{tags_list.to_sentence((@@to_sentence_bbcode_tags))}] not closed"] if tags_list.length > 0

    true
  end

Huge method, right? So after lots of thinking, I realized what was potentially the optimal fix. Instead of using ti as just a plain old hash, I thought it would be better to refactor it into an object that works just like a hash, but also has methods attached to it that perform most of the work of the #parse method.

So the beginnings of my refactoring look like this:

(original)

    .
    .
    .
    text.scan(/((\[ (\/)? (\w+) ((=[^\[\]]+) | (\s\w+=\w+)* | ([^\]]*))? \]) | ([^\[]+))/ix) do |tag_info|
      ti = find_tag_info(tag_info)

      if ti[:is_tag] and !tags.include?(ti[:tag].to_sym)
        # Handle as text from now on!
        ti[:is_tag] = false
        ti[:text] = ti[:complete_match]
      end
      .
      .
      .
    end
    .
    .
    .

(refactored)

    .
    .
    .
    text.scan(/((\[ (\/)? (\w+) ((=[^\[\]]+) | (\s\w+=\w+)* | ([^\]]*))? \]) | ([^\[]+))/ix) do |tag_info|
      ti = TagInfo.new(tag_info, tags)

      ti.handle_unregistered_tags_as_text  # if the tag isn't in the tags list, then treat it as text
      .
      .
      .
    end
    .
    .
    .

gist: https://gist.github....onymous/5145597
(lib/ruby-bbcode/tag_info.rb)

module RubyBBCode
  class TagInfo
    def initialize(tag_info, tags)
      @tag_data = find_tag_info(tag_info)
      @tags = tags
      @tag = @tags[@tag_data[:tag].to_sym] unless @tag_data[:tag].nil?
    end
    
    def [](key)
      @tag_data[key]
    end
    
    def []=(key, value)
      @tag_data[key] = value
    end
    
    def handle_unregistered_tags_as_text
      if element_is_tag? and tag_included_in_tags_list?
        # Handle as text from now on!
        self[:is_tag] = false
        self[:text] = self[:complete_match]
      end
    end
    
    def allowed_outside_parent_tags?
      @tag = @tags[@tag_data[:tag].to_sym]
      @tag[:only_in].nil?
    end
    
    def constrained_to_within_parent_tags?
      @tag = @tags[@tag_data[:tag].to_sym]
      !@tag[:only_in].nil?
    end
    
    def element_is_tag?
      self[:is_tag]
    end
    
    def element_is_opening_tag?
      !self[:closing_tag]
    end
    
    def element_is_text?
      !self[:is_tag]
    end
    
    def tag_included_in_tags_list?
      !@tags.include?(self[:tag].to_sym)
    end

    def find_tag_info(tag_info)
      ti = {}
      ti[:complete_match] = tag_info[0]
      ti[:is_tag] = (tag_info[0].start_with? '[')
      if ti[:is_tag]
        ti[:closing_tag] = (tag_info[2] == '/')
        ti[:tag] = tag_info[3]
        ti[:params] = {}
        if tag_info[4][0] == ?=
          ti[:params][:tag_param] = tag_info[4][1..-1]
        elsif tag_info[4][0] == ?\s
          #TODO: Find params
        end
      else
        # Plain text
        ti[:text] = tag_info[8]
      end
      ti
    end
    
  end
end

Is that a good start? Also, I'm noticing an awkwardness to the code. The parse method loops through a glob of text, and creates TagInfo units... But TagInfo units are all instantiated with the same "tags" variable, which serves as kind of a global variable. That must imply that maybe I should refactor the code "above" the ti unit, so it can 'harbor' these TagInfo units along side the 'tags' data? So if anyone sees the code located at https://github.com/v.../ruby-bbcode.rb and has any insights into what a good refactoring would involve, I'm all ears on that.

This post has been edited by NotarySojac: 12 March 2013 - 11:36 AM

[NotarySojac]

After a lot of tinkering, I think I settled on an optimal representation of the code. I'm still interested in any second opinions/ feedback if anyway has any thoughts, but here's the look of things now (I'm quite glad at the improvements over the original, rather lengthy code).


(New and improved)

def self.parse(text, tags = {})
    tags = @@tags if tags == {}
    @tag_collection = TagCollection.new(text, tags)
    
    if @tag_collection.invalid?
      @tag_collection.errors 
    elsif @tag_collection.expecting_a_closing_tag?  # we're still expecting a closing tag...
      ["[#{@tag_collection.tags_list.to_sentence((@@to_sentence_bbcode_tags))}] not closed"]
    else
      true
    end
    
  end

You can see there's a lot that's been 'hidden away' right now. I kind of don't like that feeling when things get moved out of plain sight, but I think this is a necessary evil. Here's what the old code looked like for reference:

(old code)

def self.parse(text, tags = {})
  tags = @@tags if tags == {}
  tags_list = []
  @bbtree = {:nodes => []}
  bbtree_depth = 0
  bbtree_current_node = @bbtree
  text.scan(/((\[ (\/)? (\w+) ((=[^\[\]]+) | (\s\w+=\w+)* | ([^\]]*))? \]) | ([^\[]+))/ix) do |tag_info|

    ti = find_tag_info(tag_info)   # NOTICE HERE

    if ti[:is_tag] and !tags.include?(ti[:tag].to_sym)
      # Handle as text from now on!
      ti[:is_tag] = false
      ti[:text] = ti[:complete_match]
    end
   
    if !ti[:is_tag] or !ti[:closing_tag]
      if ti[:is_tag]
        tag = tags[ti[:tag].to_sym]
        unless tag[:only_in].nil? or (tags_list.length > 0 and tag[:only_in].include?(tags_list.last.to_sym))
          # Tag does to be put in the last opened tag
          err = "[#{ti[:tag]}] can only be used in [#{tag[:only_in].to_sentence(@@to_sentence_bbcode_tags)}]"
          err += ", so using it in a [#{tags_list.last}] tag is not allowed" if tags_list.length > 0
          return [err]
        end

        if tag[:allow_tag_param] and ti[:params][:tag_param] != nil
          # Test if matches
          return [tag[:tag_param_description].gsub('%param%', ti[:params][:tag_param])] if ti[:params][:tag_param].match(tag[:tag_param]).nil?
        end
      end

      if tags_list.length > 0 and tags[tags_list.last.to_sym][:only_allow] != nil
        # Check if the found tag is allowed
        last_tag = tags[tags_list.last.to_sym]
        allowed_tags = last_tag[:only_allow]
        if (!ti[:is_tag] and last_tag[:require_between] != true and ti[:text].lstrip != "") or (ti[:is_tag] and (allowed_tags.include?(ti[:tag].to_sym) == false))
          # Last opened tag does not allow tag
          err = "[#{tags_list.last}] can only contain [#{allowed_tags.to_sentence(@@to_sentence_bbcode_tags)}] tags, so "
          err += "[#{ti[:tag]}]" if ti[:is_tag]
          err += "\"#{ti[:text]}\"" unless ti[:is_tag]
          err += ' is not allowed'
          return [err]
        end
      end

      # Validation of tag succeeded, add to tags_list and/or bbtree
      if ti[:is_tag]
        tag = tags[ti[:tag].to_sym]
        tags_list.push ti[:tag]
        element = {:is_tag => true, :tag => ti[:tag].to_sym, :nodes => [] }
        element[:params] = {:tag_param => ti[:params][:tag_param]} if tag[:allow_tag_param] and ti[:params][:tag_param] != nil
      else
        text = ti[:text]
        text.gsub!("\r\n", "\n")
        text.gsub!("\n", "<br />\n")
        element = {:is_tag => false, :text => text }
        if bbtree_depth > 0
          tag = tags[bbtree_current_node[:tag]]
          if tag[:require_between] == true
            bbtree_current_node[:between] = ti[:text]
            if tag[:allow_tag_param] and tag[:allow_tag_param_between] and (bbtree_current_node[:params] == nil or bbtree_current_node[:params][:tag_param] == nil)
              # Did not specify tag_param, so use between.
              # Check if valid
              return [tag[:tag_param_description].gsub('%param%', ti[:text])] if ti[:text].match(tag[:tag_param]).nil?
              # Store as tag_param
              bbtree_current_node[:params] = {:tag_param => ti[:text]}
            end
            element = nil
          end
        end
      end
      bbtree_current_node[:nodes] << element unless element == nil
      if ti[:is_tag]
        # Advance to next level (the node we just added)
        bbtree_current_node = element
        bbtree_depth += 1
      end
    end

    if ti[:is_tag] and ti[:closing_tag]
      if ti[:is_tag]
        tag = tags[ti[:tag].to_sym]
        return ["Closing tag [/#{ti[:tag]}] does match [#{tags_list.last}]"] if tags_list.last != ti[:tag]
        return ["No text between [#{ti[:tag]}] and [/#{ti[:tag]}] tags."] if tag[:require_between] == true and bbtree_current_node[:between].blank?
        tags_list.pop

        # Find parent node (kinda hard since no link to parent node is available...)
        bbtree_depth -= 1
        bbtree_current_node = @bbtree
        bbtree_depth.times { bbtree_current_node = bbtree_current_node[:nodes].last }
      end
    end
  end
  return ["[#{tags_list.to_sentence((@@to_sentence_bbcode_tags))}] not closed"] if tags_list.length > 0

  true
end

To pull off most of the work, I used an class I named a "collection" thanks to my C# background, but that name isn't entirely descriptive of the pattern I used, because there's no need for it to "collect" any of those tag_info items, it really just parses through the text and temporarily analyzes tag info and subsequently discards the tag_info as it reads the next node. Here's what my "collection" looks like. Again, any thoughts and alternative names are welcome.


(lib/ruby-bbcode/tag_collection.rb)

module RubyBBCode
  class TagCollection
    def initialize(text, tags)
      @text = text
      @defined_tags = tags
      @tags_list = []
      @bbtree = {:nodes => []}
      @bbtree_depth = 0
      @bbtree_current_node = @bbtree
      
      @tag_info_collection = []
      @errors = false
      
      commence_scan
    end
    
    def commence_scan
      @text.scan(/((\[ (\/)? (\w+) ((=[^\[\]]+) | (\s\w+=\w+)* | ([^\]]*))? \]) | ([^\[]+))/ix) do |tag_info|
        require 'pry'
        
        
        ti = TagInfo.new(tag_info, @defined_tags)    # TODO:  ti should be a full fledged class, not just a hash... it should have methods like #handle_bracketed_item_as_text...
  
        ti.handle_unregistered_tags_as_text  # if the tag isn't in the @defined_tags list, then treat it as text
        
        # if it's text or if it's an opening tag...
        # originally:  !ti[:is_tag] or !ti[:closing_tag]
        if ti.element_is_text? or ti.element_is_opening_tag?
          
          left = !ti[:is_tag] and !ti.element_is_opening_tag?
          right = ti[:is_tag] and ti.element_is_opening_tag?
          # debugging
          if right
            #log("got here...")
            #log(ti[:closing_tag].inspect)
            #log(ti.tag_data.inspect)
          end
          
          # if it's an opening tag...
          # originally:  ti[:is_tag]
          if ti.element_is_opening_tag?
            tag = @defined_tags[ti[:tag].to_sym]
            
            unless ti.allowed_outside_parent_tags? or (@tags_list.length > 0 and tag[:only_in].include?(@tags_list.last.to_sym))
              #binding.pry
              # Tag does to be put in the last opened tag
              err = "[#{ti[:tag]}] can only be used in [#{tag[:only_in].to_sentence(RubyBBCode.to_sentence_bbcode_tags)}]"
              err += ", so using it in a [#{@tags_list.last}] tag is not allowed" if @tags_list.length > 0
              @errors = [err]  # TODO: Currently working on this...
              #return [err]
              return   # TODO:  refactor these returns so that they follow a case when style syntax...  I think this will break things
                       #  Like when you parse a huge string, and it contains 1 error at the top... it will stop scanning the file
                       #  when a return is struck because it's popping completely out of the class and won't have a chance to keep scanning
                       #  ... although wait a second... that's the current behavior isn't it??
            end
  
            if tag[:allow_tag_param] and ti[:params][:tag_param] != nil
              # Test if matches
              if ti[:params][:tag_param].match(tag[:tag_param]).nil?
                @errors = [tag[:tag_param_description].gsub('%param%', ti[:params][:tag_param])]
                return
              end
            end
          end
  
          if @tags_list.length > 0 and  @defined_tags[@tags_list.last.to_sym][:only_allow] != nil
            # Check if the found tag is allowed
            last_tag = @defined_tags[@tags_list.last.to_sym]
            allowed_tags = last_tag[:only_allow]
            if (!ti[:is_tag] and last_tag[:require_between] != true and ti[:text].lstrip != "") or (ti[:is_tag] and (allowed_tags.include?(ti[:tag].to_sym) == false))
              # Last opened tag does not allow tag
              err = "[#{@tags_list.last}] can only contain [#{allowed_tags.to_sentence(RubyBBCode.to_sentence_bbcode_tags)}] tags, so "
              err += "[#{ti[:tag]}]" if ti[:is_tag]
              err += "\"#{ti[:text]}\"" unless ti[:is_tag]
              err += ' is not allowed'
              @errors = [err]
              return
            end
          end
  
          # Validation of tag succeeded, add to @tags_list and/or bbtree
          if ti[:is_tag]
            tag = @defined_tags[ti[:tag].to_sym]
            @tags_list.push ti[:tag]
            element = {:is_tag => true, :tag => ti[:tag].to_sym, :nodes => [] }
            element[:params] = {:tag_param => ti[:params][:tag_param]} if tag[:allow_tag_param] and ti[:params][:tag_param] != nil
          else
            text = ti[:text]
            text.gsub!("\r\n", "\n")
            text.gsub!("\n", "<br />\n")
            element = {:is_tag => false, :text => text }
            if @bbtree_depth > 0
              tag = @defined_tags[@bbtree_current_node[:tag]]
              if tag[:require_between] == true
                @bbtree_current_node[:between] = ti[:text]
                if tag[:allow_tag_param] and tag[:allow_tag_param_between] and (@bbtree_current_node[:params] == nil or @bbtree_current_node[:params][:tag_param] == nil)
                  # Did not specify tag_param, so use between.
                  
                  # Check if valid
                  if ti[:text].match(tag[:tag_param]).nil?
                    @errors = [tag[:tag_param_description].gsub('%param%', ti[:text])]
                    return
                  end
                  
                  # Store as tag_param
                  @bbtree_current_node[:params] = {:tag_param => ti[:text]} 
                end
                element = nil
              end
            end
          end
          @bbtree_current_node[:nodes] << element unless element == nil
          if ti[:is_tag]
            # Advance to next level (the node we just added)
            @bbtree_current_node = element
            @bbtree_depth += 1
          end
        end
        
  
        if  ti[:is_tag] and ti[:closing_tag]
          if ti[:is_tag]
            tag = @defined_tags[ti[:tag].to_sym]
            
            if @tags_list.last != ti[:tag]
              @errors = ["Closing tag [/#{ti[:tag]}] does match [#{@tags_list.last}]"] 
              return
            end
            if tag[:require_between] == true and @bbtree_current_node[:between].blank?
              @errors = ["No text between [#{ti[:tag]}] and [/#{ti[:tag]}] tags."]
              return
            end
            @tags_list.pop
  
            # Find parent node (kinda hard since no link to parent node is available...)
            @bbtree_depth -= 1
            @bbtree_current_node = @bbtree
            @bbtree_depth.times { @bbtree_current_node = @bbtree_current_node[:nodes].last }
          end
        end
      end
    end
    
    def tags_list
      @tags_list
    end
    
    def bbtree
      @bbtree
    end
    
    def errors
      @errors
    end
    
    def invalid?
      @errors != false
    end
    
    def expecting_a_closing_tag?
      @tags_list.length > 0
    end
    
  end
end

There's still some awkward deviations from SOLID that I feel (notice the references to `RubyBBCode.to_sentence_bbcode_tags`), but I think I'll be able to fix those up now that the grand scheme has been sorted out.

This post has been edited by NotarySojac: 12 March 2013 - 03:50 PM

[Lemur]

One of the nice Rubyisms you can use is the short circuit, tags ||= @@tags would set tags equal to the class variable only if the tags variable isn't defined in that method call.

Other than that, refactoring would be more of a method of sitting down and sketching things out, defining what a proper layout would be for things that would make sense as objects.

[blackcompe]

I think it's good to get the perspective of someone new to the language, yet still understands coding. I haven't been working with Ruby that long, but from the overview I did of the old code and the refactored code, I'd say your changes made the code more understandable. I think commence_scan is still pretty long though, and could probably be refactored into two or three methods. Past that, I don't know if much more abstracting should be done, as doing so could lead to unnecessary code wrapping.

This post has been edited by blackcompe: 14 March 2013 - 06:26 AM

[NotarySojac]

Thanks for the feedback.

About the commence_scan, I literally spent I think two days performing refactors and it fits on a single screen now. Instead of doing very much planning (other than my original 'writers block' period where I wasn't sure what to do) I was able to just kind of wing through it, make quick comments in the code outlining the responsibilities I was trying to "thatch onto" the data holders (plain old hashes). I never went over modeling objects in school though, I've always wondered if I should do some research into charting and graphing classes out.

Anyway, the final 'commence_scan' method now looks like this, I think this is a huge improvement. Hopefully the code maintainer finds it easy to follow too (and has time to accept a pull request).

def process_text
      @text.scan(/((\[ (\/)? (\w+) ((=[^\[\]]+) | (\s\w+=\w+)* | ([^\]]*))? \]) | ([^\[]+))/ix) do |tag_info|
        @ti = TagInfo.new(tag_info, @defined_tags)
        
        @ti.handle_unregistered_tags_as_text  # if the tag isn't in the @defined_tags list, then treat it as text
        return if !valid_element?
        
        # Validation of tag succeeded, add to @tags_list and/or bbtree
        if @ti.element_is_opening_tag?
          element = {:is_tag => true, :tag => @ti[:tag].to_sym, :nodes => [] }
          element[:params] = {:tag_param => @ti[:params][:tag_param]} if @ti.can_have_params? and @ti.has_params?
          @bbtree_current_node[:nodes] << BBTree.new(element) unless element.nil?  # FIXME:  It can't be nil here... but can elsewhere
          escalate_bbtree(element)
        elsif @ti.element_is_text?
          element = {:is_tag => false, :text => @ti.text }
          if within_open_tag?
            tag = @defined_tags[@bbtree_current_node[:tag]]
            if tag[:require_between]
              @bbtree_current_node[:between] = @ti[:text]
              if candidate_for_using_between_as_param?
                use_between_as_tag_param    # Did not specify tag_param, so use between.
              end
              element = nil
            end
          end

          @bbtree_current_node[:nodes] << BBTree.new(element) unless element.nil?
          
        elsif @ti.element_is_closing_tag?
          retrogress_bbtree
        end
      end

The only confusing thing left is the @bbtree_current_node[:nodes] << BBTree.new(element) unless element.nil? which I'm thinking might make for a good method in it's own BBTree class but I'm still deciding if that's how I should go about it.

This has been a really fun experience, holy crap. It was discouraging looking at the code initially, but now I feel like I know the code as though I wrote it myself. It's a bummer how time consuming it has been though (I think 80% of my time has been spent decoding his boolean conditions, gah!).