12
\$\begingroup\$

I am designing a new application to track sports statistics with code-first migration. I have the bare minimum POCO setup. The problem is that I am not happy with the design specifically how the players, teams and tournaments are relating to each other.

I appreciate any comments and suggestions.

PlayerAccount class:

/// <summary>
/// Player account is a separate entity from the actual user account.
/// Player account allows a user to join teams and participate in a tournament.
/// </summary>
public class PlayerAccount
{
    [Key]
    public int Id { get; set; }
    [Required]
    public string FirstName { get; set; }
    [Required]
    public string LastName { get; set; }

    /// <summary>
    /// TournamentPlayerKeys tells us which team this player is competing with 
    /// in a certain tournament.
    /// </summary>
    public virtual ICollection<PlayerTeamKey> PlayerTeamKeys { get; set; }
    public virtual ICollection<PlayerMatchStat> PlayerMatchStats { get; set; }
}

Team class:

using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

/// <summary>
/// Team can be formed by a single user or a group of user. You'll need a team to join a tournament.
/// A single user can form his own team. 
/// </summary>
public class Team
{
    [Key]
    public int Id { get; set; }
    [Required]
    public string Name { get; set; }

    public virtual ICollection<PlayerTeamKey> PlayerTeamKeys { get; set; }
    public virtual ICollection<TeamTournamentKey> TeamTournamentKeys { get; set; }
}

Tournament class:

using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;

/// <summary>
/// Tournaments are basically a collection of games in one event. 
/// For example, Summer 2014 Basketball League.
/// </summary>
public class Tournament
{
    [Key]
    public int Id { get; set; }
    [Required]
    public string Name { get; set; }

    public virtual ICollection<TeamTournamentKey> TeamTournamentKeys { get; set; }
    public virtual ICollection<TournamentMatch> TournamentMatches { get; set; }
}

On a tournament there are multiple instances of matches (games):

/// <summary>
/// Match represents one occurence of a game in a league.
/// </summary>
public class TournamentMatch
{
    [Key]
    public int Id { get; set; }
    [Required]
    public int Tournament_Id { get; set; }

    [ForeignKey("Tournament_Id")]
    public virtual Tournament Tournament { get; set; }
    public virtual ICollection<TournamentMatchResult> TournamentMatchResults { get; set; }
}

And of course we have multiple instances of match results in one match. The Score property determines who is the winner.

/// <summary>
/// This is the result of a league match.
/// </summary>
public class TournamentMatchResult
{
    [Key]
    public int Id { get; set; }
    [Required]
    public int Match_Id { get; set; }
    [Required]
    public int Team_Id { get; set; }
    [Required]
    public double Score { get; set; }

    [ForeignKey("Match_Id")]
    public virtual TournamentMatch TournamentMatch { get; set; }
    [ForeignKey("Team_Id")]
    public virtual Team Team { get; set; }
    public virtual ICollection<PlayerMatchStat> PlayerMatchStat { get; set; }

}

I have a multipart key that creates the relationship between a player and a team:

/// <summary>
/// This is a multipart key class that defines a player as part of a team.
/// Mind the naming convension, from smaller entity to a bigger entity; Player > Team.
/// </summary>
public class PlayerTeamKey
{

    [Required]
    public int PlayerAccount_Id { get; set; }
    [Required]
    public int Team_Id { get; set; }

    [ForeignKey("PlayerAccount_Id")]
    public virtual PlayerAccount Player { get; set; }
    [ForeignKey("Team_Id")]
    public virtual Team Team { get; set; }

}

And I have another like that for team and tournament:

/// <summary>
/// This is a multipart key class that is a combination of a team id and a tournament id.
/// This basically tells us that the team with the given id is going to participating on a tournament with the given id.
/// </summary>
public class TeamTournamentKey
{
    [Required]
    public int Team_Id { get; set; }
    [Required]
    public int Tournament_Id { get; set; }

