What do you think of this code?

Ruby
1

hi
In order to learn ruby I’d like to implement some simple unix tools in
ruby. I thought I beginn with ‘ls’:

  1 #!/usr/bin/env ruby
  2 #
  3 # The Unix tool 'ls' implemented in ruby
  4 #
  5 class Ls
  6         attr_accessor :parent_dir
  7         def initialize()
  8                 puts "Help: ls - list directories and files "
  9                 @parent_dir = parent_dir
 10
 11         end
 12         def get_dir(dir)
 13                 path = dir
 14                 if File.exists?(path) && File.directory?(path) && 
path!=nil
 15                         return path
 16                 else
 17                         puts "Directory doesn't exists"
 18                         exit 1
 19                 end
 20         end
 21
 22         def list_all(dir)
 23                 Dir.foreach(dir) do |entry|
 24                         if entry != "." || entry != ".."
 25                                 puts entry
 26                         end
 27                 end
 28
 29         end
 30 end
 31
 32 lister = Ls.new()
 33
 34 dir = ARGV.shift
 35 if dir!=nil
 36         lister.list_all(lister.get_dir(dir))
 37 else
 38         puts "no Argument - define path"
 39 end
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

there are a few things that I'm unsure about:

1. the application structure as a whole. Is it ok to test command line
argument outside of a class?
2. the structure of the class itself. Is the constructor
(initialization) ok that way?
3. line 24 doesn't work '.' and '..' are not. How could I do that with
regular expressions?


thanks in advance for your help and opinion
ben
2

On 12 Aug 2008, at 13:12, Ben A. wrote:

there are a few things that I’m unsure about:

  1. the application structure as a whole. Is it ok to test command line
    argument outside of a class?

Seems entirely sensible. were you to reuse this class elsewhere you
wouldn’t give two hoots about the fiddling with ARGV

  1. the structure of the class itself. Is the constructor
    (initialization) ok that way?

Your constructor doesn’t seem to be doing much @parent_dir will only
ever be set to nil.
It would make more sense to me if most of the code in get_dir was
actually in your constructor, with the list_all method then taking no
arguments and listing whatever the relevant instance variable points at.
usage wide your code would then be Ls.new(dir).list_all

  1. line 24 doesn’t work ‘.’ and ‘…’ are not. How could I do that with
    regular expressions?

it’s because you need if entry != “.” && entry != “…”. Every string
is going to be not equal to at least one of ‘.’ or ‘…’

3

On Tuesday 12 August 2008, Ben A. wrote:

6 attr_accessor :parent_dir
17 puts “Directory doesn’t exists”
28
39 end


there are a few things that I'm unsure about:

1. the application structure as a whole. Is it ok to test command line
argument outside of a class?

Yes (in my opinion, of course).

  1. the structure of the class itself. Is the constructor
    (initialization) ok that way?

It depends. Do you want the help message to be displayed every time the
application is run? If not, then you shouldn’t put it into the
constructor,
but use a command line option. For example:

dir = ARGV.shift
if dir == [’–help’] or dir == [’-h’]
puts "Help: ls - list directories and files "
elsif dir then lister.list_all(lister.get_dir(dir))
else puts “no Argument - define path”
end

By the way, note that you don’t need to write

elsif dir != nil

since everything except nil and false evaluates to true, you can just
write

elsif dir

  1. line 24 doesn’t work ‘.’ and ‘…’ are not. How could I do that with
    regular expressions?

If you don’t want to display the ‘.’ and ‘…’ entries, you need to use
&&, not
||. This is because, when entry is ‘.’, entry != ‘.’ will be false, but
entry != ‘…’ will be true, so the whole expression will be true (‘or’
is true
if at least one operand is true). If you replace || with &&, it will
work.

Stefano

4

On Aug 12, 2008, at 8:29 AM, Ben A. wrote:

11 exit 1
12 end
13 end
14 attr_accessor :path

Does this look OK to you?

I like this a lot less. Let’s say I want to build a GUI version of
the command. I can’t use your class now, because it prints text
messages and exits when I should probably be showing an error dialog.
That makes your class less generally useful, I think.

James Edward G. II

5

On 12 Aug 2008, at 14:35, James G. wrote:

9 else
messages and exits when I should probably be showing an error
dialog. That makes your class less generally useful, I think.

Yup. Instead of that puts and call to exit I’d raise an appropriate
exception (eg ArgumentError)

Your command line harness for this class can always rescue this and
print the message.
Fred

