# How can I prettify blocks with more than 2 of each block?

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

• 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. – Cary Swoveland Jun 20 '14 at 6:20
• Rolled back Rev 4 → 2. You have several options for follow-ups. – 200_success Jun 20 '14 at 7:00

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  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


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


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.