I would personally do it as something like :
def initialize_with_defaults(attributes = nil, &block) initialize_without_defaults(attributes) do self.some_attribute = 'whatever' # Setting default value yield self if block_given? end end alias_method_chain :initialize, :defaults
Looks like it's working quite well.
Thanks, Pratik
Yeah, I think I've heard the Holy Beasts of Doom argument as well, I just can't for the life of me remember where. I have certainly been bitten by trying to set the defaults before calling super though (because, dammit, that's the logical place to set defaults). Thinking about it, the way to do that would be something like:
def initialize(attrs = {}, &block) super attrs.reverse_merge(:default => 'this'), &block end
but then you make yourself a hostage to fortune that everyone's going to use symbols as keys. The post super approach to setting defaults has some edge case problems too in cases where nil is a legal value for some attribute (yeah, it's a weird edge case, but an edge case all the same). You end up with ugly code like:
def initialize(attrs = {}, &block) super unless attrs.has_key?(:content) || attrs.has_key?('content') self.content = "Write something here" end end
The issue is, I think, that ActiveRecord::Base#initialize is doing two different 'sorts' of things: class invariant metadata initialization, and instance specific initialization. You might compose the method as:
def initialize(attributes = nil, &block) initialize_metadata initialize_instance(attributes, &block) end
def initialize_metadata @attributes = attributes_from_column_definition @new_record = true ensure_proper_type end
def initialize_instance(attributes self.attributes = attributes unless attributes.nil? yield self if block_given? end
Maybe the right thing to do is to implement ActiveRecord::Base.new as:
class ActiveRecord::Base def self.new(attributes = nil, &block) returning(self.allocate) do |instance| instance.initialize_metadata instance.initialize(attributes, &block) end end
def initialize_metadata @attributes = attributes_from_column_definition @new_record = true ensure_proper_type end
def initialize(attributes) self.attributes = attributes unless attributes.nil? yield self if block_given? end end
Then anyone who wants to write code that initializes defaults before the actual attributes are set can do:
def initialize(attributes = nil, &block) self.content = "Write something here" super end
and everybody is happy.
Except in the case where it overrides a value that's already been set by initialize_without_defaults. I'm not sure that's the behaviour you want.
Plus, imitating the action of super by hand coding seems like a bit of a waste of time somehow.
Except in the case where it overrides a value that's already been set by initialize_without_defaults. I'm not sure that's the behaviour you want.
Yeah. initialize_without_defaults is basically AR::Base#initialize - so yeah, this will override values set by that. If you're already overriding default values, I'm not sure how this might be a problem.
Plus, imitating the action of super by hand coding seems like a bit of a waste of time somehow.
Well, basically this is supplying a block to AR::Base#initialize which is executed by "yield self if block_given?" - this protect it against possible changes that might happen to AR::Base#initialize. It also allows you to override the default values you've set. So you do something like :
Model.new do |model| model.i_dont_want_your_default = 'whatever' end
It might be useful in case of STI or in controllers where you want to override default values.
So yeah, it's not really imitating the action of super by hand, but using proper technique to set defaults I feel. This just feels cleaner.
Because what you're actually doing having your block override the a value that's been specified in the attributes hash with your default. Which probably isn't what you want.
Then do :
def initialize_with_defaults(attributes = nil, &block) initialize_without_defaults(attributes) do self.some_attribute ||= 'whatever' # Setting default value if not present yield self if block_given? end end alias_method_chain :initialize, :defaults
I think we're starting back around the circle again, but what happens when the default value is 'true' and the arguments specify 'false'.
And, then, when you've solved that one, what happens when 'nil' is a legal value, but the default value is non nil?
Umm..I think that leaves us with something like:
class Item < ActiveRecord::Base def initialize_with_defaults(attrs = nil, &block) initialize_without_defaults(attrs) do setter = lambda { |key, value| self.send("#{key}=", value) unless !attrs.nil? && attrs.keys.map(&:to_s).include?(key) } setter.call('scheduler_type', 'hotseat') yield self if block_given? end end alias_method_chain :initialize, :defaults end
Can you please point out situations where this will fails ?
Thanks ! -Pratik
Whenever I you try to understand it?
The 'rewrite' new approach means you have an easily overrideable 'initialize' that works with all the edge cases you're taking account of. Frankly, I'd much rather be able to write:
def initialize(attrs = nil, &block) self.body = "Write something here" super end
and know it's going to work. Your mileage may vary.
Oh yes, I'll bet dollars to doughnuts that the rewrite new approach is the faster of the two as well.
Whenever I you try to understand it?
You don't have to lean on sarcasm when you have nothing constructive to say ( especially after you pointed out deftect in implementation and I corrected them ). May be you can focus on submitting a patch.
patch -p0 speaks a lot louder than your sad grammar.
AR:Base#initialize is supposed to be an internal method. And it should be flexible enough to be changed in future for whatever reasons.
because you can say that about all of the callbacks.
Umm...what would you suggest as alternative for before/after_add/remove association collection callbacks ?
Anyways, I'm off this thread unless we see some patches. That makes
arguing a lot easier 
the trick, of course, is that attrs are sometimes nil - also we (client code) don't know what/if any block might be, or not be, for... i just had this issue last week and here is how i solved it
class Agent < ActiveRecord::Base def initialize options, &block super((options || {}).reverse_merge!(defaults), &block) end def defaults Hash.new end end
this is used in an STI situation so i also have code like this
class Group < Agent def defaults { 'allow_login' => false, } end end
class User < Agent def defaults { 'allow_login' => true, } end end
and this seems to be working well. so my own suggestion wouldn't be so much for another callback but a way to simply set the defaults for a record, perfering a method over a hash so that current object state and arbitrary logic might affect the result, Time.now, for instance.
also, i strongly agree that this is a giant hole in the ar lifecycle.
kind regards.
one other nit-pick: it's somewhat archaic and java like to __require__ client code to call super, one slip of the keyboard and someone is bug finding...
it's much nicer, imho, to relieve the client code from that burden via
class ActionRecord::Base def self.new *args, &block returning(allocate) do |object| object.instance_eval do internal_initialize *args, &block # all real work here initialize *args, &block # default does nothing end end end end
basically just following the paradigm whereby designing classes that a made to be inherited with any work in initialize is less that ideal... food for thought.
cheers.
i'm not suggesting an alternative, i saying that just because docs *can* take the place of easy code doesn't mean they should. i'd just hate to see this in the docs:
# this is how to write a 'before save' filter in rails
Yeah. You're right. When I wrote that I didn't really think about attr being false/nil etc. And I assumed the solution to be as simple as overriding initialize and calling super as the first statement.
--- vendor/rails/activerecord/lib/active_record/base.rb.org 2007-07-24 11:37:07.000000000 -0600 +++ vendor/rails/activerecord/lib/active_record/base.rb 2007-07-24 11:40:32.000000000 -0600 @@ -1502,10 +1502,14 @@ @attributes = attributes_from_column_definition @new_record = true ensure_proper_type - self.attributes = attributes unless attributes.nil? + self.attributes = (attributes || {}).stringify_keys.reverse_merge!(defaults.stringify_keys) yield self if block_given? end + def defaults + Hash.new + end
Except that, when you're setting defaults, you really would like to interpose some behaviour between the bit that gets the object to the point where the various setter/getter methods will work and the bit where ActiveRecord calls self.attribs = ...
One option would be to override ActiveRecord::Base.allocate to return an object that's been got to that point, but then you'd be doing a chunk of work that then gets thrown away whenever an object is made through AR::Base.instantiate.
Sadly, initialize tends to be one of those warts on an API where you really can't get away with requiring client code to call super. You *could* add #before_initialize and #after_initialize hooks, but then we're back where we started...
yeah i agree completely. my impl just happened to skin my own cat quite nicely at the time, but a more general solution would do just that...
before_initialize and after_initialize don't seem to bad after all eh?
kind regards.