Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As an exercise, I was asked to design a database schema (for MS SQL Server) for a tabletop game. The requirements were simple: players compete in matches and there are specific match types (e.g. 1v1 or 2v2). A hypothetical web site that would make use of this database would include things like player match history, a scoreboard page, and a leaderboard page.

My initial design had some flaws and inconsistencies, but after thinking more on it, I came up with the following schema:

create table Players (
    Id int identity(1, 1) primary key,
    PlayerName nvarchar(50) not null
);

create table MatchTypes (
    Id tinyint primary key,
    Name nvarchar(50) not null
);

create table Matches (
    Id int identity(1, 1) primary key,
    MatchTypeId tinyint not null references MatchTypes(Id),
    IsComplete bit not null,
    CompletionDate datetime2 null
);

create table Teams (
    Id int identity(1, 1) primary key,
    MatchId int not null references Matches(Id),
    Score smallint not null,
    IsWinningTeam bit null
);

create table PlayerTeams (
    PlayerId int not null references Players(Id),
    TeamId int not null references Teams(Id)
);

Although the design is fairly simple, I'm sure I've made sub-optimal design decisions. What could be improved on?

In addition, I went ahead and wrote a SQL query to extract data for a hypothetical "Player Match History" page. The goal is something like the following:

Player: Foo
Matches:
  Against Bar                  ... Won!
  Against Quux                 ... Lost
  With Baz, against Bar & Quux ... Lost
  With Bar, against Quux & Baz ... Won!

Essentially, given a player, select all matches that the player participated in, and display them with opponents/teammates.

Here's the query, although I feel like it may be unnecessarily complex.

select pm.MatchId,
    t.Id as TeamId,
    p.Id as PlayerId,
    p.PlayerName,
    case when t.Id = pm.TeamId then 1 else 0 end as IsTeammate,
    pm.Won
from (
    select m.Id as MatchId,
        m.CompletionDate,
        t.Id as TeamId,
        t.IsWinningTeam as Won
    from Players p
    inner join PlayerTeams pt
        on p.Id = pt.PlayerId
    inner join Teams t
        on pt.TeamID = t.Id
    inner join Matches m
        on t.MatchId = m.Id
    where m.IsComplete = 1 and p.Id = 1
) pm
inner join Teams t
    on pm.MatchId = t.MatchId
inner join PlayerTeams pt
    on t.Id = pt.TeamId
inner join Players p
    on pt.PlayerId = p.Id
where p.Id != 1
order by pm.CompletionDate desc
share|improve this question
    
One of the requirements for questions on this site is that the posted code must work. If it's "not tested", then how can it be known whether or not it works? –  Donald.McLean Aug 1 at 4:15
    
@Donald.McLean: You're right, I apologize. I wrongly assumed that since I was asking "it's not working, please fix," that it would be irrelevant whether it was actually tested. I tested it, and it turns out that it did contain some errors. –  voithos Aug 1 at 4:22
    

3 Answers 3

up vote 4 down vote accepted

Good work on schema

Overall, I think your schema is clean, easy to follow, and makes sense (in the limited scope of your scenario). Granted the overarching or "whole" schema for something like this would be much larger, but if you kept following this sort of normalization it should be fine.

Aliases

I see at least 5 unclear aliases: p, t, m, pt, pm. Mr. Maintainer would have to read through your entire script to even grasp what these stand for. Your column names are short enough, I really don't think aliases are of much use, and they just make your script more confusing to review. Imagine this kind of notation used on 1000 lines of codes in a real business setting, this should drive my point home.

Common Table Expressions

You have a rather large subquery (which I will address next point) and in SQL Server and several other RDBMS, you can simplify the way it reads by using a CTE. So this:

from (
    select m.Id as MatchId,
        m.CompletionDate,
        t.Id as TeamId,
        t.IsWinningTeam as Won
    from Players p
    inner join PlayerTeams pt
        on p.Id = pt.PlayerId
    inner join Teams t
        on pt.TeamID = t.Id
    inner join Matches m
        on t.MatchId = m.Id
    where m.IsComplete = 1 and p.Id = 1
) pm

You could instead write, at the very beginning of your script:

with pm as (
        select m.Id as MatchId,
            m.CompletionDate,
            t.Id as TeamId,
            t.IsWinningTeam as Won
        from Players p
        inner join PlayerTeams pt
            on p.Id = pt.PlayerId
        inner join Teams t
            on pt.TeamID = t.Id
        inner join Matches m
            on t.MatchId = m.Id
        where m.IsComplete = 1 and p.Id = 1
    )
select /* bunch of work here */
from pm    -- reference CTE in main query as if it were a table

Redundancy

I noticed your subquery (or CTE) is joining some of the same stuff your main query is doing. Look:

