From: Stefan Schmiedl <s@...>
Date: 2002-09-23T21:19:51+09:00
Subject: Re: Criticise me

ok, here we go:

On Fri, 20 Sep 2002 00:44:00 GMT,
Matthew, Graeme <Graeme.Matthew@mercer.com> wrote:
> 
> require "C:/dev/cgi-bin/DbHighlander.rb"
> 
> class Project
> 
> 	SQL_PROJECT = "SELECT prj_id, prj_name, prj_description,
> prj_commenced, prj_completed, prj_status,prj_acc_id FROM project "
> 		     
> 	def initialize(id=nil)
> 
> 		@data = nil
> 		
> 		if defined? id
> 			
> 			db = DbHighlander.Connection
> 			qry = db.prepare(SQL_PROJECT + " WHERE prj_id =
> #{project_id} ")
> 			qry.execute
> 			@data = qry.fetch_hash
> 			qry.finish
> 			
> 		end
> 		
> 	end

By makeing the parameter id optional, it will always be defined:

irb(main):001:0> def a(b=nil)
irb(main):002:1>   if defined? b
irb(main):003:2>     puts "defined"
irb(main):004:2>   end
irb(main):005:1> end
nil
irb(main):006:0> a
defined
nil
irb(main):007:0> a(1)
defined
nil

And you might want to wrap your database connection with "begin ... ensure ... end"
to, well, ensure that it is really closed.

Right now I am not sure if you do something terribly wrong by naming
your methods with uppercase initials, but it is at least very unusual
to do so as uppercase initials usually denote constants.

> 
> 	
> 	#Class Attributes
> 	
> 	def Code
> 		return @data['prj_id']
> 	end
> 
> 	def Name(name)
> 		if defined? name then @data['prj_name'] = name end
> 		return @data['prj_name]
> 	end

there is a ' missing above, and I think you don't mean "name" below.
I guess it could be "description" which you're testing, in which case
I have two remarks:
  - name and description will always be defined or an ArgumentError
    will be raised:
    irb(main):008:0> def c(d)
    irb(main):009:1>   if defined? d
    irb(main):010:2>     puts "defined" 
    irb(main):011:2>   end
    irb(main):012:1> end
    nil
    irb(main):013:0> c(1)
    defined
    nil
    irb(main):014:0> c(nil)
    defined
    nil
    irb(main):015:0> c
    ArgumentError: wrong number of arguments(0 for 1)
            from (irb):15:in `c'
            from (irb):15
    irb(main):016:0> 
  - you could save some typing by using the following helper method
    for setting up all of your similar accessor methods. Keep DRY :-)

    irb(main):001:0> def caching_attr_reader(*symbols)
    irb(main):002:1>   symbols.each do | sym |
    irb(main):003:2*     module_eval "def #{sym}(arg) @data['prj_#{sym}'] ||= arg; end"
    irb(main):004:2>   end
    irb(main):005:1> end
    nil
    irb(main):006:0> class A
    irb(main):007:1>   def initialize() @data={}; end
    irb(main):008:1>   caching_attr_reader :one, :two, :three
    irb(main):009:1> end
    [:one, :two, :three]
    irb(main):010:0> a = A.new
    #<A:0x401c5b64 @data=[]>
    irb(main):011:0> a = A.new
    #<A:0x40213d40 @data={}>
    irb(main):012:0> a.one("hi")
    "hi"
    irb(main):013:0> a.inspect
    "#<A:0x40213d40 @data={\"prj_one\"=>\"hi\"}>"
    irb(main):014:0> 

> 
> 	def Description(description)
> 		if defined? name then @data['prj_description'] = description
> end
> 		return @data['prj_description]
> 	end
> end
> 

HTH
s.