3
\$\begingroup\$

Actually, I just have no ideas.

How can I make breaklines to make it more readable?

Can anyone suggest ways on improving this?

module TestingDataLoad
  def get_files_from(search_target)

    Dir[search_target].each do | d |
      testing_files={}
      if File.directory? d
        Dir.glob("#{d}/*json") do |file|
          if File.basename(file,".json") =~ /(\w+)(_\d+-\d+)/
            event_type = $1.to_sym
            if testing_files.has_key? event_type
              testing_files[event_type] << file
            else
              testing_files[event_type] = [file]
            end
          elsif File.basename(file,".json") =~ /(\w+)/
            event_type = $1.to_sym
            testing_files[event_type] = [file]
          end
        end
      end

      return testing_files
    end
  end
\$\endgroup\$
  • \$\begingroup\$ When checking a string against different regexes, it is sometimes handy to use a case statement. For example, case str; when /ab/;...; when /cd/, /ef/;...end. This works because case` uses === for selections. \$\endgroup\$ – Cary Swoveland Jun 20 '14 at 6:20
  • \$\begingroup\$ Rolled back Rev 4 → 2. You have several options for follow-ups. \$\endgroup\$ – 200_success Jun 20 '14 at 7:00
4
\$\begingroup\$

A couple of things I noticed:

  • Dir[...] and Dir.glob(...) are synonymous, but you're using both forms?
  • Speaking of, glob accepts fairly complex patterns, which could help narrow the search
  • Skip the testing_files.has_key? check; just use testing_files[event_type] ||= []
  • The explicit return makes little sense. You're looping through the globbed dir paths, but your explicit return means that only the first will ever be checked. Maybe there's only supposed to be one path, but that depends on what search_target is, which you don't know.
  • Your method will return nil, an empty hash or a populated hash. Limit the number of possible return values.

Mostly though, it seems like the filename-branching is unnecessary. In both cases you add grab the \w+ part of the name, and use it as a hash key for an array. The only difference is that if the filename has numbers in it, it'll add the file to the array, otherwise it'll replace the array with a new 1-element array.

I'd say treat all files the same, and append them to the array. Don't ever replace the array. As far as I can figure, your method is entirely dependent on the platform/filesystem you're running it on, since glob doesn't dictate the order in which files are returned.

So given 3 files in the following order:

foobar.json
foobar_1-1.json
foobar_1-2.json

your method will return all three. But if the order's reversed, it'll only return "foobar.json". Or, if it's somehow mixed, it might return "foobar.json" and "foobar_1-1.json", but not the 3rd file.

So instead of risking inconsistent behavior, I'd suggest sticking to 1 way of treating the files, regardless of whether they're numbered or not.

def get_files_from(search_target)
  files = {}

  Dir[search_target].each do | d |
    next unless File.directory?(d) # skip if d isn't a dir

    Dir["#{d}/*.json"] do |file|
      event = File.basename(file)[/^\w+/]
      testing_files[event] ||= []
      testing_files[event] << file
    end
  end

  files
end

If you want to be more exacting, you can do this instead

Dir["#{d}/*.json"].select { |file| File.basename(file, '.json') =~ /^\w+(_\d+-\d+)?$/ }.each do |file|
  ...
end
|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

I think that your root problem is that your function is more complex than it needs to be. If you eliminate some of the nesting, then your code will prettify itself naturally.

Observations:

  • Dir[search_target].each do |d| … return testing_files; end is not much of a loop. It will always return testing_files on the first iteration. So, that's one level of nesting that you can remove.

    Incidentally, search_target may be misleadingly named, if you're always using just its first element.

  • if File.directory? d is a superfluous check, since the immediately following Dir.glob("#{d}/*json") will just produce no results if d is not a directory or a symlink to a directory.
  • Dir.glob("#{d}/*json") is somewhat redundant with your subsequent regular expression matching. It's also dangerous to use a non-fixed glob pattern: if the directory name happens to contain * or ?, then those characters will be globbed.
  • The two branches of if File.basename(…) =~ … elsif File.basename(…) =~ … end have essentially the same bodies, and should therefore be combined. I believe that the intention is to append to an array if the key exists, or append to a new array if necessary. There's a less repetitive idiom for that.
  • You could just incorporate File.basename(file, ".json") into the regular expression test.

Here's what I came up with, which is a bit shorter and reduces nesting by two levels:

def get_files_from(search_target)
  testing_files = {}

  begin
    dir = Dir.new(Dir.glob(search_target).first)
    dir.each do |filename|
      if filename =~ /(\w+)(_\d+-\d+).*\.json\Z/ || filename =~ /(\w+).*\.json\Z/
        event_type = $1.to_sym
        (testing_files[event_type] ||= []) << File.join(dir, filename)
      end
    end
  rescue Errno::ENOTDIR
    # Empty result
  end

  return testing_files
end
|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

I just have a few suggestions:

  • when one has ifs without else's, I find it's often more readable to use next if... instead, to reduce the number of nested clauses by one. This is a style issue; not everyone would agree.

  • I'm a big fan of case statements, especially when one wants to discriminate based on an object's class, a string is being matched on different regex patterns or multiple conditions result in the same action. Then first two of these make use of the fact that case uses === rather than ==. That allows case object when String... and case "my string" when /str/ .....

  • Use (testing_files[event_type] ||= []) << file to avoid the need to check whether the hash has a key equal to the value of event_type.

Below I've suggested how pcc's code might be altered. The edit history of the question shows that pcc came up with virtually the same thing. In fact, before pcc's question was rolled back, I left a comment saying how I liked that factoring, in part because it was virtually the same as what I had come up with independently. (My comment has since vanished.)

module TestingDataLoad
  def get_files_from(search_target)
    Dir[search_target].each do | d |
    next unless File.directory? d
    testing_files={}
    Dir.glob("#{d}/*json") do |file|
      case File.basename(file,".json")
      when /(\w+)(_\d+-\d+)/
        (testing_files[event_type] ||= []) << file
      when /(\w+)/
        testing_files[$1.to_sym] = [file]
      end
    end
    return testing_files
  end
end
|improve this answer|||||
\$\endgroup\$
-2
\$\begingroup\$

I have no idea what your code does--never used ruby or json--but if you're going for readability, space your if statements:

// Pseudo Code(!!):
if expression
    if expression
        result
    end

    if expression
        result
    end
end

That works, or you can use the following method:

if expression // -- 1 --
    if expression // -- 2 --
        result
    end // -- 2 --

    if expression // -- 3 --
        result
    end // -- 3 --
end // -- 1 --

Doing this allows you to recognize the beginning and end of each statement as well as the result within each statement.

|improve this answer|||||
\$\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.