Re: Classes and OO design - help

Hi David,

I am not entirely sure about the list's etiquette. I hope it's OK to  
reply to you _and_ to the list (I imagine somebody in the future  
might be very interested in this discussion!)

First of all: _THANK YOU_ David!
I think I'm getting there...

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

Yeah right, I know people like you :-D

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

OK.

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

OK.

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

OK. Is there a way of "forcing" the deallocation of an object?

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

OK, done.

> 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

Oh... OK. I was thinking about performance. This way, Ruby has to  
recalculate the string all the time.
However, I can see that this should be the way to go...

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

OK.

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

OK, done.

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

Done.

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

OK, true.

> E.g.:
>
> 	def premium?
> 		get_flag(:premium_flag)
> 	end
>
> 	def premium=(value)
> 		set_flag(:premium_flag, value)
> 	end

Yep.

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

Done!

> he above would change to something like with what I have in mind:
>
> 	# Presuming the record exists.
> 	sub = Subscriber.load("[e-mail]")
> 	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.

For a number of reasons, I took the approach that changing a variable  
also changes the file right away.
One of the reasons is that I will _very_ often 1) Open a subscriber  
2) Change a little piece of information 3) Close the subscriber. The  
sub.save method would have to be intelligent enough to work out  
what's changed... it's a little too messy. It's much easier this way.

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

Cool!

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

Very true.
I chose "link_to()".

> The solution I'd probably go for is:
> 	- ::new would create a subscriber record in memory.

OK.

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

OK. That's exactly what I did. It seems to be working fine.

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

OK.

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

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

I know. And I can see why it doesn't really make sense to do it this  
way.
THANKS!

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

...and it would be really quite messy to manage the "writer", for  
example!

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

Oh... true!

> 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

OK. I can't believe I didn't think of this myself.

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

You mean with something like:

sub_container=SubscribersContainer.new()
sub_container['merc'].name="tony"

...?
When the method [](email) is called, the object SubscribersContainer  
would need to create a new object (if necessary), or return the one  
already created at some point in the past. Is that right?

I am not sure why you say that this class would need the data  
directory...!

Also, I wonder if accessing too many objects that way wouldn't  
clutter the collection too much (the "real" number of subscribers we  
have is about 14000. I KNOW we need a DB. We didn't expect quite so  
many. I am planning to switch to DB)

>  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'll ask about this later...
The question will include the fact that I would like to make this  
class "generic", so that in the future I can change it so that it  
connects to a DB rather than reading the file system. At this point,  
I am not sure what the right path is. Maybe create a subscribers  
class, and then create a SubscriberFileSystem subclass which needs to  
implement get_flag, set_flag, get_field, set_field...?

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

OK.

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

OK.

>> * 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,

Woops... I've lost you here. Are you talking about one specific book?  
Or two books?
There are quite a few books which have a title that starts with  
"refactoring", and I couldn't find any with "Gang of four"...!
What I am really after, is a book with a list of common problems and  
common patterns, so that I can just apply the one that fits my  
design. Something Ruby-oriented would be _perfect_!

> 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 ;)

OK :-)

Well... here is the latest version of my class.
It would be fantastic if you could have a quick look at it, and let  
me know if I actually got it right design-wise.
I haven't actually tested it properly - I am more interested in the  
big picture for now...

THANKS AGAIN!

#!/usr/local/bin/ruby -w

class Subscriber

         # Set the config file.
         #
         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()
                 @email=nil
         end

         def first_letter
                 return nil if ! @email
                 @email[0, 1].upcase if @email
         end

         def full_path
                 return nil if ! @email
                 @@config_data_dir+"/current/"+self.first_letter 
