Improving this messy has_many :through with :include

I have a question about cleaning up a rather complex has_many. I'll describe the context in case it helps, but perhaps you can skip straight to the code below.

* I am designing a fantasy football type system. * I am modeling a Team which consists of many Players. * I am connecting Teams and Players via a Transfer join model. * A Transfer also has two GameWeeks (a GameWeek encapsulates the start_time and end_time of a period of play). * Transfer.from_end_of_gameweek indicates the GameWeek from which the Player becomes part of the Team. * Transfer.until_end_of_gameweek indicates the GameWeek from which a Player ceases to be part of the Team.

So Transfers is essentially acting as a warehouse, storing history about all player transfers that have taken place (in the future I might make a dedicated warehouse but not yet).

Now, to determine which Players are playing for a Team at a given instant I have this:

<pre> class Team < ActiveRecord::Base   has_many :transfers   has_many :players, :through => :transfers,     :include => [{ :transfers => :from_end_of_gameweek }, { :transfers => :until_end_of_gameweek }],     :conditions => ['gameweeks.start_time <= ? AND (gameweeks.end_time IS NULL OR ? < gameweeks.end_time)', Time.now.utc, Time.now.utc] end </pre>

Thus I can say team.transfers to get a list of all Transfers the Team has made, or team.players to get the current squad.

It works, but that has_many is a pretty unpleasant bit of code and the SQL doesn't look the most efficient. The question is, can this be made more elegant and maybe efficient?

Thanks.

I have a question about cleaning up a rather complex has_many. I'll describe the context in case it helps, but perhaps you can skip straight to the code below.

* I am designing a fantasy football type system. * I am modeling a Team which consists of many Players. * I am connecting Teams and Players via a Transfer join model. * A Transfer also has two GameWeeks (a GameWeek encapsulates the start_time and end_time of a period of play). * Transfer.from_end_of_gameweek indicates the GameWeek from which the Player becomes part of the Team. * Transfer.until_end_of_gameweek indicates the GameWeek from which a Player ceases to be part of the Team.

So Transfers is essentially acting as a warehouse, storing history about all player transfers that have taken place (in the future I might make a dedicated warehouse but not yet).

Now, to determine which Players are playing for a Team at a given instant I have this:

<pre> class Team < ActiveRecord::Base has_many :transfers has_many :players, :through => :transfers, :include => [{ :transfers => :from_end_of_gameweek }, { :transfers => :until_end_of_gameweek }], :conditions => ['gameweeks.start_time <= ? AND (gameweeks.end_time IS NULL OR ? < gameweeks.end_time)', Time.now.utc, Time.now.utc] end </pre>

Thus I can say team.transfers to get a list of all Transfers the Team has made, or team.players to get the current squad.

It works, but that has_many is a pretty unpleasant bit of code and the SQL doesn't look the most efficient. The question is, can this be made more elegant and maybe efficient?

Are you sure it works ? the gameweeks table is joined twice, so the second time it will be aliased, ie you're looking at both the start and the endtime of the from_end_of_gameweek. on top of that while the date range condition may work in development (where code is reloaded on every request) it won't work in production, the value of Time.now used will always be what it was when you last restarted you rails app.

A small change you could make is replace the include with :joins => {:transfers => [:from_end_of_gameweek, :until_end_of_gameweek]} This barely changes the sql generated but it means that rails won't go off and insantiate objects for the transfers/gameweeks.

You might be able to at least make some slightly more readable code by having a active_transfers association (those transfers which are currently in effect) and then having players through that association.

It doesn't look to me like the query could be a lot simpler than it is (as usual having the right indexes can make a world of difference) Perhaps you could side step it by having appropriate flags on the transfers table and a cron job that updated those flags at the end of every gameweek ?

Fred

That Time.now was a bit of a noob mistake, and yes well spotted, some of the conditions were not using the correct alias. Thanks.

Have reworked and it appears okay now but I am still unhappy because of hardcoding table aliases. It also peforms an unncessary join.

class Team < ActiveRecord::Base   has_many :transfers   has_many :players, :through => :transfers

  def players_during_gameweek(gameweek = GameWeek.current)     players.find(:all, :include => [{ :transfers => [:from_end_of_gameweek, :until_end_of_gameweek] }],       :conditions => ['gameweeks.end_time <= ? AND (until_end_of_gameweeks_transfers.end_time IS NULL OR ? < until_end_of_gameweeks_transfers.end_time)', gameweek.start_time, gameweek.start_time])   end end