6

this is my solution to your inputs

1.) better errorhandling

2.) no more string methods in the class.

  1 #!/usr/bin/env ruby
  2 # The Unix tool 'ls' implemented in ruby
  3 class Ls
  4         def initialize(path)
  5                 if File.exists?(path) && File.directory?(path) && 
path!=nil
  6                         @path = path
  7                 else
  8                         raise ArgumentError, "Directory doesn't 
exists"
  9                 end
 10         end
 11         attr_accessor :path
 12
 13         def list_all_array(dir)
 14                 files_n_dirs = Array.new
 15                 Dir.foreach(path) do |entry|
 16                         if entry != "." &&  entry != ".."
 17                                 files_n_dirs << entry
 18                         end
 19                 end
 20                 return files_n_dirs
 21         end
 22 end
 23
 24 dir = ARGV.shift
 25 if dir == ['--help'] or dir == ['-h']
 26         puts "Help: ls - list directories and files "
 27 elsif dir
 28         begin
 29                 ls = Ls.new(dir)
 30                 ls.list_all_array(dir).each do |i|
 31                         puts i
 32                 end
 33
 34         rescue ArgumentError => e
 35                 puts e.message
 36         end
 37 else puts "no Argument - define path"
 38 end

Like it better now?

On Tue, Aug 12, 2008 at 9:46 AM, Frederick C.

7

thanks a lot for qour hints. I made a few changes based on your tips.

1.) the constructor:

  6         def initialize(path)
  7                 if File.exists?(path) && File.directory?(path) && 
path!=nil
  8                         @path = path
  9                 else
 10                         puts "Directory doesn't exists"
 11                         exit 1
 12                 end
 13         end
 14         attr_accessor :path

Does this look OK to you?

2.) program start, object creation

27 dir = ARGV.shift
 28 if dir == ['--help'] or dir == ['-h']
 29         puts "Help: ls - list directories and files "
 30 elsif dir then Ls.new(dir).list_all(dir)
 31 else puts "no Argument - define path"
 32 end

line 30 is a bit ugly, but I wanted an transparent method signature
for list_all method.
The other thing I thought was to put a help message into the list_all
method in case of more options and more methods like list_files

ben

8

Hi,

just a short notice: On line 11, you create attribute accessors for
@path, i.e. a reader and a writer for @path.
That way, as a user of your class, I can set @path, without
the checks made in initialize(path).

ls = Ls.new(dir)
ls.path=nil
ls.list_all_array -> will not work as expected

So, I would either move
the checks to a “path=” method or remove the write accessor for
@path.

Greetings,

Waldemar

9

On Aug 12, 8:12 am, “Ben A.” [email protected] wrote:

6 attr_accessor :parent_dir
17 puts “Directory doesn’t exists”
28
39 end


there are a few things that I'm unsure about:

1. the application structure as a whole. Is it ok to test command line
argument outside of a class?

It’s always better to do everything in a class.

  1. the structure of the class itself. Is the constructor
    (initialization) ok that way?

Did you test it? Where is the local variable parent_dir coming form?
Did you mean

def intialize(parent_dir)

  1. line 24 doesn’t work ‘.’ and ‘…’ are not. How could I do that with
    regular expressions?

not what? looks okay.

T.

10

On 12 Aug 2008, at 15:48, Ben A. wrote:

Like it better now?

(for me at least) the point of passing dir to initialize was that
list_all would use @path instead of you passing a parameter.

Fred

11
  1. line 24 doesn’t work ‘.’ and ‘…’ are not. How could I do that with
    regular expressions?

not what? looks okay.

I’m guessing maybe he used a regex like:

if path =~ /…?/

but you need to escape the dots inside regexes if you want to catch
the dot character

if path =~ /..?/

You will also need to check begin and end of the file name using the
anchors:

if path =~ /^..?$/

Although I like regexes very much, maybe it is a bit overkill to use
them in this case.

path == “.” || path == “…”

looks good for this check.

–
Emmanuel O.
ELC Technologies ™
1921 State Street
Santa Barbara, CA 93101
[email protected]

(866)863-7365 Tel
(866)893-1902 Fax

+44 (0) 20 7504 1346 Tel - London Office
+44 (0) 20 7504 1347 Fax - London Office

http://www.elctech.com

12

hi
I’m hugely impressed by your helpfull comments. Thanks alot. I’ve
tried to follow your recommondations and made again a new version. I
thinks its a cool little program, and I think I post it somewhere with
comments. As reminder for me and maybe valuable information for others

