Improving this messy has_many :through with :include


#1

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:

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

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.


#2

On Feb 6, 9:53 pm, Alan removed_email_address@domain.invalid wrote:

Player becomes part of the Team.

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


#3

On Feb 7, 1:08 pm, Frederick C. removed_email_address@domain.invalid
wrote:

  • I am connecting Teams and Players via a Transfer join model.

IS NULL OR ? < gameweeks.end_time)’, Time.now.utc, Time.now.utc]
Are you sure it works ? the gameweeks table is joined twice, so the

Fred

Thanks.

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