Simple Ruby directory navigator functions

Question

In the process of writing scripts to work with CSVs and other files of the like I found myself in the need of a way to navigate and select files for various purposes. I wrote the following function for one of the scripts I needed to finish quickly and now I'm trying to get it into a more reusable form

def select_file()
 i = 1
 selection_set = Dir.glob("*") 
 puts "--------------------------------------------------------------------------"
 puts "Listing entries in #{File.basename(Dir.pwd)}:"
 puts
 selection_set.each do |entry|
 puts i.to_s + " - " + entry
 i = i+1 
 end
 print "Select File/Folder: "
 user_selection = gets.chomp
 selection = selection_set[user_selection.to_i - 1]
 puts "Selected: #{selection}"
 puts "--------------------------------------------------------------------------"
 selection
end

In an attempt to make this more reusable in the future I refactored this single function into a series of functions. Part of the motivation in doing it this way was to attempt to use this for displaying other sets of things the user may need to select. Was I better off sticking with the original approach or am I on a decent track but not quite doing it right? I get the sense this can all be done much simpler. Also I am deliberately avoiding using any kind of UI gem for no reason other than I am trying to learn.

def full_directory_navigator
 selection_set = Dir.glob('*', File::FNM_DOTMATCH)
 list_directory_entries(selection_set)
end
def dir_header(pattern)
 100.times { |i| !(i == 99) ? print('-') : puts }
 pattern_message = (pattern == '*') ? '' : "matching \"#{ pattern }\""
 puts
 puts "Listing entries in #{ File.basename(Dir.pwd) }#{ pattern_message }"
 puts
 puts 'Index Entry'
 100.times { |i| !(i == 99) ? print('-') : puts }
end
def dir_select
 print 'Select File/Folder: '
end
def spacer(i)
 size = i.to_s.size
 spacer = ''
 (9 - size).times { spacer = ' ' + spacer }
 spacer
end
def display_selection_set(selection_set)
 i = 1
 selection_set.each do |entry|
 puts i.to_s + spacer(i) + '- ' + entry
 i += 1
 end
end
def list_directory_entries(selection_set, pattern = '*')
 dir_header(pattern)
 display_selection_set(selection_set)
 dir_select
 user_selection = gets.chomp.to_i
 selection = selection_set[user_selection - 1]
 puts "Selected: #{selection}"
 selection
end
full_directory_navigator

Any and all comments are appreciated. If my code sucks please let me know so I can start writing less awful code. I also just realized I didn't comment the code at all, so I will begin commenting it and if there are any ambiguous sections it would be greatly appreciated if it could be brought to my attention.

• Nakilon: I would like to know the solution of throwing this keystrokes right to bash autocomplete.

Answer 1

A few notes on your current implementation(s):

• Use string interpolation. E.g. i.to_s + " - " + entry becomes "#{i} - #{entry}" (the to_s is implicit with string interpolation).

• Repeating strings can be done by multiplying the string with *, e.g. puts "-" * 99.

• By the way, if you want to print something "full width" the most common default is to print 80 characters. Of course, there are also ways of figuring out the terminal's actual width, but that's out of scope for this answer.

• !(i == 99) seems backwards. The more straightforward code would be i != 99.

• each with a "manually" maintained index (the i = i + 1, which incidentally is more commonly written as i += 1) can be replaced with each_with_index

• Add a little input checking. A line like selection_set[user_selection.to_i - 1] is pretty fragile. What if the user typed "foobar" or "-99999"? You quickly get nil - or worse: The wrong item, as 200_success points out. So the script may carry on, even though the user entered complete nonsense.

• Your spacer method appears to do the same as the built-in String#rjust

---

I'm generally in favor of breaking things into separate parts. Your original code is probably fine for its stated purpose, but, as you say, you want it to be more generic.

Part of the motivation in doing it this way was to attempt to use this for displaying other sets of things the user may need to select.

And you're close. But your methods still have some assumptions about being about directories, or seem to have been split along slightly arbitrary lines.

Forget about directories for a moment, and just focus on presenting options for a user to pick. Any options.

Try making a small script that just does that - just make pick words in a list or something. Once you're happy with it, then try using to pick through directories. Don't try to anticipate too much; let the actual needs dictate the design.

