2
\$\begingroup\$

I am trying to model information around football teams and fixtures. I am using the Premier League fantasy football API for my data. The fixtures API is here, and the team data (along with a lot of extra data) is here - see the "teams" element.

I want to be able to do things like report on all of Arsenal's fixtures/results, for example...

ARSENAL
event kickoff      opponent         venue result scoreline
1     12 aug 16:00 Manchester City  H     L      1-2
2     18 Aug 17:30 Chelsea          A     L      2-3
3     25 Aug 15:00 West Ham         H     -

However I may want to access more data items than shown here - it's just an example.

I'm experienced with database, so if I were writing this to a normalized database I would have two tables, teams and fixtures, and I could write a query like this.

SELECT t.team_name, f.[event], f.kickoff, opp.name,
       CASE WHEN t.team_id = f.home_team_id THEN 'H' ELSE 'A' END AS venue,
       CASE WHEN f.team_h_score IS NULL THEN '-'
            WHEN f.team_h_score = f.team_a_score THEN 'D'
            WHEN f.team_h_score > f.team_a_score AND t.team_id = f.home_team_id THEN 'W'
            WHEN f.team_h_score < f.team_a_score AND t.team_id = f.away_team_id THEN 'W'
            ELSE 'L' END AS result,
       CASE WHEN t.team_id = f.home_team_id THEN f.team_h_score ELSE team_a_score END as own_score,
       CASE WHEN t.team_id = f.home_team_id THEN f.team_a_score ELSE team_h_score END as opp_score       
FROM   team t
INNER JOIN fixture f ON t.team_id IN (f.away_team_id, f.home_team_id)
INNER JOIN team opp ON opp.team_id IN (f.away_team_id, f.home_team_id)
WHERE t.team_id = @input_team_id

I would be happy with this - it "feels" pretty good and I trust my feelings - I know there are rules and principles that I am following but they have been internalised such that I can usually "feel" what is right in database design.

However, when I'm trying to write this in an object-oriented way, I have much less experience so I'm trying to follow guidelines and principles of object-oriented design - SRP, managing dependencies and so on.

I decided that starting from the SQL above is a decent way to start. So, here's my thinking so far. There obviously needs to be a team class and a fixture class. I also thought a TeamFixture class would be good too - and can encapsulate some of the logic in the long case statements above.

class Databank
  def initialize
    #some stuff outside the scope of this review
    #just assume the fixtures and teams methods work
  end

  def fixtures
    #returns array of all Fixtures
  end

  def teams
    #returns array of all Teams
  end

end

class Fixture
  attr_reader :data
  def initialize(data)
    @data = data
  end
end

class Team
  attr_reader :data, :fixtures
  def initialize(data, fixtures)
    @data = data
    @fixtures = fixtures.map { |f| TeamFixture.new(f, @data['id']) }
  end
end

class TeamFixture
  attr_reader :fixture, :venue
  def initialize(fixture,team_id)
    @fixture=fixture
    @venue = fixture.data['team_h']==team_id ? 'H' : 'A'
    @h_score = fixture.data['team_h_score']
    @a_score = fixture.data['team_a_score']
  end

  def result
    return '-' if @h_score=='null'
    return 'D' if @h_score==@a_score
    winner = @h_score==@a_score ? 'H' : 'A'
    return @venue==winner ? 'W' : 'L'
  end

  def own_score
    @venue=='H' ? @h_score : @a_score
  end

  def opp_score
    @venue=='H' ? @a_score : @h_score
  end

  def opponent
    id = @venue=='H' ? @fixture['team_a'] : @fixture['team_h']
    # How to get from this id to the Team object it represents?
  end
end