+"/"+@email+"/"
         end

         # This just checks that the directory actually
         # exists. It creates a "link"
         #
         def link_to(email_param)

                 # Hang on: if the file doesn't exist, undo everything
                 #
                 @email=email_param
                 if ! File.exist?(self.full_path)
                         @email=nil
                         return false
                 end

                 @premium_flag=nil
                 @moderator_flag=nil
                 @unconfirmed_flag=nil

                 @country=nil
                 @creation_date=nil
                 @name=nil
                 @password=nil
                 @postcode=nil
                 @premium_expiry_date=nil
                 @questionnaire_res=nil
                 @subscriber_code=nil
                 @subscriber_comments=nil

                 true

         end

         attr_reader :email

         # Accessors for the subscriber's information

         #
         # FLAGS:
         #

         # premium_flag
         #
         def premium_flag?
                 @premium_flag ||= get_flag(:premium_flag)
         end
         def premium_flag=
                 @premium_flag = set_flag(:premium_flag,value)
         end

         # moderator_flag
         #
         def moderator_flag?
                 @moderator_flag ||= get_flag(:moderator_flag)
         end
         def moderator_flag=(value)
                 @moderator_flag = set_flag(:moderator_flag,value)
         end

         # unconfirmed_flag
         #
         def unconfirmed_flag?
                 @unconfirmed_flag ||= get_flag(:unconfirmed_flag)
         end
         def unconfirmed_flag=(value)
                 @unconfirmed_flag=set_flag(:unconfirmed_flag,value)
         end

         #
         # ATTRIBUTES
         #

         def country
                 @country ||= get_field(:country)
         end
         def country=(value)
                 @country = set_field(:country,value)
         end

         def creation_date
                 @creation_date ||= get_field(:creation_date)
         end
         def creation_date=(value)
                 @creation_date = set_field(:creation_date,value)
         end

         def name
                 @name ||= get_field(:name)
         end
         def name=(value)
                 @name = set_field(:name,value)
         end

         def password
                 @password ||= get_field(:password)
         end
         def password=(value)
                 @password = set_field(:password,value)
         end

         def postcode
                 @postcode ||= get_field(:postcode)
         end
         def postcode=(value)
                 @postcode = set_field(:postcode,value)
         end

         def questionnaire_res
                 @questionnaire_res ||= get_field(:questionnaire_res)
         end
         def questionnaire_res=(value)
                 @questionnaire_res = set_field 
(:questionnaire_res,value)
         end

         def subscriber_code
                 @subscriber_code ||= get_field(:subscriber_code)
         end
         def subscriber_code=(value)
                 @subscriber_code = set_field(:subscriber_code,value)
         end

         def subscriber_comments
                 @subscriber_comments ||= get_field 
(:subscriber_comments)
         end
         def subscriber_comments=(value)
                 @subscriber_comments = set_field 
(:subscriber_comments,value)
         end

         private

         def get_flag(flag)
                 return nil if ! @email
                 File.exist?(self.full_path+flag.to_s)
         end

         def get_field(field)

                 return nil if ! @email

                 # Reads the file from the file system; returns
                 # nil if the file can't be opened
                 #
                 begin
                         IO::read(self.full_path+field.to_s)
                 rescue SystemCallError
                         nil
                 end

         end


         def set_field(field,value)

                 return nil if ! @email

                 # Open the file
                 #
                 begin
                         ios=File::open(self.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)
                 #
                 begin
                         ios.print(value)
                 rescue SystemCallError
                         ios.close
                         return nil
                 end

                 # OK, it worked: assign the new value
                 #
                 ios.close
                 value

         end

         def set_flag(flag,value)

                 return nil if ! @email

                 if(value)
                         begin
                                 File.open(self.full_path+flag.to_s,"w")
                         rescue SystemCallError
                                 return nil
                         end
                         return true
                 else
                         begin
                                 File.delete(self.full_path+fiag.to_s)
                         rescue SystemCallError
                                 return nil
                         end
                         return false
                 end
         end

end

a_subscriber=Subscriber.new()
p a_subscriber.email
p a_subscriber.link_to("[e-mail]")
p a_subscriber.email
p a_subscriber.premium_flag?
p a_subscriber.moderator_flag?
puts "OK:"
p a_subscriber.name="ooppp!!!"
p a_subscriber.name


Merc.