4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ What is spec_repo and spec_type? If they operate on something external, shouldn't they be parameters? Or are they methods on self? \$\endgroup\$ – Mark Thomas Jan 29 '14 at 18:00
  • \$\begingroup\$ Ah, good point. So the method above is encased in the following structure: module Lucid --> class Setup --> def spec_context. So the spec_repo and spec_type are currently methods in that same class that simply (for the moment) return values like %w(specs features stories) -- for spec_repo -- and %w(spec feature story) -- for spec_type. Eventually those methods will actually be reading values from an Options module. For now I just hard coded the values for demonstration purposes. \$\endgroup\$ – Jeff Nyman Jan 29 '14 at 18:02
  • \$\begingroup\$ f you are satisfied with any of the answers, you should select the one that was most helpful to you. \$\endgroup\$ – Cary Swoveland Feb 27 '14 at 20:34
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Quite interesting! Thank you for the time you spent on this. I tried your suggested approach and it certainly seems to work without issue. I agree with you about the removal of .uniq. The more I think about it, it probably has no relevance simply because every file would have to be unique within its own directory. Likewise, I agree with putting .sort along with .flatten. Thank you indeed for your help on this. \$\endgroup\$ – Jeff Nyman Jan 30 '14 at 13:02
  • \$\begingroup\$ I see your point about re-naming the spec_repo and spec_type methods to spec_repos and spec_types. Regarding the naming of spec_context, that's interesting. Does the name of a method have to indicate the type it returns? Is that considered good practice? The reason I call it spec_context is because my app has two different contexts: spec files and execution files. Those files, together, make up a context for other actions. That was why I named the method as I did. I can certainly see your point, however, about that not indicating the kind of context (i.e., files) that is being returned. \$\endgroup\$ – Jeff Nyman Jan 30 '14 at 13:04
  • \$\begingroup\$ You beat me to it. Very nice answer. \$\endgroup\$ – Mark Thomas Jan 30 '14 at 13:06
  • \$\begingroup\$ Finally, regarding the naming of the methods (with prefix spec_): these methods are all in a module called Lucid, which contains a class called Setup. The reason I put these methods in Setup was because the execution and spec context is first setup when the app starts. Would naming the methods setup_spec_files (or setup_spec_context) be more indicative? It's not really setting up the files, though, so much as setting up the context so that it can pass that context (which happens to be a set of files) to a Loader class. \$\endgroup\$ – Jeff Nyman Jan 30 '14 at 13:07
  • \$\begingroup\$ @JeffNyman Firstly, you're welcome. To answer your questions, I added a bunch of stuff to my answer above \$\endgroup\$ – Flambino Jan 30 '14 at 19:49
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.