I'm left asking myself three questions that I'm not sure how to answer. Though feel free to make any other comments.

  1. If I want to access information about an opponent, I feel like I'm "reaching through" many objects, for example to print every fixture for every team it would be something like this...

    d = Databank.new
    d.teams.each |t| do
      puts "#{t.data['name']} are playing these teams..."
      t.fixtures.each |tf| do
        puts "#{tf.fixture.data['kickoff_time_formatted']} - #{tf.opponent.data['name']}"
      end
    end
    

    So effectively I'm doing Databank → teams → teamFixture → opponent.data albeit across a few lines. The Law of Demeter states that this is not usually a good thing, but I'm not experienced enough to know if this is a good violation or not? My database voice in my head says this is fine but the OOP voice isn't so sure.

  2. I don't like the Team and Fixture classes, having to write t.data['name'] feels clunky and that it would be easier to just call t['name'], t[:name] or t.name. However looking around I can't seem to find a "good" way to achieve this. I understand that subclassing a Hash is a bad idea. Then there's this Stackoverflow question, which suggests either an OpenStruct (which wouldn't work for the nested nature of some of my data) or some ugly-looking code to achieve this. Am I right in wanting something like t['name'] to work, and if so how should I go about it?

  3. The opponent method is not finished. What is the best way to get it to return the appropriate Team object? If I had an Enumerable of the teams I could do teams.find { |t| t["id"]==id} but such an Enumerable is only in the Databank object, so it would be a lot of reaching through objects again and it does not feel right at all to do this.

\$\endgroup\$
1
\$\begingroup\$
  1. Reaching through is not necessarily bad, although it's better to ask an object for its data rather than to going and getting it yourself.

  2. You can fix the Team class to have regular attributes, or at least methods to retrieve them from a database result. So this class can have a name method, def name; data['name']; end; at the very least. FWIW, the alarms go off when I see data as a name in code. Most things are data... anyway...

  3. As David Aldridge suggested, an OR mapping tool may help-- but not absolutely necessary. For scaling, I wonder whether you want a team to load all its fixtures-- or perhaps it can just load the IDs of it's fixtures and let the fixture class take care of loading the details of the fixture. And OR tool provides lots of help here.

As far as OO, think about what data really belongs in a class-- from a concrete, real-world perspective. A fixture/match has a few things: a location, time, home team and away team, and eventually a score. I think you're missing that class. For OO design, it's best to start out with factual, non-abstract elements until you have a good grasp of the problem. Premature abstraction is this quickest way to confusion.

But also draw hard boundaries about what a class is responsible for. Does a team need to know the scores of all its matches? Probably not.

I personally don't like all the if statements based on home/away. This complexity in extracting information about a match based on your perspective, home or away, could be simplified with a small abstraction. I'd consider a decorator or bridge pattern to handle help with this. Here's one idea:

You can have a class that allows you to look at a match as the home team, or the visiting team. ...fixture home = AsHome.new(fixture) home.result home.own_score home.opponent_score home.opponent away = AsAway.new(fixture) ... I'm not sure exactly what this looks like, but it gets rid of some of the messiness of the TeamFixture class.

\$\endgroup\$
0
\$\begingroup\$

I should say that your work might get easier here if you are using an ORM such as ActiveRecord or Squeel or Sequel.

I think that the TeamFixture may be incorrect there.

Fixture feels like an object, with attributes of home_team_id and away_team_id, and 'home_team_score' and 'away_team_score'.

This allows you to report fixtures for all teams and also whether they are playing home or away.

You would probably have two associations, home_fixtures and away_fixtures, and could also construct an association for fixtures by hand-writing the join condition, I think.

It's fine to reach through an object to another, and a through association allows you to do that without referencing the intermediate object if you like. But you can also eager load the opposing team as well to get access to both the fixture attributes and the opposing team attributes.

I suppose that where a TeamFixture object might come into play is when you want to define attributes for the team in the context of that one fixture. For example, you might want to list players, with a TeamFixturePlayer model that has stats on who scored, who got a yellow or red card, playing time etc.

\$\endgroup\$
  • \$\begingroup\$ Fixture is already an object - the team ID's and scores (and much more) are available in there inside the data hash. So if I want to give a list of fixtures I can easily do that currently. TeamFixture isn't strictly necessary I agree - but it does make things easier - or at least appears to - by having certain bits of logic live there. I'm not 100% sure what you mean by associations? Can you elaborate? \$\endgroup\$ – WelshGandalf Aug 21 '18 at 21:58
  • \$\begingroup\$ ref. associations I was thinking of the Rails/ActiveRecord approach to relating models, through associations. If you were working with database-backed classes then something like that would make sense to me. \$\endgroup\$ – David Aldridge Aug 22 '18 at 14:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.