3
\$\begingroup\$

I wrote a script that I decided to refactor so I could add functionality to it as my coworkers think of it. I only saved four lines in the effort, but the main change is I removed both methods and reduced the number of called variables in favor of string interpolation/manipulation. Is there a preference for this? Is it better to declare a new variable just to use once, or is it more DRY to just make minor tweaks to the string when you need to use it? For example here is the original code:

$EMBED_HTML = "" 

$BASE_URL = ""

$SAMPLE_FLV = ""

def normalize_directory(dir)
  normalized = dir.gsub('\\', '/')
  normalized += '/' unless normalized[-1] == '/'
end

def validate_directory(dir)
  puts "Enter the full directory path of the flv files." unless dir
  dir =  dir || gets.chomp
  dir = normalize_directory(dir)
  until File.directory?(dir) && Dir.glob("#{dir}*.flv") != []
    puts "That directory either doesn't exist or contains no .flv files. \nEnter the full directory path of the flv files."
    dir = $stdin.gets.chomp
    dir = normalize_directory(dir)
  end

  dir
end

def output_html_wrapper(flv_filename, output_folder)
  html_filename = flv_filename.gsub(".flv", ".html")

  html_body = $EMBED_HTML.gsub($SAMPLE_FLV, flv_filename.sub(/[a-z][:]/, '')) 
  html_output = File.open(html_filename, "w")
  html_output.write(html_body)
  html_output.close

  link = flv_filename.sub(/[a-z][:]/, '').gsub(".flv", ".html")
  link_list = File.open("#{output_folder}List of Links.txt", "a")
  link_list.write($BASE_URL + link + "\n")
  link_list.close

  puts "#{html_filename} created successfully." if File.exists?(html_filename)
end

folder = ARGV[0].dup unless ARGV.empty?
folder = validate_directory(folder)
flvs = Dir.glob("#{folder}*.flv") 
File.delete("#{folder}List of Links.txt") if File.exists?("#{folder}List of Links.txt")
flvs.each { |flv| output_html_wrapper(flv, folder) }

And the new stuff:

flash_folder = ARGV[0].dup unless ARGV.empty?

if !flash_folder
  puts "Enter the full directory path of the flv files."
  flash_folder = gets.chomp
end

flash_folder.gsub!('\\', '/')
flash_folder += '/' unless flash_folder[-1..-1] == '/'
until File.directory?(flash_folder) && Dir.glob("#{flash_folder}*.flv") != []
  puts "That directory either doesn't exist or contains no .flv files. \nEnter the full directory path of the flv files."
  flash_folder = $stdin.gets.chomp
  flash_folder.gsub!('\\', '/')
  flash_folder += '/' unless flash_folder[-1..-1] == '/'
end

flash_files = Dir.glob("#{flash_folder}*.flv")

File.delete("#{flash_folder}List of Links.txt") if File.exists?("#{flash_folder}List of Links.txt")

flash_files.each do |flv|
  html_output = File.open("#{flv.gsub(".flv", ".html")}", "w")
    html_output.write("#{embed_code.gsub("sample.flv", flv.slice(7..flv.length))}")
  html_output.close
  link_list = File.open("#{flash_folder}List of Links.txt", "a")
    link_list.write("#{flash_url}#{flv.slice(2..flv.length).gsub(".flv", ".html")}\n")
  link_list.close
end

puts "Finished."
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Your refactor seems to have reduced the readability of your code, which is usually the opposite of your intent when refactoring. The code may be terser, and string interpolation and re-use is a part of how you've achieved this, but it doesn't make the code easy to follow. That will make it harder for you and your colleagues to modify it over time.

I actually prefer your original code a lot. You've pulled two methods out of your main execution flow, so it's possible to see at a glance what the code is doing - validate a directory, find all the files in it and do some form of outputting on each. That clarity is missing from the newer code because it is all written at the same level.

Your code makes use of #dup and #clone a lot. These methods are not needed in many of the places you use them, so they add noise to the code. For example files = folder.clone + "*.flv" does not need the #clone call because the + method will create a new String from the content of folder.clone and *.flv. Writing files = folder + "*.flv" would work just fine.

Here is a take on your original code that adds some readability by avoiding repetition and giving fuller names to variables and methods.

    def normalize_directory(dir)
        normalized = dir.gsub('\\', '/')
        normalized += '/' unless normalized.[-1,1] == '/'
    end

    def validate_directory(dir)
        if dir.empty?
            puts "Enter the full directory path of the flv files." 
            dir = gets.chomp
        end
        dir = normalize_directory(dir)

        until File.directory?(dir) && !Dir.glob("#{dir}*.flv").empty?
            puts "That directory either doesn't exist or contains no .flv files. \nEnter the full directory path of the flv files."
            dir = gets.chomp
            dir = normalize_directory(dir)
        end

        dir
    end

    def output_html_wrapper(flv_filename, output_folder)
        html_filename = flv_filename.gsub(".flv", ".html")
        vid = flv_filename.slice(0..6)

        html_body = $EMBED.gsub("sample.flv", vid) 
        html_output = File.open(html_filename, "w")
        html_output.write(html_body)
        html_output.close

        link = vid.gsub(".flv", ".html")
        link_list = File.open("#{output_folder}List of Links.txt", "a")
        link_list.write($BASE + link + "\n")
        link_list.close

        puts "Files created successfully."
    end

    folder = validate_directory(ARGV[0])             
    flvs = Dir.glob("#{folder}*.flv") 
    File.delete("#{folder}List of Links.txt") if File.exists?("#{folder}List of Links.txt")
    flvs.each { |flv| output_html_wrapper(flv, folder) }

Being DRY is about keeping a piece of knowledge in one place, whether that's a file name, the way you clean up some input or anything similar. The normalize_directory method I added DRYs up your code by putting in one place the logic for making a valid directory name out of the user's input.

Declaring a variable for a single use is extremely useful if it helps describe what the value is being used for. The vid variable in your code is still a mystery to me regarding what it actually contains. This is a great place to give something a name, and in doing so document your code without comments.

\$\endgroup\$
4
  • \$\begingroup\$ I like that you broke out the normalizing and the validation. I was thinking that might be the way to go in the methods version, and it is definitely clearer. In the validate_directory method, isn't the "unless dir" redundant with the if statement? Same for the "dir" at the end of the method. Is the return of dir not implied? Or is this just for clarity? What makes you decided to include an explicit return for clarity? Thanks for the great response. \$\endgroup\$
    – user12204
    Mar 28, 2012 at 22:23
  • \$\begingroup\$ The explicit return of dir was because it's not obvious what will be returned when there are conditionals - it's easier to see a single return point than to scan the different branch points. I've removed the unless dir, as you're absolutely right about it being redundant. \$\endgroup\$
    – sgmorrison
    Mar 29, 2012 at 15:06
  • \$\begingroup\$ So normalized.last was giving me a 'no method' error, and the vid thing (which I opted to just do with string manipulation on the fly) was to remove the drive letter and colon from the beginning of the file. This made me realize this script will only work on windows, so that's another thing I should update. \$\endgroup\$
    – user12204
    Mar 29, 2012 at 16:12
  • \$\begingroup\$ You're right on normalized.last - I forgot I was dealing with a String, not an Array. The ActiveSupport gem adds this function to String, including that for a small script is a step to far. Updated code again, with what seems to be a more common (and slightly more readable) way to get the last character. Note that I've not tested the code at all, so apologies for any bugs. \$\endgroup\$
    – sgmorrison
    Mar 29, 2012 at 20:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.