Da Pondelok 20 Februr 2006 10:48 Tony Mobily napsal: > 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("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. > > 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("merc2 / mobily.com") > 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