Search for directories with a certain prefix that contain no files of a given type

Question

These two functions search for directories with a certain prefix that contain no files of a given type.

1. Does the code adhere to Ruby standards and conventions?
2. Am I doing something the hard way or the "non-Ruby" way?
3. Should I use more/fewer/different comments?

def find_empties(directory, dirprefix, fsuffix)
  empty_dirs = Array.new
  Dir.chdir directory do
    Dir["#{dirprefix}**/"].each do |subdir|
      Dir.chdir subdir do
        empty_dirs.push subdir if Dir["**.#{fsuffix}"].empty?
      end
    end
  end
  return empty_dirs
end

def find_empties_sorted(directory, dirprefix, fsuffix)
  empties = find_empties(directory, dirprefix, fsuffix)
  nums = Hash.new
  non_numeric = Array.new
  empties.each do |x|
    # if starts with digits, trim leading zeroes and extract; else, nothing
    num = x.sub(/^#{Regexp.escape dirprefix}0*(\d+).*/, '\1')
    is_num = num !~ /\D/
    if is_num
      n = Integer num
      nums[n] = Array.new unless nums[n]
      nums[n].push x
    else
      non_numeric.push x
    end
  end
  result = Array.new
  nums.sort.each do |k, v|
    v.each do |x|
      result.push x
    end
  end
  result.concat non_numeric.sort
  result
end

The code works as intended, but here's a test case:

$ ls
emptyfinder.rb
emptyrunner.rb
$ cat emptyrunner.rb
require_relative 'emptyfinder.rb'

puts find_empties_sorted(Dir.pwd, 'e', 'txt')
$ mkdir testcase
$ cd testcase
$ mkdir e1 e2 e003 e004 e4 e5 e9 e10 e11 empty emptier emptynot
$ touch emptynot/file.txt
$ touch empty/not_a_txt.csv
$ ruby ../emptyrunner.rb
e1/
e2/
e003/
e004/
e4/
e5/
e9/
e10/
e11/
emptier/
empty/
$

Answer 1

You could simplify your code quite a bit. Here's one way:

def find_empties_sorted(directory, dirprefix, fsuffix)
  Dir["#{directory}/#{dirprefix}**/"].each_with_object([]) { |e,arr|
    arr << e[/[^\/]+(?=\/$)/] if Dir["#{e}**.#{fsuffix}"].empty? }
    .each_with_object ({}) do |x,h|
      n = x[/\d+$/]
      ni = n ? n.to_i : n
      (h[ni] ||= []) << (n ? x[0,x.size-n.size] + ni.to_s : x)
    end.sort { |(k1,_), (k2,_)| k1.nil? ? (k2.nil? ? 0 : 1) : (k2.nil? ? -1 : k1<=>k2) }
       .transpose
       .last
       .flatten
end

For the data you used, this would return:

  #=> ["e1",  "e2",  "e3",      "e4",   "e4", "e5",
  #    "e9", "e10", "e11", "emptier", "empty"]

If you want / at the end of each subdir, change .flatten to

.flatten.map { |s| s << '/' }
  #=> ["e1/",  "e2/",  "e3/",      "e4/",    "e4/", "e5/",
  #    "e9/", "e10/", "e11/", "emptier/", "empty/"]

I've used Enumerable#each_with_object (v1.9+) twice, as it provides a convenient way to build an object such as an array or hash (which is returned). In the first case the object is an initially-empty array (with block variable arr); in the second it is an initially-empty hash (with block variable h).

I will use your data--with two changes--to explain what's going on here. I've added an empty directory e and a file f.txt to the directory to testdir.

directory = 'testcase'
dirprefix = 'e'
fsuffix   = 'txt' 

a = Dir["#{directory}/#{dirprefix}**/"].each_with_object([]) { |e,arr|
      arr << e[/[^\/]+(?=\/$)/] if Dir["#{e}**.#{fsuffix}"].empty? }
  #=> [ "e", "e003", "e004", "e1",     "e10",   "e11",
  #    "e2",   "e4",   "e5", "e9", "emptier", "empty"]

A word about the regex /[^\/]+(?=\/$)/ (which I've used to avoid the need to chdir). [^\/]+ looks for a string that contains one or more characters other than /. (?=\/$) is called a positive lookahead. It is not part of the match (which is why it is sometimes referred to as having "zero width"), but it requires that the match be followed by /$, where $ signifies the end-of-line. This strips off directory from the beginning of e. To make it easier to remove leading zeros on trailing numbers, I've not included / at the end of each subdir. (It can be added at the end if desired.)

Next we execute

b = a.each_with_object ({}) do |x,h|
      n = x[/\d+$/]
      ni = n ? n.to_i : n
      (h[ni] ||= []) << (n ? x[0,x.size-n.size] + ni.to_s : x)
    end

Initially the hash h is empty. x is first assigned the value "e" (from the array a). We compute

n = x[/\d+$/]        #=> nil (because no trailing number)
ni = n ? n.to_i : n  #=> nil
(h[ni] ||= []) << (n ? x[0,x.size-n.size] + ni.to_s : x) # h => { nil=>["e"] }

h[ni] ||= [] is shorthand for h[ni] = h[ni] || [], so if h[ni] is undefined (nil), as it is here, || causes h[ni] => []; if h[i] is already defined as an array (so is neither nil nor false),[] is not considered. Alternatively (as @Nat mentioned), we could substitute (Hash.new { |h,k| h[k] = [] }) for ({}) following each_with_object.

Next, x is assigned the value "e003". We compute

n = x[/\d+$/]        #=> "003"
ni = n ? n.to_i : n  #=> 3
(h[ni] ||= []) << (n ? x[0,x.size-n.size] + ni.to_s : x)
  # h => { nil=>["e"], 3=>["e3"] } 

Continuing through the elements of a, we obtain:

b = {nil=>["e", "emptier", "empty"], 3=>["e3"], 4=>["e4", "e4"], 1=>["e1"],
      10=>["e10"], 11=>["e11"], 2=>["e2"], 5=>["e5"], 9=>["e9"]} 

We now sort the keys. b.sort would raise the exception ArgumentError: comparison of Array with Array failed because Fixnum objects cannot be compared with nil. We therefore need to define the "spaceship" method, <=> that sort is to use. Since we want the key nil to be last, we do it this way:

c = b.sort { |(k1,_), (k2,_)| k1.nil? ? (k2.nil? ? 0 : 1) : (k2.nil? ? -1 : k1<=>k2) }
  #=> [[1, ["e1"]], [2, ["e2"]], [3, ["e3"]], [4, ["e4", "e4"]], [5, ["e5"]],
  #    [9, ["e9"]], [10, ["e10"]], [11, ["e11"]], [nil, ["e", "emptier", "empty"]]]

Note that sort returns an array. We could have written:

c = b.sort { |(k1,v1), (k2,v2)|... 

where k1 => v1 is one member of the hash and k1 => v1 is another, but since we are sorting on the keys, and do not use the values v1 and v2, we can replace the latter with underscores.

We could have done this differently, using sort without a block, had we used n instead of nil for the key for directories without trailing numbers, and chosen n so that it was larger than any of the other keys. This is one way to do that:

empties = find_empties_sorted(directory, dirprefix, fsuffix)
  Dir["#{directory}/#{dirprefix}**/"].each_with_object([]) { |e,arr|
  arr << e[/[^\/]+(?=\/$)/] if Dir["#{e}**.#{fsuffix}"].empty? }
n = ('1' + '0'*empties.map(&:size).max).to_i #=> 10000000

We are almost finished. Next we transpose to obtain an array with only two elements:

d = c.transpose
  #=> [[1, 2, 3, 4, 5, 9, 10, 11, nil],
  #   [["e1"], ["e2"], ["e3"], ["e4", "e4"], ["e5"],
  #    ["e9"], ["e10"], ["e11"], ["e", "emptier", "empty"]]]

As we only want the second element:

e = d.last
  #=> [["e1"], ["e2"], ["e3"], ["e4", "e4"], ["e5"],
  #    ["e9"], ["e10"], ["e11"], ["e", "emptier", "empty"]]

And lastly, we must flatten:

e.flatten
  #=> ["e1",  "e2",  "e3", "e4",      "e4",    "e5",
  #    "e9", "e10", "e11",  "e", "emptier", "empty"]

This array is returned by the method.

If you would prefer less chaining, go ahead and add temporary variables and/or additional methods. The more you work with Ruby, however, the more chaining becomes the natural thing to do. It's no more difficult to debug, as you can do what I did in explaining what I've done.

In deciding the proper balance between conciseness and the use of temporary variables and separate methods, I'm always thinking about how the code reads. Aside from performance, my main objectives are to make my code read well and read fast. I want an experienced Rubyist to be able to read and comprehend it in minimum time. Personally, I much prefer reading tightly-chained code to code that is all spread out, uses lots of very short methods, lots of temporary variables with highly-descriptive (and accordingly, long) names, and so on.

It would take very little time (well under 30 seconds) for any experienced Rubyist to scan my code and have a general idea of what I was doing: creating an array of directories, converting that to a hash, sorting the hash on its keys, transposing the resulting array, extracting its last element and flattening. Yes, they would have to look more closely at certain parts, such as the regexes, sort criteria, and so on, and for that I would help by adding a few comments. Comprehension of some parts of the code, including .transpose, .last and .flatten would be virtually automatic, so it makes perfect sense to express those three operations as compactly as possible: here, three words separated by dots.

What I've just said is the opinion of one Ruby hobbiest. I invite others--particularly those who write Ruby code for a living--to weigh-in on these issues by adding comments.

Answer 2

Looks alright.

Your style is more spread out than is usually the case in ruby (though that's not necessarily a bad thing.) For example

is_num = num !~ /\D/
if is_num
  ...

could as well be one line sans assignments, without loosing clarity. if num !~ /\D/ # is a number

Instead of declaring nums to be a Hash, then having an if statement to check if a pair needs initialised with an array, you can just declare the hash with a block that it then uses to default all values to new arrays like so nums = Hash.new { |h,k| h[k] = [] }.

Instead of iterating through the array v of sorted nums and pushing each item to result, you can just do result.concat v. Sometimes for simple logic, denser one liners are arguably easier to read in ruby: e.g. nums.sort.each { |k, v| result.concat v }

Be aware that you can initialise arrays and hashes as literals, e.g. a = [] and this is sometimes prefered.

Your level of commenting seems appropriate to me, i.e. explaining what the code is meant to do (or why but not how) when it's not obvious from the code itself.

Answer 3

Although @CarySwoveland gave a very extensive answer, with quite a few good tips, I agree with @WChargin's remark on the code not being very readable, as it is riddled with es,ns,xs and hs, which make it not very readable.

Also using the more obscure transpose.last is a nice trick, but, using map(&:last) will give the same result, and look more familiar to more ruby developers.

The pattern:

.each_with_object([]) { |e,arr|
    arr << e[/[^\/]+(?=\/$)/] if Dir["#{e}**.#{fsuffix}"].empty? }

could be more readable when written with a combination of select and map:

.select { |e| Dir["#{e}**.#{fsuffix}"].empty? }.map { |e| e[/[^\/]+(?=\/$)/] }

e[/[^\/]+(?=\/$)/] - could be replaced by File.basename(e), which is, again, more readable, and less error-prone to future maintainers of your code.

The pattern:

.each_with_object ({}) do |x,h|
   n = x[/\d+$/]
   ni = n ? n.to_i : n
   (h[ni] ||= []) << x
 end

can be better represented using group_by:

.group_by do |x|
  n = x[/\d+$/]
  n ? n.to_i : n
end

Here is my version of the code:

def find_empties_sorted(directory, dirprefix, fsuffix)
  Dir["#{directory}/#{dirprefix}**/"]
     .select { |dirName| Dir["#{dirName}**.#{fsuffix}"].empty? }
     .group_by { |dirName| (suffix = dirName[/\d+\/?$/]).nil? ? Float::INFINITY : suffix.to_i }
     .sort.map(&:last).flatten
end

The first line selects all subdirectories matching the pattern. The second line selects only the ones which are empty. The third groups by the numeric suffix (non-numeric suffixes will be grouped under Float::INFINITY, so they will be last). The last line sorts by the numeric suffix of the directories, and returns the directory names.