Classes and OO design - help


#1

Hello Everybody,

First of all, thank you so much for participating to this list and
help poor Ruby newbies like me!

I feel the need to start this email with an apology. I am a terrible
programmer.
Ruby is the only thing that could possibly convince me to start
programming again. I hadn’t fallen in love with a computer language
in a long time!

I can say that I “know” Ruby. I have a very good memory, which helped
me along the way. I don’t know it well, and I lack the necessary
experience to be comfortable with it, but I “know” it.

However, I am absolutely hopeless at designing anything in terms of
objects and OOP. This is why I have the feeling I am just about to
embarrass myself. But hey…

I started writing my first “real” program in Ruby, and here I am,
thinking: is this correct? Would a “real” programmer do this like way?

So, here I am. I have a file system, with the following contents:

[…]/A/
[…]/B/
[…]/C/
[…]/D

Under /A/, there is:

/A/removed_email_address@domain.invalid/

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”! :slight_smile: ) a class to access this
information on the file system.

Here it is:


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

class Subscribers

     # 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

     # 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

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

             true

     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)
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(“removed_email_address@domain.invalid”)
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)


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

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

  • Is this solution OO sound?

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.

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

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?
How would it access the class information such as @@config_data_dir,
or the subscriber’s instance variable?

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

I am just a little scared of doing anything right now. Did I design
it all wrong? Should I have created the classes SubscribersFlags and
SubscribersAttributes, and have two instance variables in Subscribers
derived from SubscribersFlags and SubscribersAttributes?

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

Thanks a lot!

Merc.


#2

I know it doesn’t answer your question, but wouldn’t it be way, WAY
easier to use a database to store your data?

Jon


#3

Hi,

I know it doesn’t answer your question, but wouldn’t it be way, WAY
easier to use a database to store your data?

Short answer: yes :slight_smile:

It’s a long story - I can’t do it right now…

Merc.


#4

DÅ?a Nedeľa 19 Február 2006 17:19 Tony M. 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 :wink: (For given values of full.)

I know there are much better ways of doing so, and that this approach

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

class Subscribers

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

     end
     # (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”.

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

             #
                     return nil
             rescue SystemCallError
     def set_flag(flag,value)

a_subscriber=Subscribers.new()
puts a_subscriber.link_to_fs(“removed_email_address@domain.invalid”)
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("removed_email_address@domain.invalid")
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.

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 :stuck_out_tongue: 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 :wink:

David V.


#5

Tony M. wrote:

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!

  • Design Patterns: Elements of Reusable Object-Oriented Software (aka
    “the Gang of Four book”)

This is the book that basically started the software patterns movement.

http://www.amazon.com/gp/product/0201633612/102-0783346-4510536?v=glance&n=283155

I need to bookmark that last one, and buy the other two myself… :slight_smile:


#6

Dave C. wrote:

I need to bookmark that last one, and buy the other two myself… :slight_smile:

i prefer Head 1st Design patterns (Oreilly) and Shalloway/Trott DP
explained (i think that’s waht it’s called).

Herrington’s bit is very good:
http://www.artima.com/rubycs/articles/modular_apis_with_ruby.html

this has potential
http://raa.ruby-lang.org/cat.rhtml?category_major=Library;category_minor=Design%20Patterns


#7

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 :wink: (For given values of full.)

Yeah right, I know people like you :smiley:

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.

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.

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!

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

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

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

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

OK :slight_smile:

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(“removed_email_address@domain.invalid”)
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.


#8

DÅ?a Pondelok 20 Február 2006 10:48 Tony M. 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 :wink:

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.

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.

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

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

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

             return nil if ! @email
             #
             @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.

     #
     # moderator_flag
     def unconfirmed_flag?
     def country
             @creation_date = set_field(:creation_date,value)
             @password ||= get_field(:password)
     end
             @subscriber_code ||= get_field(:subscriber_code)
             @subscriber_comments = set_field
     def get_field(field)
             end
             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.

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.

                     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.

p a_subscriber.name

Merc.

David V.


#9

Hi,

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

OK.

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.

True. I did some testing, the overhead is negligible.

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.

OK.

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