#!/usr/bin/env ruby
# The Unix tool 'ls' implemented in ruby
class Ls
  @path = ""
  def initialize(path)
    if File.exists?(path) && File.directory?(path)
      @path = path
    else
      raise ArgumentError, "Directory doesn't exists"
    end
  end

  def list_all()
    files_n_dirs = Array.new
    Dir.foreach(@path) do |entry|
      next if ['.','..'].include?(entry)
      files_n_dirs << entry
    end
    return files_n_dirs
  end
end

dir = ARGV.shift
if dir == ['--help'] or dir == ['-h']
  puts "Help: ls - list directories and files "
elsif dir
  begin
    puts Ls.new(dir).list_all()
  rescue ArgumentError => e
    puts e.message
  end
else puts "no Argument - define path"
end
13

Hi –

Here are a few more comments.

On Tue, 12 Aug 2008, Ben A. wrote:

4 def initialize(path)
Conversion to conventional indentation for free :slight_smile:

5 if File.exists?(path) && File.directory?(path) && path!=nil

File.exists?(nil) will raise a TypeError, so you’ll never get to the
final test. If you make it through the first two tests, the third test
is not necessary. So you can get rid of it, and decide whether you
want the nil error to trickle up. The path argument is required,
though, so it’s not going to be nil just because someone forgot it; it
will only be nil if someone passes in nil.

You might even consider just storing the path, and then letting
whatever happens later happen – like someone trying to access a
non-existent or restricted directory.

16 if entry != “.” && entry != “…”
17 files_n_dirs << entry
18 end
19 end

A slightly more straightforward way to handle that might be:
next if [’.’, ‘…’].include?(entry)
files_n_dirs << entry

30 ls.list_all_array(dir).each do |i|
31 puts i
32 end

You can just do:

puts ls.list_all_array(dir)

David

14

On Tue, Aug 12, 2008 at 4:37 PM, Ben A. [email protected] wrote:

#!/usr/bin/env ruby

The Unix tool ‘ls’ implemented in ruby

class Ls
@path = “”

This is not doing what you think it does - when @path = “” is
executed, you are in the context of the class object Ls, which will
acquire a “class instance variable” @path, which is then never used
(or, indeed, accessible). If your intent was to have a default path of
“” if none was given, you need the next line to be

   def initialize(path)

def initialize(path = “”)

but since immediately after you are checking the path

           if File.exists?(path) && File.directory?(path)

you might as well let it be nil if not supplied, and omit the default
altogether.

martin

15

On 8/13/08, Ben A. [email protected] wrote:

thanks for the input. Here are my changes and my reasoning about it:

  • like the unix ‘ls’ command the default parameter is ‘.’

  • I demand ‘path’ as a initialize argument but I wont set a default
    value because creating a new instance with new() is wrong and should
    raise an error.

This doesn’t make sense to me. Why shouldn’t you be able to create a
new instance without an argument and have it default to the current
directory?
System ‘ls’ defaults to the current directory. I think I shoud be
able to type “ls.rb” without arguments and get the current directory
list, not a “No Argument” warning.
To make it work like that, I would use a default argument,

4 def initialize(path = ‘.’)
5 @path = path.empty? ? ‘.’ : path
8 if File.exists?(@path)
…

and remove the ‘elsif dir’ clause before the begin block at the end of
your script.

Also, a standard idiom for files that can be either run from the
command line or required is to put this test around the command line
handling part:

if FILE == $0
end

That condition will only be true if this file is being executed
directly, not when it is 'require’d.

-Adam

16

This is not doing what you think it does - when @path = “” …

thanks for the input. Here are my changes and my reasoning about it:

  • like the unix ‘ls’ command the default parameter is ‘.’

  • I demand ‘path’ as a initialize argument but I wont set a default
    value because creating a new instance with new() is wrong and should
    raise an error.

  • I try to catch all the important exceptions

  4         def initialize(path)
  5                 @path = path
  6                 if @path == "" || @path == nil
  7                         @path = "."
  8                 elsif File.exists?(@path)
  9                         raise ArgumentError, "This is a file not a
directory"
 10                 elsif  !File.directory?(@path)
 11                         raise ArgumentError, "Directory doesn't 
exists"
 12                 end
 13         end
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I have to admit that I have a bit a hard time to grock the usage of
the different ruby class and object variables (local vars, global,
instance, class). I hope correct this time (line 5)