So I'm trying to refactor one of our services that checks the Markdown before cleaning it up a bit more.
Here's the original:
# instance method in article.rb
def fix_markdown
fixed_body_markdown = MarkdownFixer.new(body_markdown).fix_all
fixed_body_markdown.gsub!("\r\n", "\n")
fixed_body_markdown.gsub!(/\ntags:.*\n/) do |tags|
tags.split(" #").join(",").gsub("#", "").gsub(":,", ": ")
end
fixed_body_markdown
end
# markdown_fixer.rb service
class MarkdownFixer
attr_accessor :markdown
def initialize(markdown)
@markdown = markdown
end
# This turns --- into ------- after the first two,
# because --- messes with front matter
def modify_hr_tags
markdown.gsub(/^---/).with_index { |m, i| i > 1 ? "#{m}-----" : m }
end
end
The goal is to put the fix_markdown
method from article.rb
into MarkdownFixer.rb
, and add an additional step to add quotation marks around the title, add_quotes_to_title
. The tricky part is that I need the return value of each .gsub
method call, and I personally think chaining the methods over and over would not be readable.
class MarkdownFixer
attr_accessor :markdown
def initialize(markdown)
@markdown = markdown
end
def fix_all
# this part is my pain point
markdown = add_quotes_to_title
markdown = modify_hr_tags
markdown = markdown.gsub!("\r\n", "\n")
markdown.gsub!(/\ntags:.*\n/) do |tags|
tags.split(" #").join(",").gsub("#", "").gsub(":,", ": ")
end
end
def add_quotes_to_title
quoted_beginning = markdown.gsub("title: ", "title: \"")
quoted_beginning.gsub("\r\npublished: ", "\"\r\npublished: ")
end
# This turns --- into ------- after the first two,
# because --- messes with front matter
def modify_hr_tags
markdown.gsub(/^---/).with_index { |m, i| i > 1 ? "#{m}-----" : m }
end
end
ANYWAY, I'm not too big of a fan of setting markdown
over and over again, but I'm not sure of a better way of cleaning up the fix_all
method. Would love to hear your thoughts.
Top comments (9)
Hi Andy,
I thought about three variations.
The first one uses
gsub!
and method chaining. Each method has its own clear responsibility and the "entry point" method,fix_all
just chains the calls:The second option is a variation on the first one, because looking at your example I see that the MarkdownFixer is a throw away object, it doesn't really need to hold state, just to transform it and return it parsed:
The third one is "almost" functional, in the sense there's no state at all and the transformations pass down the value (especially because the order is not exactly important):
You might also look into the visitor pattern which is definitely overkill for this example :D
This is my favorite one for now :D
This line
methods.inject(markdown){|result, method| self.send(method, result) }
calls each method in the array passing the result of the previous method the following one, withmarkdown
as the initial value.Hmmmmm this one is pretty great! I like the use
inject
a lot. Thanks a lot for the answers! I'm going to think it over on my commute home. :)Love these smaller methods.
Easy to read && easy to follow.
Learned some neat refactoring tips-- thanks!
refine the String class then just chaining methods at fix_all
Dev.to is written in Ruby?๐ฎ๐๐ค
removes his account
Why do you use classes here? As i understand there is no reason to instantiate the MarkdownFixer. A module could be a more meaningful solution. What do you think? ๐
Another more dry approach:
What do you think about this: :)