OK.

instance.
Not really useful though.

I don’t have enough experience in Ruby to even understand why you’d
do that, or when it would make sense to create singleton classes.
In fact: when does it make sense to mix a class with Singleton?

sub_container[‘merc’].name=“tony”

…?
When the method 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.

Wooops… Maybe we had a misunderstanding here? I meant that
sub_container[‘merc’] would need to create a new object (of type
Subscriber) and link it to the file system. I wasn’t thinking about
creating a new record on disk.
So, my question was: if each one of these calls:

p sub_container[‘merc’].name
p sub_container[‘dave’].name
p sub_container[‘bridget’].name
p sub_container[‘anna’].name

Allocates a Subscriber object in the collection, after 14000 I’d have
allocated 14000 Subscriber objects.

Or maybe my understanding of containers is still far too poor to
follow you properly.

There are other ways to preventing that though, like providing a
method to
check whether a given record exists.

OK.
Right now, I must admit I have no idea where I’d start creating a
container.

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.

Very true.

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

You’re completely right.
Now that I have a better understanding of scoping, this makes sense.

as much context as possible.
But this would surely mean that “Subscriber” is not really usable
without the container… wouldn’t it?
(maybe that’s not a problem?)

class “generic”, so that in the future I can change it so that it
backend and the SQL one.
OK.

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.

OK.

case study
bit as an explanation that shows an example of quite a few of those
applied
in a single program.

OK.
I find that a lot of these books apply to Java or C++. Is there a
Ruby Patterns book out there?
If not… well, it should exist!

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

Where abouts?

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.

OK.

variables results
in a nil by default.

This is me being me. It’s nice to know WHAT instance variables are
there. I’m an obsessive compulsive, you see :slight_smile:
OK, comments exists for that reason…

                     return nil

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.

I wasn’t sure about this bit, and you confirmed my fear.
Thanks a lot!
I added a “raise” there.

             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.

OK.
I didn’t have logging abilities in the object right now. I have no
idea where to start, with that.
I also divided the block in two, because in the second half of the
code I close the file if there was a problem.
I can see that with the “block” in File::open I can do everything at
once…!

So, the function has become:

     def set_field(field,value)

             return nil if ! @email

             # Open the file
             #
             begin
                     File::open(self.full_path+field.to_s,"w") do

|ios|
ios.print(value)
end
rescue SystemCallError
raise
return nil
end
value

     end

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.

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.

OK.
I’ve just put “raise” there:

def set_flag(flag,value)

             return nil if ! @email

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

Well, the only obscure bit now is how to make this “containable”.
It’s probably not necessary, because the class works well “as it is”.
However…

OK, I am assuming that to make the container, basically I would have:

  • A class called “SubscribersContainer”. This class would have the
    methods “country=”, country(), and so on; those methods would all use
    the methods set_field and get_field, NOT implemented in the container

  • A class called SubscriberFS (which I have), which would ONLY
    implement get_field, set_field, get_flag, set_flag. These methods
    will be used by the container to do the “real” work

  • I could also have a class called SubscribersDB, which would do the
    same things but connecting to a database

However, I have so many questions in this case… For example,
SubscribersDB would need far more information than just a path (like
SubscribersFS). Where would this information be stored? What if it
changed?
And what would actually happen when I did Subscriber
[‘removed_email_address@domain.invalid’].name=“Tony”, data-wise?

I feel I am out of my leagues here.

If you have time, David (or anybody else), I would love it if you
could write a basic skeleton for the two classes - something that
would make me understand what goes where.
However, I feel I am abusing of your time. So… let’s say that I’m
not expecting it!

Bye,

Merc.


#10

Hi,

I am not sure if this is considered off-topic, but…

               *** THANKS A MILLION *** !!!

Thanks to David, and to everybody else in this list.

Merc.


#11

DÅ?a Utorok 21 Február 2006 09:58 Tony M. napísal:

singleton and then delegate calls to the class object to the single
instance.
Not really useful though.

I don’t have enough experience in Ruby to even understand why you’d
do that, or when it would make sense to create singleton classes.
In fact: when does it make sense to mix a class with Singleton?

