HiveBrain v1.2.0
Get Started
← Back to all entries
patternsqlMinor

Counting penalties for each player by joining tables, where some of the data is null

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
tablesthecountingeachplayerpenaltieswherenullforsome

Problem

I have a table full of player names:

I also have a table full of player penalties:

I have to list all of the players names, player numbers, and the number of penalties that they've incurred.

To do that, I'm using this code:

select players.playerno, name, ifnull(`Penalty count`, 0) as `Penalty count` from players
left join
(select players.playerno, count(players.playerno) as `Penalty count` from players, penalties
where players.playerno = penalties.playerno
group by players.playerno) as `query1`
on players.playerno = query1.playerno
order by playerno


The code works. Here's the output table:

It seems like a lot of code just to merge two tables together. Is there a better way to do it?

I've also read online that keeping the NULL is generally a better practice than changing it to zero, but would that apply in this case, since it's supposed to be the total number of penalties? It seems like NULL would give the impression of "no data" or "unknown data", but in this case the number is known to be zero.

Solution

You should stay away from pre-ANSI92 join syntax, as it is more error-prone and more difficult to read. It's better to use explicit join syntax in all cases.

This join:

from players, penalties
where players.playerno = penalties.playerno


Should be written as:

from players
join penalties
  on players.playerno = penalties.playerno


The primary reasons for this are:

-
Avoid accidental cross/Cartesian joins if you forgot a WHERE clause.

-
Easier to read, since your join conditions are isolated in the ON clause instead of being mixed in with your filtering criteria in the WHERE clause.

Use meaningful table aliases to shorten column references. For instance, players AS ply and penalties AS pen would make for good aliases. Do avoid using single-letter aliases like p though, for readability.

select ply.playerno, ply.name, count(pen.playerno)
from players AS ply
left join penalties AS pen 
...


Use vertical white space (i.e. new lines) to make statements, in particular SELECT clauses, easier to read:

select
    ply.playerno,
    ply.name,
    count(pen.playerno) as `Penalty count`
from ...


MySQL uses all lowercase for all table, columns, etc. identifiers, so using UPPER CASE for keywords makes it easier to tell SQL keywords from identifiers. Also, name is a reserved SQL keyword, so when you have a column called name (or any other SQL keyword) it should be quoted with back-ticks like:

SELECT ply.playerno, ply.`name` FROM players AS ply


There is a much simpler way to aggregate this than how you have done it, with no need for a nested sub-query with an additional join. COUNT(), like other SQL aggregate functions, will take care of handling potentially NULL aggregates to zero, as long as they are included in the query (in this case by using LEFT JOIN instead of INNER JOIN), so we can get the same result much more simply like this:

SELECT
    ply.playerno,
    ply.`name`,
    COUNT(pen.playerno) AS `Penalty count`
FROM players AS ply
LEFT JOIN penalties AS pen
    ON ply.playerno = pen.playerno
GROUP BY
    ply.playerno,
    ply.`name`
ORDER BY
    ply.playerno;

Code Snippets

from players, penalties
where players.playerno = penalties.playerno
from players
join penalties
  on players.playerno = penalties.playerno
select ply.playerno, ply.name, count(pen.playerno)
from players AS ply
left join penalties AS pen 
...
select
    ply.playerno,
    ply.name,
    count(pen.playerno) as `Penalty count`
from ...
SELECT ply.playerno, ply.`name` FROM players AS ply

Context

StackExchange Code Review Q#157759, answer score: 3

Revisions (0)

No revisions yet.