How much logic should I add to my ActiveRecords

[John SB]

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.

[Colin Law]

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

[John Feminella]

> 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 Feminella
Principal Consultant, BitsBuilder
LI: http://www.linkedin.com/in/johnxf
SO: http://stackoverflow.com/users/75170/

> --
> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group.
> To post to this group, send email to rubyonra...@googlegroups.com.
> To unsubscribe from this group, send email to rubyonrails-ta...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
>
>

[Colin Law]

On 20 June 2011 09:15, John Feminella <jo...@bitsbuilder.com> wrote:
>> 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
> =====

Yes, much better, I had forgotten about ago.

Colin

[John SB]

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?

[Colin Law]

On 22 June 2011 06:36, John SB <jo...@shriver-blake.com> wrote:
> 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.

Any suggestions from the more experienced here?

Colin

>
> On Jun 20, 6:53 am, Colin Law <clan...@googlemail.com> wrote:
>> On 20 June 2011 09:15, John Feminella <jo...@bitsbuilder.com> wrote:
>>
>> >> 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
>> > =====
>>
>> Yes, much better, I had forgotten about ago.
>>
>> Colin
>

[Chirag Singhal]

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])}

[Colin Law]

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

>
> --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Talk" group.

> To view this discussion on the web visit
> https://groups.google.com/d/msg/rubyonrails-talk/-/9S1lx20_7JoJ.

[Matt Jones]

On Jun 22, 3:29 am, Colin Law <clan...@googlemail.com> wrote:

> On 22 June 2011 06:36, John SB <j...@shriver-blake.com> wrote:
>
>
>
>
>
> > 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.
>
> 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 Jones

[Andrew Skegg]

Chirag Singhal <chirag.singhal@...> writes:

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])}

[Colin Law]

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 Singhal]

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