I have a basic stylistic question I would like some feedback on.
I have a table containing messages, each message has a status of
current or expired.
Messages expire a set period after they were created. Right now I
have a function in my controller to get the current message. This
function also has the expiration logic built into it.
Get Current
msg = Message that is marked current in the DB.
if msg == nil
return nil
end
if msg.hasExpired?
Set message to expired in DB
return nil
else
return msg
end
end
So I was thinking it would be useful to put this in my model. I could
add a method to the model called getCurrent that had this exact
logic. I could also put the expiration logic in an after_find
callback.
But this seems like business logic so I’m not sure this is a good
idea? Should this stay in the controller, if I put in the model what
approach is best.
On 20 June 2011 01:15, John SB [email protected] wrote:
msg = Message that is marked current in the DB.
So I was thinking it would be useful to put this in my model. I could
add a method to the model called getCurrent that had this exact
logic. I could also put the expiration logic in an after_find
callback.
But this seems like business logic so I’m not sure this is a good
idea? Should this stay in the controller, if I put in the model what
approach is best.
Expiring messages is certainly business logic so it should go in the
model not the controller. You could put the code in an after_find
callback in the model so it is run every time a record is fetched.
However if the decision as to whether a message is expired or not is
entirely based on the time since creation there may be no need for a
column in the database. Simple add a method to the model, something
like
def expired?
Time.now - self.created_at > 10.days # or whatever the period is
end
Colin
def expired?
Time.now - self.created_at > 10.days # or whatever the period is
end
fwiw, I prefer to write that sort of condition as:
=====
def expired?
self.created_at < 10.days.ago
end
~ jf
John F.
Principal Consultant, BitsBuilder
LI: http://www.linkedin.com/in/johnxf
SO: User John Feminella - Stack Overflow
On 20 June 2011 09:15, John F. [email protected] wrote:
=====
Yes, much better, I had forgotten about ago.
Colin
Thanks for that, I simplified things a bit for the post. There is
actually a reason to have an explicit status.
I’m going ahead with adding the after_find callback but I think I’ve
run into another issue.
I was thinking I would do the following in after_find when I detected
an expired message.
Change its status to expired.
Save It
Return false.
I was thinking returning false would result in the find not returning
that result. But it still seems to be returning it. I couldn’t find
anything online that really explained what is supposed to happen when
you return false in this callback. Any ideas?
Always make your logics in Model,.
On 22 June 2011 06:36, John SB [email protected] wrote:
Save It
Return false.
I was thinking returning false would result in the find not returning
that result. But it still seems to be returning it. I couldn’t find
anything online that really explained what is supposed to happen when
you return false in this callback. Any ideas?
I presume here you are trying to get round the problem that if a find
is performed with a condition of !expired then the find should somehow
reject the record after setting it to expired. I had not thought of
that complication. In particular if the find is for a set of records
then the changed records must be removed from the collection. I don’t
know how to do that.
Any suggestions from the more experienced here?
Colin
On 22 June 2011 08:38, Chirag S. [email protected] wrote:
It may sound silly simple, but why not just defined a named scope for that
and be done with it.
Assuming you are on Rails 3 something like this should work:
scope :active, lambda {where([“created_at > ?”, 10.days.ago])}
I tend to agree, it is much simpler to do it this way rather than
having an explicit status. It is also poor database design as you
have the same information stored twice in the database (once in the
explicit status and once in created_at). If you need an explicit
status then just provide a method called expired which returns the
state based on created_at. To the code calling this it is virtually
indistinguishable from a value stored in the database.
Colin
It may sound silly simple, but why not just defined a named scope for
that
and be done with it.
Assuming you are on Rails 3 something like this should work:
scope :active, lambda {where([“created_at > ?”, 10.days.ago])}
On Jun 22, 3:29am, Colin L. [email protected] wrote:
run into another issue.
anything online that really explained what is supposed to happen when
you return false in this callback. Any ideas?
I presume here you are trying to get round the problem that if a find
is performed with a condition of !expired then the find should somehow
reject the record after setting it to expired. I had not thought of
that complication. In particular if the find is for a set of records
then the changed records must be removed from the collection. I don’t
know how to do that.
Any suggestions from the more experienced here?
If it’s absolutely necessary to have an explicit status, I’d typically
do this with a cron script that expires messages at a sensible
frequency (nightly, perhaps?) as expiring them on-load (as you’ve
noticed) is going to make an epic mess - not only will collection
finds not work right, but simple stuff like “count the number of
active messages” will be harder than needed.
–Matt J.
On 23 June 2011 00:33, Andrew S. [email protected] wrote:
If you want to search for expired messages, just add another scope:
scope :expired, lambda {where([“created_at < ?”, 10.days.ago])}
I am not sure about that, but have not got time to test it now. If
you have a default scope and then add another will not the :expired
scope be applied as well as the default, resulting in no records
found at all.
Also, even if the above does work I think one of the scopes should
include the exact equality, otherwise records that are exactly
10.days.ago will not be found by either (not that there will be many
as it must be exact to the second I think).
Colin
Chirag S. <chirag.singhal@…> writes:
It may sound silly simple, but why not just defined a named scope for that
and be done with it.
Assuming you are on Rails 3 something like this should work:
scope :active, lambda {where([“created_at > ?”, 10.days.ago])}
Bingo!
Although given what he seems to be attempting here I would make it the
default
scope:
scope :default, lambda {where([“created_at > ?”, 10.days.ago])}
If you want to search for expired messages, just add another scope:
scope :expired, lambda {where([“created_at < ?”, 10.days.ago])}
Also, you can’t define default scope with lambda until Rails 3.1
You can use “unscoped” if you want to override default scope
–
Chirag
http://sumeruonrails.com