from (
    select m.Id as MatchId,
        m.CompletionDate,
        t.Id as TeamId,
        t.IsWinningTeam as Won
    from Players p
    inner join PlayerTeams pt
        on p.Id = pt.PlayerId  -- (1)
    inner join Teams t
        on pt.TeamID = t.Id  -- (2)
    inner join Matches m
        on t.MatchId = m.Id
    where m.IsComplete = 1 and p.Id = 1
) pm
inner join Teams t
    on pm.MatchId = t.MatchId
inner join PlayerTeams pt
    on t.Id = pt.TeamId  -- (2)
inner join Players p
    on pt.PlayerId = p.Id  -- (1)

Best to avoid redundant operations, especially expensive join's, as it makes the execution slower. Only one of each should be needed in most cases.

Nitpick

This preferably should not be used: where p.Id != 1

Instead, use this: where p.Id <> 1

Although both work just the same, != is not ANSI-92 standard.

share|improve this answer
2  
Thank you for your answer - you bring up good points regarding aliases, as well as the use of a CTE. My habit has been to only resort to a CTE when there are repeated subqueries, but it makes sense to use it to aid readability. As for the redundancy, it exists because the query essentially needs to follow the joins from Players to Matches and back to Players, in order to select all teammates/opponents. I couldn't figure out a more straightforward way to do this. –  voithos Aug 1 at 5:45

Do you need both IsComplete and CompletionDate on Matches? If the Match has a CompletionDate that's not null, it's safe to say that the match has been completed and IsComplete is redundant data. A boolean to indicate which team won/lost is also redundant data (if team 1 won, then team 2 obviously lost). If the winning team is always the one with the highest score, then IsWinningTeam is also redundant since it can be derived.

You're not using MatchTypes for anything.

There's nothing preventing 1 player from playing in the same match multiple times (player 1 vs player 1?), same goes for teams (team 1 vs team 1?).

This is how I would design the tables (pseudocode):

Players
    id not null
    name not null

Teams (optional)
    name not null -- (contains generic values like Home & Visitor, or Red, Blue, and Green)

Matches
    id not null
    completed_time
    winner

    (id, winner) references MatchTeams (match_id, name)

MatchTeams
    match_id not null
    team not null
    score

    primary key (match_id, team)
    (team) references teams (name)

MatchParticipants (or MatchPlayers)
    match_id
    team
    player_id

    unique (match_id, player_id)
    (match_id, team) references MatchTeams (match_id, team)

The query would then look like this (You'll have to forgive me if the syntax isn't quite right here, I'm familiar with PostgreSQL, not MS SQL):

SELECT
    team,
    player_id,
    Players.name,
    player_team = team AS IsTeammate,
    player_was_winner
FROM
    -- if you need to limit this to X most recent matches,
    -- you do it in the subquery
    (   SELECT
            match_id
            Matches.winner = MatchParticipants.team AS player_was_winner
            team AS player_team
        FROM
            MatchParticipants
            JOIN Matches ON Matches.id = match_id
        WHERE
            user_id = ?
            AND CompletedDate IS NOT NULL) AS SelectedPlayerMatches
    JOIN MatchParticipants USING (match_id)
    JOIN Players ON Players.id = MatchParticipants.player_id;

Personally, I'd drop using the query to pick out the player's teammates and whether or not the player won the match and leave that to the portion of the application that will handle the rendering. If there's a chance that your game will allow 3+ teams, then the user might prefer to know which team won the match rather than just knowing that they didn't win.

share|improve this answer

Have you considered using views:-

-views gives you an explicit way to reduce the complexity of code. so your whole line of code will drop to only few lines. you can replace each subquery with the same view name (or named common table expression as Phrancis metioned) then the optimizer knows it's the same thing and can either cache the result or restructure the execution path so much better than inner query

 Create VIEW vw_GetMatchStatistics
 AS
 SELECT  m.Id AS MatchId ,
            m.CompletionDate ,
            t.Id AS TeamId ,
            t.IsWinningTeam AS Won ,
            IsComplete ,
            p.Id AS PlayerId ,
            p.PlayerName
 FROM    Players p
            INNER JOIN PlayerTeams pt ON p.Id = pt.PlayerId
            INNER JOIN Teams t ON pt.TeamID = t.Id
            INNER JOIN Matches m ON t.MatchId = m.Id


SELECT  pm.MatchId ,
    vw.TeamId ,
    vw.PlayerId ,
    vw.PlayerName ,
    CASE WHEN pm.TeamId = vw.TeamId THEN 1
         ELSE 0
    END AS IsTeammate ,
    pm.Won
   FROM    ( SELECT    MatchId ,
                CompletionDate ,
                TeamId ,
                Won
      FROM      vw_GetMatchStatistics
      WHERE     IsComplete = 1
                AND PlayerId = 1
    ) pm
    INNER JOIN vw_GetMatchStatistics vw ON pm.matchId = vw.MatchId
    WHERE   vw.PlayerId != 1
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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