Shorten code that gathers files from directories

Question

I have the following method in Ruby that appears to be just one line too long according to Ruby style guides. (Specifically: Rubocop tells me that my method has too many lines.)

Granted, I'm only one over the apparent allowed "limit" but in the interests of seeing if the style guides are realistic, I'm trying to adhere to them. Here is the method:

def spec_context
  spec_repo.map do |path|
    files_to_sort = []

    if File.directory?(path)
      spec_type.each do |type|
        files_to_sort << Dir["#{path}/**/*.#{type}"].sort
      end

      files_to_sort
    else
      []
    end
  end.flatten.uniq
end

The method essentially looks for any paths passed in, checks if there are files in those paths, and, if so, adds those files to an array (files_to_sort). If the path provided does not exist, an empty array is returned.

This is about as good as I can get this method. What I don't see is any way that I can shorten that to the Ruby style guidelines (10 lines) and still keep the contents nicely expressive.

Responding to a comment below, I wasn't clear at all about what spec_repo and spec_type are. Those are methods that get called which currently just return arrays:

def spec_repo
  %w(specs features stories)
end

def spec_type
  %w(spec feature story)
end

(I say "currently" because eventually the logic in those methods will be reading from a configuration setting.)

So the idea is to look over each directory that is listed as a repo and then, for each such directory, loop over each possible file type. (The file extensions being .spec, .feature, or .story.) If any files of the spec_type are found in any of the spec_repo directories, those are stored in the files_to_sort.

Answer 1

Unless I'm misinterpreting your code, you should be able to get by with something like this:

def spec_context
  directories = spec_repo.select { |path| File.directory?(path) }
  files = directories.map do |dir|
    spec_type.map { |type| Dir["#{dir}/**/*.#{type}"].sort }
  end
  files.flatten.uniq
end

That should be well within the line-limit (you could actually write that in one line, but that'd be kinda' annoying to read).

Point is, we filter to get the directories first, rather than in an if...else, and we map rather than push stuff to an array.

By the way, the title of question is a little confusing. Yes, the code "sorts file names", but only within their directory structure. Perhaps you want to put that sort at the very end, after the flatten, when you got all the files in a single array?
Also, as far as I can tell, the call to uniq isn't actually necessary; the files you'll find will either be in different directories or have different types, so there shouldn't be any repeats. But, if the addition of user configuration might cause repeats, I'd suggest handling that filtering elsewhere, so spec_repo and spec_type are sure to always return arrays with unique items only. Again, you should be able to get rid of the call to uniq here. Perhaps something similar could be done to make sure the spec_repo array only contains directories (assuming that's all it's supposed to contain) so we avoid doing that filtering in the method above.

Speaking of those methods, I'd suggest plural names for those two methods, since they return arrays. Similarly, the name spec_context doesn't quite communicate that the method returns an array of file names. It might be OK within the jargon of your code, but taken out of context, I might prefer something like spec_files as it's slightly more descriptive.

You also seem to be prefixing all your methods (the ones shown here, at least) with spec_. You might consider creating another class for this logic, since prefixing method names is indicative of methods that should be separately encapsulated.

---

Responding to the questions posed in the comments:

re method names: No, a method name doesn't need to follow any conventions or declare at its return type. In fact, if you overdo it, the code just becomes weirdly stilted. E.g. a method that returns a path ought to just be called path, not path_string - it's just redundant. Besides, Ruby doesn't have strict types, so trying to pseudo-declare types is going against the grain.

But hinting at what something is is very useful, and helps self-document the code. For example, I usually use plural for array variables or methods that return array. Also, somewhat particular to Ruby, a method call without arguments looks like any other variable, since we don't need to add () at the end. For instance, a line like directories.map ... looks the same whether directories is a local variable or a method. So if you were to refactor the code above and extract the first line into a separate method, you might as well call that method directories, since, hey, the name's worked well so far and it's pretty descriptive:

def directories # same name, but now a method
  spec_repo.select { |path| File.directory?(path) }
end

def spec_context # besides removing a line, nothing needs to change here. Yay!
  files = directories.map do |dir| # looks just like before
    spec_type.map { |type| Dir["#{dir}/**/*.#{type}"].sort }
  end
  files.flatten.uniq
end

You could go with directory_paths to further indicate that it's paths we're talking about, but... meh, not necessary. The simpler the better in my opinion. That said, it's always a judgment call.

You certainly shouldn't go for something like setup_spec_context; if it's already in a module called Setup, it's totally redundant to repeat that in the individual method names. For instance, the uniq method isn't called array_uniq, because, well, we know it's an array already.

re the word context: From the code and what you're describing, it sounds like a "context" in your case is actually more than just a list of files. It's also the directories to search, and the file types to search for (if not more). So you may want something like a Context class. Then you could have two instances of that class, spec and execution, each containing all the relevant stuff (spec.files, spec.types, spec.directories, etc.). Basic OOP encapsulation; modelling the notion of a "context".

If you find yourself prefixing the names of a bunch of methods, what you actually want is likely a class to contain those methods, so they're properly encapsulated.

Answer 2

I believe Dir can do most of the work for you. The trick is to use {} in your glob to find the correct directories. So,

Dir["{specs,features,stories}/**/*.{spec,feature,story}"]

will get all files with your extensions in the paths you need.

If none are found then nil is returned. Just added to_a makes it return an empty array when no results are found.

The resulting method is:

def spec_context
  directories = spec_repo.join(',')
  extensions = spec_type.join(',')

  Dir["{#{directories}}/**/*.{#{extensions}}"].to_a.sort
end

I am not sure if pulling the variables out is better or worse. It depends on the reuse of a comma delimited list of spec_repo and spec_type.