what do you think of this code?

[Ben Aurel]

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

 

[Frederick Cheung]

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

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

3. 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 '..'

 

[Stefano Crocco]

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

Yes (in my opinion, of course).

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

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

 

[Ben Aurel]

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

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

Yes (in my opinion, of course).

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

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

 

[James Gray]

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 ath
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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 Gray II

 

[Frederick Cheung]

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.

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

 

[Ben Aurel]

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

 

[Frederick Cheung]

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

 

[Waldemar Dick]

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

 

[Trans]

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

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
=A0 1 #!/usr/bin/env ruby
=A0 2 #
=A0 3 # The Unix tool 'ls' implemented in ruby
=A0 4 #
=A0 5 class Ls
=A0 6 =A0 =A0 =A0 =A0 attr_accessor arent_dir
=A0 7 =A0 =A0 =A0 =A0 def initialize()
=A0 8 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 puts "Help: ls - list directories a= nd files "
=A0 9 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 @parent_dir =3D parent_dir
=A010
=A011 =A0 =A0 =A0 =A0 end
=A012 =A0 =A0 =A0 =A0 def get_dir(dir)
=A013 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 path =3D dir
=A014 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if File.exists?(path) && File.direc= tory?(path) && path!=3Dnil
=A015 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return path
=A016 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
=A017 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 puts "Directory doe= sn't exists"
=A018 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 exit 1
=A019 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 end
=A020 =A0 =A0 =A0 =A0 end
=A021
=A022 =A0 =A0 =A0 =A0 def list_all(dir)
=A023 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 Dir.foreach(dir) do |entry|
=A024 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if entry !=3D "." |= | entry !=3D ".."
=A025 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 put= s entry
=A026 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 end
=A027 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 end
=A028
=A029 =A0 =A0 =A0 =A0 end
=A030 end
=A031
=A032 lister =3D Ls.new()
=A033
=A034 dir =3D ARGV.shift
=A035 if dir!=3Dnil
=A036 =A0 =A0 =A0 =A0 lister.list_all(lister.get_dir(dir))
=A037 else
=A038 =A0 =A0 =A0 =A0 puts "no Argument - define path"
=A039 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.

2. 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)

3. line 24 doesn't work '.' and '..' are not. How could I do that with
regular expressions?

not what? looks okay.

T.

 

[Emmanuel Oga]

3. line 24 doesn't work '.' and '..' are not. How could I do that with

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 Oga
ELC Technologies (TM)
1921 State Street
Santa Barbara, CA 93101
(e-mail address removed)

(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

 

[David A. Black]

Hi --

Here are a few more comments.

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)

Conversion to conventional indentation for free

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.

6 @path = path
7 else
8 raise ArgumentError, "Directory doesn't exists"
9 end
10 end
11 attr_accessor ath
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

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

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

You can just do:

puts ls.list_all_array(dir)


David

 

[Ben Aurel]

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

Hi --

Here are a few more comments.

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)

Conversion to conventional indentation for free

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.

6 @path = path
7 else
8 raise ArgumentError, "Directory doesn't exists"
9 end
10 end
11 attr_accessor ath
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

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

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

You can just do:

puts ls.list_all_array(dir)


David

--
Rails training from David A. Black and Ruby Power and Light:
* Advancing With Rails August 18-21 Edison, NJ
* Co-taught by D.A. Black and Erik Kastner
See http://www.rubypal.com for details and updates!

 

[Martin DeMello]

#!/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

 

[Ben Aurel]

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)

 

[Adam Shelly]

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