From: David Vallner <david@...>
Date: 2006-02-20T03:24:40+09:00
Subject: Re: Classes and OO design - help

Dňa Nedeľa 19 Február 2006 17:19 Tony Mobily napísal:
> I feel the need to start this email with an apology. I am a terrible
> programmer.
>

Meh. So am I, and I do it full-time ;) (For given values of full.)

> So, here I am. I have a file system, with the following contents:
>
> [...]/A/
> [...]/B/
> [...]/C/
> [...]/D
>
> Under /A/, there is:
>
> /A/abcexample@iinet.net.au/
>
> In each directory, there are the following files:
>
> name
> surname
> password
> state
> unconfirmed_flag
> moderator_flag <-- The file can exist (flag = true ) or not exist
> (flag = false)
>
> I know there are much better ways of doing so, and that this approach
> creates a lot of tiny files, but unfortunately, at least for now, I
> am stuck with it.
> If you are curious, this is how the subscribers' information is
> stored for Free Software Magazine (http://www.freesoftwaremagazine.com).
>
> I wrote (see: I didn't write "designed"! :-) ) a class to access this
> information on the file system.
>
> Here it is:
>
> ---------------------------------------------------
> #!/usr/local/bin/ruby -w
>
> class Subscribers

Subscriber is probably a better name. Start creating single entities of your 
application.

>
>          # Get the config file's contents
>          #
>          begin
>                  @@config_data_dir=IO.read("#{ENV['HOME']}/.subs/
> data_dir")
>                  @@config_data_dir.chomp!
>          rescue SystemCallError
>                  STDERR.puts("WARNING! Can't find the config file!");
>                  @@config_data_dir=""
>          end
>
>          def initialize()
>                  @good_state=false # This might be completely useless
>                  @first_letter=""
>                  @full_path=""
>                  @attr_values={}
>          end
>
>
>          # This is the same as initialize... for now!
>          # (who knows...)
>          #
>          def de_initialize
>                  @good_state=false
>                  @first_letter=""
>                  @full_path=""
>                  @attr_values={}
>          end
>

A deinitialize method seems completely useless to me. If you want an object (a 
subscriber record) to stop existing, delete it from disk, remove it from any 
listings or caches you store on disk separately. If it's a persistent 
application, drop the old object representing it from memory too. Clobber and 
forget, you don't need to cater to an invalid object that's not being used / 
doing anything anymore in the application.

You might want to make some sort of #delete method for the abovementioned 
"housekeeping".

>
>          # This just checks that the directory actually
>          # exists. It creates a "link"
>          #
>          def link_to_fs(email)
>
>                  # Gets the person's information
>                  #
>                  @good_state=true
>                  @first_letter=email[0,1].upcase
>                  @full_path=@@config_data_dir+"/current/"+
>                                  @first_letter+"/"+email+"/"
>                  @attr_values={}
>                  @attr_values[:email]=email

Use accessors (see below) and instance variables for this. A big hash for all 
attributes of the object is very bad style IMO.

E.g. you'd have in the class definition:

	attr_reader :good_state

	def first_letter
		email[0, 1].upcase
	end

	def full_path
		# insert that string addition thingy I'm too cheap to copy / paste
	end

	attr_accessor :email

and change the method body to:
	
	def link_to_fs(email)
		@good_state = true
		self.email = email
	end

>
>                  # Hang on: if the file doesn't exist, undo everything
>                  #
>                  if ! File.exist?(@full_path)
>                          de_initialize()
>                          return false
>                  end
>

Not quite good. First find out if you can create a new object - then proceed 
if you can. I'd move the code of #link_to_fs into #initialize myself in this 
case. You can throw an exception if the record creation fails, but I'd use a 
different approach.

>                  true
>
>          end
>

Replace your own catch-all getters and setters with proper accessors for the 
subscriber attributes - this data access class becomes a bit more 
self-descriptive.

You can define your own accessors per these universal getters and setters, but 
I'd tag them as private methods - they seem a bit bug-prone to be part of the 
interface.

E.g.:

	def premium?
		get_flag(:premium_flag)
	end

	def premium=(value)
		set_flag(:premium_flag, value)
	end

>          def get_flag(flag)
>                  File.exist?(@full_path+flag.to_s)
>          end
>
>          def get_field(field)
>
>                  # Email is special: it's not in a file
>                  #
>                  if( field == :email)
>                          return @attr_values[:email]
>                  end
>
>                  # Either return the existing @attr_values[field], or
>                  # (if it's nil) reads it from the file system.
>                  # CACHE!
>                  #
>                  begin
>                          @attr_values[field]||=IO::read(@full_path
> +field.to_s)

You could cache this way with ordinary instance attributes. Unless you expect 
the subscriber attributes (not their values) to change very often and 
unexpectedly. It also gives a bit more exact behavior.

>                  rescue SystemCallError
>                          nil
>                  end
>
>          end
>
>
>          def set_field(field,value)
>
>                  # Email cannot be set
>                  #
>                  if(field == :email)
>                          return nil
>                  end
>
>
>                  # Open the file
>                  #
>                  begin
>                          ios=File::open(@full_path+field.to_s,"w")
>                  rescue SystemCallError
>                          return nil
>                  end
>
>                  # Set the value to nil. This is to reflect the
>                  # "real" state of the variable (the file has just been
>                  # cleared up by the previous call)
>                  #
>                  @attr_values[field]=nil
>
>                  begin
>                          ios.print(value)
>                  rescue SystemCallError
>                          ios.close
>                          return nil
>                  end
>
>                  # OK, it worked: assign the new value
>                  #
>                  ios.close
>                  @attr_values[field]=value
>          end
>
>          def set_flag(flag,value)
>                  # NOT DONE YET. Ask the mailing list if I wrote a pile
>                  # of crap first... :-|
>
>
>          end
>
> 	# TODO: methods to create a new entry (just creates the directory),
> 	# methods to get ALL of the parameters in one go in a Hash, etc.
>
> end
>
>
> a_subscriber=Subscribers.new()
> puts a_subscriber.link_to_fs("merc2@mobily.com")
> puts a_subscriber.get_field(:email)
> puts a_subscriber.get_flag(:premium_flag)
> puts a_subscriber.get_flag(:moderator_flag)
> puts "OK:"
> puts a_subscriber.set_field(:name,"BLAH!!!")
> puts a_subscriber.get_field(:name)
>

The above would change to something like with what I have in mind:

	# Presuming the record exists.
	sub = Subscriber.load("merc2@mobily.com")
	puts sub.email
	puts sub.premium?
	puts sub.moderator?
	puts "OK:"
	puts sub.name = "BLAH!!!"
	puts sub.name
	sub.save # To put the changes to disk.

> ----------------------------------------------------------
>
> Now... here are my questions:
>
> * Is it sane for the class to set the class variable
> @@config_data_dir? @@config_data_dir=IO.read("#{ENV['HOME']}/.subs/
> data_dir")
>

For a simple enough application, this seems like a perfectly good 
organization. Bonus points for tying configuration to the model.

> * Would it make more sense to create a new person with the
> method ::new? In this case, what would you call the method to access
> an existing person?
>

::load? The path where a subscriber's data rests is directly computable from 
the e-mail adress.

The solution I'd probably go for is:
	- ::new would create a subscriber record in memory.
	- ::load would load a record from disk, or return nil if there is no record
		for a given e-mail. Actually, it wouldn't load any data, just some "proxy"
		object that would load the data from disk when needed and keep
		(cache) them in memory.
	- ::save would store a (changed) object back into the respective files. You 
		could have ::save take an optional flag to explicitly allow / deny the
		creation of new records - to prevent duplicate records clobbering each 
		other.


> * Is this solution OO sound?
>

Definately, Your Subscriber object is a data access object, which uses the 
filesystem as the underlying "database". Very commonly done / used piece of 
code, I'd say.

> I can't think of a way, in Ruby, to do access the information like
> this: a_subscriber.get_field[:name] or a_subscriber.get_flag
> [:moderator_flag]. This is the tricky part, and that's where my lack
> of understanding of OOP shows blatantly.
>

Use instance attributes and accessors instead of the catch-all hash. It's a 
rather basic part of OO, but understandably foreign to anyone with a strict C 
background.

> a.subscriber.get_field[:name] is the equivalent of saying
> a.subscriber.get_field.[](name)
>

Mind you, you don't use this construct in the code you posted.

> This implies that get_field returns an object of some kind able to
> respond to []. If this were the way to go design-wise (which I doubt,
> but now I am confused, so...), where would such an object be created?
>

Well, in the #get_field method :P The way to do so would be returning a Hash 
with the required attributed in it. But it's much more concise OO-wise to 
have a data object respond to queries about its data directly than via a 
"middle-man" hash.

> How would it access the class information such as @@config_data_dir,
> or the subscriber's instance variable?
>

Accessors, accessors, acceessors... 

You can have Ruby generate them if you can do with the default ones that only 
read / write to instance attributes, or custom ones as the examples I've 
shown above

Even class objects have accessors:

	class Subscriber
		def self.config_data_dir
			@@config_data_dir
		end
	end

> * I am thinking about a container class for this:
> SubscribersContainer. The class would implement the method each(), so
> that I can scan through the list of people stored (creating a
> Subscriber object for iteration). Is this a sane approach?
>

Yes. This class could also store the data directory path and manage the 
lifecycle of Subscriber objects, reducing those to only data retrieval / 
caching. 

If you wanted to go extreme, you could also separate the data retrieval part 
and keep subscribers only as dumb data structured, but I'd say it would only 
be deconstructing code for the sake of deconstruction, and more confusing 
than anything else in this case.

> I am just a little scared of doing anything right now. Did I design
> it all wrong? 

Surprisingly well actually, saying you're an OO beginner. The changes I 
proposed are more tweaks and use of common idioms than horrible flaws.

> Should I have created the classes SubscribersFlags and 
> SubscribersAttributes, and have two instance variables in Subscribers
> derived from SubscribersFlags and SubscribersAttributes?
>

Overkill. The attributes and flags belong to the subscriber. They are inherent 
to a subscriber, and should stay there.

> * The most important of all: can you suggest a book or a web site
> which would help me design more decent classes? Possibly something
> that is Ruby-centric...
>

I'd say read through the Gang of Four and Refactoring, but those might be out 
of your scope. Not necessarily though, and I believe those two to be very 
important not-too-advanced OO reading. Java inside : you have been warned ;)

David Vallner