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.
--
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
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.
On 20 June 2011 01:15, John SB <john-HgOH2ccuizrKr2zmH8/g2gC/G2K4zDHf@public.gmane.org> wrote:> 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 Colin -- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
> def expired? > Time.now - self.created_at > 10.days # or whatever the period is > endfwiw, 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/ On Mon, Jun 20, 2011 at 04:04, Colin Law <clanlaw-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:> On 20 June 2011 01:15, John SB <john-HgOH2ccuizrKr2zmH8/g2gC/G2K4zDHf@public.gmane.org> wrote: >> 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 > > Colin > > -- > 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. > For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en. > >-- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
On 20 June 2011 09:15, John Feminella <johnf-u89qwezJ71hz+5FpPkU+UQ@public.gmane.org> 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 -- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
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? On Jun 20, 6:53 am, Colin Law <clan...-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:> On 20 June 2011 09:15, John Feminella <jo...-u89qwezJ71hz+5FpPkU+UQ@public.gmane.org> 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-- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
On 22 June 2011 06:36, John SB <john-HgOH2ccuizrKr2zmH8/g2gC/G2K4zDHf@public.gmane.org> 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...-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote: >> On 20 June 2011 09:15, John Feminella <jo...-u89qwezJ71hz+5FpPkU+UQ@public.gmane.org> 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 > > -- > 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. > For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en. > >-- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
Chirag Singhal
2011-Jun-22 07:38 UTC
Re: Re: How much logic should I add to my ActiveRecords
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])}
--
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.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en.
On 22 June 2011 08:38, Chirag Singhal <chirag.singhal-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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> > -- > 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. > To post to this group, send email to rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > To unsubscribe from this group, send email to > rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > For more options, visit this group at > http://groups.google.com/group/rubyonrails-talk?hl=en. >-- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
On Jun 22, 3:29 am, Colin Law <clan...-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:> On 22 June 2011 06:36, John SB <j...-HgOH2ccuizrKr2zmH8/g2gC/G2K4zDHf@public.gmane.org> 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 -- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
Chirag Singhal <chirag.singhal@...> writes:> > > It may sound silly simple, but why not just defined a named scope for thatand 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])} -- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
On 23 June 2011 00:33, Andrew Skegg <andrewskegg-BUHhN+a2lJ4@public.gmane.org> wrote:> Chirag Singhal <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])}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 -- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.
Chirag Singhal
2011-Jun-23 06:14 UTC
Re: Re: How much logic should I add to my ActiveRecords
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 On Thu, Jun 23, 2011 at 11:35 AM, Colin Law <clanlaw-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:> On 23 June 2011 00:33, Andrew Skegg <andrewskegg-BUHhN+a2lJ4@public.gmane.org> wrote: > > Chirag Singhal <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])} > > 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 > > -- > 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > To unsubscribe from this group, send email to > rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > For more options, visit this group at > http://groups.google.com/group/rubyonrails-talk?hl=en. > >-- 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 rubyonrails-talk-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org To unsubscribe from this group, send email to rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en.