    [ForeignKey("Team_Id")]
    public virtual Team Team { get; set; }
    [ForeignKey("Tournament_Id")]
    public virtual Tournament Tournament { get; set; }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ It's been a while since I've used EF but I don't recall having to create my own model for a N-N relationship. I'll take a look into it tonight after my Civ 5 game but I believe you can indeed make that much cleaner. \$\endgroup\$ – Jeroen Vannevel Jul 12 '14 at 16:36
10
+50
\$\begingroup\$

Don't use underscores in identifiers. If that's how the database has them, use a ColumnAttribute to specify the column name scratch that, you're going code-first. This:

public int Tournament_Id { get; set; }

public int Match_Id { get; set; }
public int Team_Id { get; set; }

public int PlayerAccount_Id { get; set; }

Is much more seesharpesque like this:

public int TournamentId { get; set; }

public int MatchId { get; set; }
public int TeamId { get; set; }

public int PlayerAccountId { get; set; }

I like that most of these foreign key properties follow a very predictable naming pattern [ForeignEntityTypeName]Id - IIRC that's all EF needs to be able to infer the foreign keys.

...With the exception of TournamentMatchResult, which breaks the pattern with its MatchId (/Match_Id) property:

public int Match_Id { get; set; }
public virtual TournamentMatch TournamentMatch { get; set; }

Would be more consistent like this:

public int TournamentMatchId { get; set; }
public virtual TournamentMatch TournamentMatch { get; set; }

You're using System.ComponentModel.DataAnnotations;; it works, but I find all these attributes pollute the POCO:

[Key]
public int Id { get; set; }
[Required]
public int Match_Id { get; set; }
[Required]
public int Team_Id { get; set; }
[Required]
public double Score { get; set; }

[ForeignKey("Match_Id")]
public virtual TournamentMatch TournamentMatch { get; set; }
[ForeignKey("Team_Id")]
public virtual Team Team { get; set; }

EF is smart enough to know Id is your [Key] (by naming convention), and smart enough to know Team is a FK that uses TeamId (by naming convention) - I'm not sure whether Team_Id breaks it or not, but relying on EF's established conventions makes your POCO code less polluted:

public int Id { get; set; }

[Required]
public int TournamentMatchId { get; set; }

[Required]
public int TeamId { get; set; }

[Required]
public double Score { get; set; }

public virtual TournamentMatch TournamentMatch { get; set; }
public virtual Team Team { get; set; }

Now all that's left is [Required] attributes. You could remove them too, if you moved POCO configurations/mappings elsewhere.


One way of doing that would be in the OnModelCreating override of your DbContext:

modelBuilder.Entity<TournamentMatchResult>()
            .Property(t => t.TournamentMatchId).IsRequired();
modelBuilder.Entity<TournamentMatchResult>()
            .Property(t => t.TeamId).IsRequired();
modelBuilder.Entity<TournamentMatchResult>()
            .Property(t => t.Score).IsRequired();

Obviously this gets very verbose, very fast. An alternative is to use EntityMappingConfiguration<TEntityType> classes. For each entity type, you create a mapping class:

public class TournamentMatchResultMap : EntityMappingConfiguration<TournamentMatchResult>
{
    public TournamentMatchResultMap()
    {
        Property(t => t.TournamentMatchId).IsRequired();
        Property(t => t.TeamId).IsRequired();
        Property(t => t.Score).IsRequired();
    }
}

You can also use these mapping classes to configure mapped column names and table, and relationships with other entities. See EntityMappingConfiguration<TEntityType>

Then the OnModelCreating override simply loads all the configurations:

modelBuilder.Configurations.Add(new TournamentMatchResultMap());

I find that's the cleanest approach; it lets your POCO classes be POCO's, it leaves the OnModelCreating method free of verbose fluent API mappings, and it puts each entity's configurations into its own class.


Let's look at the relationships. If I didn't make mistakes, this is what your code can be modeled as:

http://yuml.me/edit/5090cace

As @Jeroen explains, EF doesn't need junction tables to model a many-to-many relationship; PlayerTeamKey and TeamTournamentKey entities can be removed.

I would believe this simplified model could very well suit your needs:

http://yuml.me/edit/15eb9eca

The idea is to use the navigation properties to navigate between entities - from a business standpoint it makes sense for tournament matches to know what teams are involved, but from a data standpoint, it doesn't: when your application creates a new tournament, it also creates a TournamentMatchResult for all teams involved, with each team's score at 0 - and that's enough for a TournamentMatch to know what teams are playing.

As the game evolves, you can then update the TournamentMatchResult records, and if a TournamentMatch needs to know what teams are involved, it can easily find out by selecting its .TournamentMatchResults.Select(result => result.Team).

Notice this results in simple, easy-to-follow, one-to-many relationships everywhere.

\$\endgroup\$
  • \$\begingroup\$ What is the meaning of arrows in your diagram? I beleive the arrow should signify "A uses B" and thus should be drawn in oposite direction, e.g. TournamentMatchResult uses Team. \$\endgroup\$ – Vojtěch Dohnal Jul 14 '14 at 7:45
  • \$\begingroup\$ "One to many". The "many" end has a star (*). Maybe they are reversed, UML isn't my cup of tea. But one gets the idea I'd guess. \$\endgroup\$ – Mathieu Guindon Jul 14 '14 at 11:06
  • \$\begingroup\$ FWIW I've drawn the arrows based on the yUML "tutorial": yuml.me/diagram/class/draw \$\endgroup\$ – Mathieu Guindon Jul 14 '14 at 11:11
  • \$\begingroup\$ Are you sure that all those associations should be really "shared aggregations"? It does not make sense to me. Most common types of associations can be seen here: msdn.microsoft.com/en-us/library/dd409437%28VS.100%29.aspx. And when I think about it - is it logical for TournamentMatchResult to belong to many different Tournaments? I beleive it is more common for a Match to belong to only one Tournament... \$\endgroup\$ – Vojtěch Dohnal Jul 14 '14 at 21:47
  • 1
    \$\begingroup\$ Ok, thanks, I got it, it makes sense this way. It is based on a presumption, that N teams can participate in one match. Still shared aggregation is not the right assoication to be used here. \$\endgroup\$ – Vojtěch Dohnal Jul 15 '14 at 8:02
9
\$\begingroup\$

I made a small example to show you that all your worries are in the past. Before you implement this yourself though, keep Mat's remarks in mind: I made this as a quick sketch and separating the mappings from the context, naming conventions, etc are important for your code's clarity.

That being said, this small setup shows you how you can change it:

public class Player
{
    public int Id { get; set; }
    public string Firstname { get; set; }
    public string Lastname { get; set; }

