When to use a new variable vs string interpolation?

Question

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

Answer 1

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.

• user12204: 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.
• sgmorrison: 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.
• user12204: 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.
• sgmorrison: 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.