From: Robert Klemme <bob.news@...>
Date: 2005-12-31T22:27:58+09:00
Subject: Re: Another Newb asks questions.

Daniel Harple <dharple@generalconsumption.org> wrote:
> Joseph Divelbiss wrote:
>> Thanks again all.. so that you know your efforts wern't in vain I
>> think
>> I learned a thing or two.  The rough code (still needing debugging
>> about
>> as much as I need sleep) is below.  I have a high level of confidence
>> that I will find the typo or missing end to make it work when I
>> wake up
>> in the morning.
>>
>> require 'enumerator'
>> if Dir["#{$script_dir}data/"].last != nil
>>         script_data_dir = "#{$script_dir}data/"
>> end
>> build_hash = Proc.new do |type|
>> type_hash = Hash.new
>> file.open("#{script_data_dir}#{type}.dat", "r") do |file|
>> file.each_slice(2) do |key, value|
>> type_hash[key.chomp] = value.chomp
>> end
>> end
>> $type_hash = type_hash
>> end
>> data_files=Dir["#{script_data_dir}*.dat"]

If I'm not mistaken there's a path separator missing.  You better use

data_files=Dir[File.join(script_data_dir, "*.dat")]

>> data_files.each do |name_of_file|
>> build_hash.call(file.basename(name_of_file, '*.*'))

What do you use the second parameter for?  The only reasonable value here 
seems to be '*.dat'.  Also you're trying to remove the suffix ".dat" and 
reappend it again.  Seems a bit strange to me.

>> end
>>
>> VR,
>>
>> Joseph
>>
>> --
>> Posted via http://www.ruby-forum.com/.
>
> I see two typos --
> 1) file.open should be File.open ( actually, you can leave off File
> too, so it is just open(..). See Kernel#open ).
> 2) file. basename should be File.basename

Also it's unnecessary since Dir[] always returns basenames *only*.

> Slight style suggestions --
> * The first conditional statement may be rewrote as:
>   script_data_dir = "#{$script_dir}data/" unless Dir["#{$script_dir}
> data/"].last.nil?

Even better would be to use File.directory? to test for presence of this 
directory.  And it would be even better if the absence of the directory 
would be handled properly.  As far as I can see at the moment 
script_data_dir simply remains unset and thus the current working directory 
is used.

> * {} is equivalent to Hash.new
> * Use { |x| ... } block syntax when the block has only one simple
> statement (the each_slice part)

Don't use blocks or procs unless you need them (i.e. want to encapsulate a 
piece of code and refer it from a variable).

Don't assign a global variable as a side effect from your proc / method if 
you also return it.  There's especially the problem of your code that you 
only get the last hash because all others are lost (the global is 
overwritten all the time).

Here's what I'd probably do:

require 'enumerator'

def build_hash(file)
  h = {}
  File.open(file, "r") do |io|
    io.each_slice(2) {|k,v| h[k.chomp]=v.chomp}
  end
  h
end

def build_hashes(dir='.')
  h = {}
  Dir[ File.join( dir, "*.dat") ].each do |file|
    h[ File.basename( file, ".dat" ) ] = build_hash( File.join(dir, file) )
  end
  h
end

This builds a hash of hashes.  Dunno whether that's what you want.

HTH

    robert