    public virtual ICollection<Team> Teams { get; set; }
}

public class Team
{
    public int Id { get; set; }
    public string Name { get; set; }

    public virtual ICollection<Player> Players { get; set; }
}

That's it. You're done! All we need to do now is configure our mapping so it will generate the intermediate table itself:

public class TournamentContext : DbContext
{
    public DbSet<Player> Players { get; set; }
    public DbSet<Team> Teams { get; set; }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Player>()
            .HasKey(x => x.Id)
            .HasMany(x => x.Teams);

        modelBuilder.Entity<Team>()
            .HasKey(x => x.Id)
            .HasMany(x => x.Players);
    } 
}

If you want to test it out yourself, you can use this sample code:

static void Main(string[] args)
{
  using (var context = new TournamentContext())
  {
      context.Database.ExecuteSqlCommand("delete from Players");
      context.Database.ExecuteSqlCommand("delete from Teams");

      var team1 = context.Teams.Add(new Team {Name = "Power rangers"});
      var team2 = context.Teams.Add(new Team { Name = "Ninja turtles" });

      context.Players.Add(new Player
      {
          Firstname = "John",
          Lastname = "Johnson",
          Teams = new List<Team> {team1, team2}
      });

      context.Players.Add(new Player
      {
          Firstname = "Jack",
          Lastname = "Jackson",
          Teams = new List<Team>{team1}
      });

      context.SaveChanges();

      foreach (var player in context.Players.ToList())
      {
          Console.WriteLine("Player: " + player.Id);
          Console.WriteLine("Teams: " + string.Join(", ", player.Teams.Select(x => x.Name).ToArray()));
      }
  }

  Console.Read();
}

Looking at our database using the Server Explorer we now see these tables:

enter image description here

enter image description here

enter image description here

With this output:

enter image description here

You'll find that you can configure this a lot and add restrictions to the table that gets generated but I'm too lazy to look them up myself; mapping isn't my best side. This should give you an idea though about how you can get rid of those not-so-pretty intermediate tables.

\$\endgroup\$
6
\$\begingroup\$

I suggest to use the same name for primary and foreign keys following some consistent pattern:

It could be e.g. _id or Id, so you will have in PlayerAccount table the primary key PlayerAccountId (instead of just Id) and in PlayerTeamKey also PlayerAccountId as a foreign key.

Naming of fields and classes is very important in non-small projects.

I believe it does not matter whether you follow code-first or model-first or whatever approach, your problem is propper class design and naming. I personally like very much code-second pattern using PowerToys - you can do your data diagram 1] in a object design tool like Enterprise Architect 2] in SQL database like SQL Server. Then you export this design as code-first using PowerToys.

You must organize the terms you use and convert them to meaningful class design, which is not allways easy.

Here is the diagram of classes you presented: Diagram of your classes

You can create quite easily this class diagram in Visual Studio. Appearently there is missing Match class. (Match_Id - foreign key) (OK it is not missing, it is a link to the very suspicious TournamentMatch class, I get it)

Your basic classes are Player (originally PlayerAccount), Team, Match (originally TournamentMatchResult), Tournament. I have renamed the classes - it is more comprehensible this way I think. The other classes are empty association classes that are used to create M:N relation. Are all those M:N relations really necessary and OK?

There are too many unanswered questions

enter image description here

Basically your main problem is that in your design you have too many atribute-less association classes (PlayerTeamKey, TournamentMatch ... I see 4 of them), which is very probably error in class design.

You should first answer the kind of questions, which I wrote into second diagram, and then and only then start writing your code. Otherwise your design will fail.

EDIT:

To give an example how you can make proper design by answering analytical questions, I have created simplified version. You can as I believe, create your model with just 4 classes depending on how you answer the analytical questions. This model can be easily convereted to C# code first, but firts confirm whether this is the model you seek.

SimplifiedVersion

One more hint: if you use word Match for a match, do not call it Game, use just as few terms as it is absolutely necessary.

\$\endgroup\$
  • \$\begingroup\$ There's no Match entity - it's TournamentMatch: I almost fell in the same trap, it's the name of the foreign key property (Match_Id) that's confusing. \$\endgroup\$ – Mathieu Guindon Jul 12 '14 at 18:36
  • \$\begingroup\$ In that case TournamentMatch class should be deleted and replaced by composition 1:N Tournament - TournamentMatchResult. The names of classes are misleading which should be corrected. \$\endgroup\$ – Vojtěch Dohnal Jul 12 '14 at 18:41
  • \$\begingroup\$ You're right, TournamentMatch is not needed. \$\endgroup\$ – Mathieu Guindon Jul 16 '14 at 18:42

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.