• Soviet_Jesus: In general then when I'm trying to make something I've written generic, should I almost clear my head of the specific problem I just solved and focus only on what I know to be the abstract contents? (For example not worrying about actually generating the selection set, but rather take it as a given and work from there?) So for example would it be productive to take dir_header, and come up with some sort of generic selection header function who's argument is the message? or can I abstract even further?
• Flambino: @Soviet_Jesus Uh, it's a big topic :) Typically, only making perfectly abstract tools leads you to write too much stuff, because everything might be useful. It can quickly become a fun exercise, but have little practical use. So start with a very concrete implementation and make that work, and then worry about making it more generic and abstract - if/when you need it. In this case, you've already made a concrete implementation (the 1st one), so now you're on to step 2. The trick is to find a balance between writing generic and specific stuff, but that's a matter of practice.
• Flambino: @Soviet_Jesus To answer your question more directly: I wouldn't worry about making a "generic header method" - unless you find yourself repeating the code for print such headers all the time. But if you just need to print a header once, then just print it and move on. Another way to figure out what to extract/abstract is if it's a task that just feels generic. Like "select something from a list". That, in and of itself, is a generic task, so it could be useful to extract that logic, so you can focus on it. And once solved, it becomes a building block for something else.
• Soviet_Jesus: Part of my "dream" if you will is to build this towards a more complete UI suite I can just require on my files and then "draw" the components I need (prawn is what comes to mind). Obviously you can't review code I haven't written, but moving forward, after implementing your and 200_success's comments, would the next step to be and try and organize things into classes, or is it purely a situation by situation thing that I'll just have to figure out with experience? What do people typically call this topic? I'm very interested in researching it further.
• Flambino: @Soviet_Jesus Ooh, I can't think of a specific name for this. It's sort of impinges on a lot of general software engineering topics. For your project, I'd say build what you need right now. If you need something later, your need it later. Not now. So don't overdo it. Still, as mentioned, the trick it to notice when there's an opportunity to make something more generic without overdoing it. The simplest indicator is probably repetition. Repeating some lines? Make a method. Using that method across classes? Put it in a module. Using it across projects? Make a gem. And so on.

Answer 2

Your two code samples are not exactly equivalent. The second one has nicer formatting (fixed-width numbering) and attempts to parameterize the glob pattern (but fails in that regard — full_directory_navigator should accept a pattern parameter and pass it to both Dir.glob and list_directory_entries).

I'm not convinced that splitting up the select_file function into six functions makes it any better. It's not any more readable or extensible than the original. The parameter passing just gets in the way. (If you had defined a class, there might be some advantage, in that a subclass could customize the behaviour by overriding one of the methods.)

With more effective string formatting, the code is simple enough to fit in one function.

• To repeat a character, use string multiplication.
• The ternary conditional could just be an if-qualified expression, since one of the outcomes is nil.
• To make a fixed-width column of numbers, use '%-8d' % number.

def select_file(glob='*', glob_flags=File::FNM_DOTMATCH)
 entries = Dir.glob(glob, glob_flags)
 # Header
 puts '-' * 79,
 "Listing entries in #{File.basename(Dir.pwd)}#{
 " matching \"#{glob}\"" if glob != '*'
 }",
 '',
 '%-8s %s' % ['Index', 'Entry'],
 '-' * 79
 # Listing
 entries.each_with_index { |entry, i| puts '%-8d - %s' % [i + 1, entry] }
 # Prompt
 print 'Select File/Folder: '
 entries[gets.to_i - 1]
end
selection = select_file()
puts "Selected: #{selection}"

Note that you haven't done any validation on the input. If the input doesn't look like a number, then gets.to_i - 1 becomes 0 - 1, which refers to the last item in the array.

• Soviet_Jesus: So if I understand you correctly, the initial function is simple enough to do what I want in the context of directories with a few modifications? As something of a follow up question regarding the initial select_file() function, is there any way to make that File::FNM_DOTMATCH optional? I tried some stuff with eval but couldn't seem to get the syntax right. Is there where one would use the splat method?
• 200_success: Think about what you want to be customizable before you start breaking things up, otherwise breaking things up just adds complication with no benefit.
• 200_success: You can make a glob_flags parameter that defaults to File::FNM_DOTMATCH, and turn it off by passing 0.