In this case? I’d do that to make the code horribly confusing while
still
being able to drop pattern names as an excuse. Pure malice :wink:

It would make sense to make singletons when you need to make sure
there’s one
and only one object of a given class. Basically, they’re the same as
globals,
except you can use encapsulation with them. In Java, they also save up a
lot
of typing “static” over and over in the long run, too (major feature
grin).
They also probably provide you with a little more flexibility in one
case or
another, can’t imagine an example though.

record.
p sub_container[‘anna’].name

OK.
Right now, I must admit I have no idea where I’d start creating a
container.

The Subscriber objects have to be allocated somewhere anyway, the
difference
is where you’d put what. 14000 elements isn’t a particularly large data
set
anyway - with 150 bytes of data per record on average, this is about 2
MB in
total.

SubscriberContainer wouldn’t necessarily be a real collection. But I’ll
let
the code do the talking:

class SubscriberContainer
def self.
sub = Subscriber.new
sub.link_to(email)
return sub
end
end

Of course, you could use some weak array / hash or something like that
to
cache these objects. I haven’t really worked with that though. In this
case,
it would even be recommendable, because two objects representing the
same
record would very probably lead to bugs because of the caching.

For example if there’s a record for “removed_email_address@domain.invalid” with an
attribute
“name” with the value “Fred Flintstone”, you’d get:

ff1 = Subscriber.new
ff1.link_to “removed_email_address@domain.invalid”

ff2 = Subscriber.new
ff2.link_to “removed_email_address@domain.invalid”

ff1,name # The value “Fred Flinstone” gets cached.

ff2.name # Same as above.

ff1.name = “Barney Rubble” # Value gets changed on disk and inside
the ff1
object.

puts ff2.name # The ff2 object doesn’t notice a change, and doesn’t
reread
a cached value

Only accessing the records per objects managed in the container would
prevent
this, because at most one Subscriber object would exist per record.

On this spot, I’d probably think even more of using an ACID compliant
database
backend and a persistence layer to handle the nitty gritty details for
you.

runtime from a
container to
You wouldn’t have to actually store the retrieved object in the

No, it’s not a problem. The functionality provided would stay the same,
just
the responsibility for providing it would be split across the two
classes.
The fact a container class exists could be concealed to a lot of code
using
the subscriber objects.

The point is keeping sets of related bits of code separated from each
other as
much as possible - we need only very little information from a
Subscriber
object to store a new value of a field of the object - only the id of
the
record (the e-mail address), the field name, and the new value.
Therefore
it’s more concise to have a separate component with access to the
minimum
amount of information necessary to implement this operation.

there are

I think someone made “translations” of the source code in these books to
Ruby
and announced that to the ML. Try searching the archives for it? The
basic
concepts are pretty much the same between the languages, except for a
few
differences in what “special” language features can be used to implement
what
patterns more efficiently than the “standard” ones.

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

This is me being me. It’s nice to know WHAT instance variables are
there. I’m an obsessive compulsive, you see :slight_smile:
OK, comments exists for that reason…

Hehe, I know that. I can’t learn to omit the return keyword, even if
it’s
actually slower, and on the other hand, can’t make myself write
parentheses
when declaring a method without arguments…

Oh, and I also do the assignment of nil thing, just not for variables I
have
accessors for.

             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.

OK.
I didn’t have logging abilities in the object right now. I have no
idea where to start, with that.

Print to STDERR? You might check how your webserver works and if you can
integrate into that.

                     raise
                     return nil
             end
             value

     end

I think you can actually omit the begin / rescue / end here. The “return
nil”
is never reached. You can’t both raise an exception and return from a
function normally.

the stack -it’s probably not possible to gracefully recover from it.
File.open(self.full_path+flag.to_s,“w”)
return false
end
end

Same as above, you can as well let the call to File.open raise the
exception
for you, it’s not necessary to do it explicitly.

Well, the only obscure bit now is how to make this “containable”.
It’s probably not necessary, because the class works well “as it is”.
However…

Very true. The code as it is is likely to work well enough until you get
a
large enough codebase to warrant separating data storage and data
access.

  • I could also have a class called SubscribersDB, which would do the
    same things but connecting to a database

