Re: Classes and OO design - help

Dňa Pondelok 20 Február 2006 10:48 Tony Mobily napísal:
> 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!)
>

Not particularly, although not really necessary. I'm enough of an attention 
whore to see if I've been replied on the list ;)

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

Not really on the low level, short of starting the garbage collector. You'd 
call the deletion method explicitly.

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

Well, I doubt it's a killer in this case, but indeed probably a waste of CPU 
speed doing the same over and over.

You could cache the result of the path generation in the instance variable and 
lazily initialize it in the accessor, as you've done in the other getters. 
This should work:

	def full_path
		@full_path ||= parts + of + full + path
	end

Or plain keep this bit in the initializer as it was, the value is bound to be 
used anyway. Keeping the path generation bit where it's used seems a taaad 
bit cleaner to me.

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

Wouldn't call it messy. Either way works, depends on how you use the objects. 
If you're can handle the difference between creating new records and loading 
old ones cleanly, it's fine. 

I think ActiveRecord only requires explicit saves on newly created objects and 
makes modifications to existing records transparently as you intend.

> >> * 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()

A particularly sick thing to do would be making SubscribersContainer a 
singleton and then delegate calls to the class object to the single instance. 
Not really useful though.

> 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'd still keep the creation and loading as separate operations to avoid 
clobbering some data by mistake when thinking you're making a new record. 
There are other ways to preventing that though, like providing a method to 
check whether a given record exists.

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

Well, I thought of this class providing the created Subscriber with the full 
record path and the e-mail address when creating the object. That way, you'd 
keep the what's stored where bit out of the Subscriber class.

You could also determine / change the config file directory at runtime from a 
parameter to the application more intuitively 
(SubscriberContainer.new("/path/to/record/directory/") instead of hardcoding 
it or manipulating class variables), or even have several containers - even 
if this would probably be rarely useful.

Last, but not least, at least to me it makes more sense for the container to 
have the information where its contents are stored.

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

You wouldn't have to actually store the retrieved object in the container 
after creation / loading, just have the container do this work and dumb down 
the Subscriber storage to data transfer and mutation, those being ignorant to 
as much context as possible.

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

For a SQL DB, I'd just skip the whole issue, use an ORM library like Og, port 
the code using Subscriber to it and not care about this bit. Possibly the 
easiest way if you can do an "instantaneous" migration between the filesystem 
backend and the SQL one.

The very generic solution would be worth the effort if you had to use the two 
wildly differing backends simultaneously. In that case, I'd definately keep 
the "housekeeping" to a container class, have the Subscriber objects know 
what container they belong in, and have them call on the container to sasve 
changes to them to disc. This would also possibly let you relocate objects 
between containers if that was of any use at all. You'd probably want to 
store which field(s) was changed to avoid unnecessary file access with this 
approach.

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

Well, THE Refactoring book *cackle*. But Dave Cantrell already answered this 
perfectly. Gang of Four is a nickname of the four authors of the book. Not 
quite up-to-date as far as the patterns mentioned are concerned - there are 
already droves more that have been invented since. But I like the case study 
bit as an explanation that shows an example of quite a few of those applied 
in a single program.

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

Very minor quibble: I don't quite like too much code executed inline in class 
definitions, and would probably move this along with all other application 
initialization into one place. Quite possibly a matter of taste more than 
anything else though - since the code doesn't have side-effects, it's not 
likely to cause any obscure bug due to initialization timing.

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

This shouldn't be necessary. Reading uninitialized instance variables results 
in a nil by default.

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

You might not want to ignore the error here. It probably means something 
really, really unexpected happened - the only thing short of a kernel-ish 
problem, like a file system faliure or the file handle table filling up that 
could call an error in this I can imagine is that a file of the same name 
without the necessary write permissions exists, and that seems unlikely for 
this application. Either way, a SystemCallError sounds like a showstopper for 
me.

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

I'd merge this code block with the previous one, and possibly handle the 
exception somewhere else, reporting it to the user as a severe failure, and 
logging it. You could also use the block form of File::open here.

You'd end up cluttering the code with checks for nil all the time, which is 
only proper if you expect the issue to appear during more-or-less regular 
operation; e.g. for calls where a failure isn't abnormal.

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

Same here as in the previous method, just let the SystemCallError pass up on 
the stack -it's probably not possible to gracefully recover from it.

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

David Vallner