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