Actually, I meant the naming the other way around. Subscriber would
access the
data, and the container would represent the backends - the roles of the
respective objects stay the same. You’d have a single Subscriber class,
and
then a separate FSContainer and a DBContainer, that would implement the
specifics of writing the data into the backends.

Something like (excerpts):

class Subscriber
def initialize(container)
@container = container
end
def get_field(field)
container.get_field(@email, :field)
end
# Etc. for #set_field, #get_flag, #set_flag
end

class FSContainer
def get_field(email, field)
# Find respective file, read it, return what’s inside.
end

# Create a new Subscriber stored in this container.
def self.[](email)
  sub = Subscriber.new(self)
  sub.email = email
  return sub
end

end

class DBContainer
def get_field(email, field)
# Connect to the DB and get the needed data from it.
end
end

My assumption is that most of the time, you need to manipulate the data
in the
Subscriber records without caring how or where they are stored. For
creation
of new records, you could set a “default” container to use for that in
initialization

Or possibly make a “container of containers” - when looking for an
existing
record, this one would search the two “real” ones, and when creating a
new
record, use the default one. This way you’d completely contain the way
the
records are stored in the backends from the creation / loading of
records -
the operations you commonly need would be the same code no matter what
backend is used.

In the latter case, better names for classes would be Subscriber,
SubscriberFactory, FSStorage, DBStorage. The SubscriberFactory would be
the
mentioned “container of containers”, a class responsible for the
creation and
finding of Subscribers.

However, I have so many questions in this case… For example,
SubscribersDB would need far more information than just a path (like
SubscribersFS). Where would this information be stored? What if it
changed?

Given my proposed design, of course, the DBStorage would need
information how
to connect to a database, and about its layout. However, the subscribers
would still remain uniquely identified by their e-mail addresses, and
DBStorage would implement the same operations the Subscriber class needs
from
its storage object as FSStorage, just using DB access instead of file
access.

Mind you, I’d only use this specific approach if, and only if you really
need
to support both the backends at once, and only for few classes.
Otherwise,
you’d need to eventually connect each data class with each backend using
a
separate backend, which would be really messy, or have to implement a
generic
storage adapter for any type of record, which would be complicated, and
probably has already been done for SQL DBs. Of course, code based on
very
similar concepts could be handy when migrating the data between
backends.

And what would actually happen when I did Subscriber
[‘removed_email_address@domain.invalid’].name=“Tony”, data-wise?

Well, supposing the record doesn’t exist already:

- The call to Subscriber::[] (SubscriberFactory::[] in the naming I 

proposed
earlier) would use FSStorage or DBStorage to find a record for
“removed_email_address@domain.invalid”. (Using a method named like BlahStorage#include? or
similar.)
- This would fail (the record doesn’t exist), so the SubscriberFactory
would
create a new Subscriber object for this e-mail, using the default
storage to
handle it (let’s say it’s DBStorage)
- SubscriberFactory::[] returns this new Subscriber object - I’ll name
it
“subscriber” below
- The call to subscriber.name = “Tony” delegates to
subscriber.set_field(:name, “Tony”)
- subscriber.set_field(:name, “Tony”) calls
@storage.set_field(“removed_email_address@domain.invalid”, :name, “Tony”)

(The last two steps could be merged into one, but the call to set_field
would
have to be changed in all the setters. Using this middle-man is the lazy
way
out.)

The last method is probably implemented as something like:

class DBStorage
def set_field(email, field, value)
# db is the database connection
db.execute(
“UPDATE subscribers
SET subscribers.#{field.to_s} = ?
WHERE subscribers.email = ?”,
value, email)
end
end

I feel I am out of my leagues here.

Patience, young grasshopper. (Even if it’s more likely you’re older than
me :P)

If you have time, David (or anybody else), I would love it if you
could write a basic skeleton for the two classes - something that
would make me understand what goes where.
However, I feel I am abusing of your time. So… let’s say that I’m
not expecting it!

Maybe when I remember this in daytime to get my mind off Java at work
for a
little while, I have enough coding Ruby in my free time on a side job…

David V.