HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
searchwithcontaintypefilesthatforprefixcertaingiven

Problem

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

  • Does the code adhere to Ruby standards and conventions?



  • Am I doing something the hard way or the "non-Ruby" way?



  • 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/
$

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#40655, answer score: 8

Revisions (0)

No revisions yet.