# Bad practices in this procedure to calculate player statistics?

I recently self-taught myself MySQL as quickly as possible because it was needed for a project that I'm working on. Now that I've got some time I've been working on improving some thrown together SQL procedures. I've already done a few things, like logic improvements and removal of sub-queries, but I'm wondering if anyone can quickly skim through the procedure and point out anything specifically bad that I might have missed.

CREATE PROCEDURE userstatsUpdate(IN userauth VARCHAR(32))
BEGIN

DECLARE mapcount INT DEFAULT 0;
DECLARE stagecount INT DEFAULT 0;
DECLARE bonuscount INT DEFAULT 0;

DECLARE mapmult FLOAT DEFAULT 0;
DECLARE stagemult FLOAT DEFAULT 0;
DECLARE bonusmult FLOAT DEFAULT 0;

DECLARE pc FLOAT DEFAULT 0;

DECLARE stagerecs INT DEFAULT 0;
DECLARE bonusrecs INT DEFAULT 0;
DECLARE maprecs INT DEFAULT 0;
DECLARE tops INT DEFAULT 0;

DECLARE p_comps FLOAT DEFAULT 0;
DECLARE p_tops FLOAT DEFAULT 0;
DECLARE counter INT DEFAULT 0;
DECLARE ranknum INT DEFAULT 0;

DECLARE userID INTEGER DEFAULT 0;

SELECT playerID INTO userID FROM cs_players WHERE cs_players.userauth = userauth;

WHILE counter < 10 DO

SET ranknum = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid=cs_maps.mapid WHERE stage = 0 AND rank = counter+1 AND playerid = userID AND active = 1);

SET p_tops = p_tops + (((10-counter)+(10-counter+2)*30)*ranknum);
SET counter = counter + 1;

END WHILE;

SELECT count(*),
COALESCE(
(count(*)*((difficulty+4)/5)),0)
AS points INTO mapcount, mapmult from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND playerid = userID AND cs_maps.active = 1;
SELECT count(*), COALESCE(
(count(*)*((difficulty+4)/5)),0)
AS points INTO stagecount, stagemult  from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 0 AND playerid = userID AND cs_maps.active = 1;
SELECT count(*), COALESCE(
(count(*)*((difficulty+4)/5)),0)
AS points INTO bonuscount, bonusmult from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 1 AND playerid = userID AND cs_maps.active = 1;
SET maprecs = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND rank = 1 AND playerid = userID AND cs_maps.active = 1);
SET stagerecs = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 0 AND rank = 1 AND playerid = userID AND cs_maps.active = 1);
SET bonusrecs = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 1 AND rank = 1 AND playerid = userID AND cs_maps.active = 1);
SET tops = (SELECT count(*) from cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND rank <= 10 AND rank > 0 AND playerid = userID AND cs_maps.active = 1);
SET @TotalStage = (SELECT sum(checkpoints) FROM cs_maps WHERE mtype = 0 AND active = 1);
SET @TotalBonus = (SELECT sum(cp_bonus) FROM cs_maps  WHERE active = 1);
SET @TotalMaps = (SELECT COUNT(*) FROM cs_maps WHERE active = 1);

SET @div1 = ((mapcount*2)+stagecount+(bonuscount*1.5));
SET @div2 = ((@TotalMaps*2)+@TotalStage+(@TotalBonus*1.5));

SET pc = @div1/@div2;

SET p_comps = (mapmult * 50) + (bonusmult * 20) + (stagemult * 5);

INSERT INTO cs_userstats(playerid, c_maps, c_stages, c_bonuses, wr_m, wr_s, wr_b, wr_top, p_top, p_comp, percomp) VALUES (userID, mapcount, stagecount, bonuscount, maprecs, stagerecs, bonusrecs, tops, p_tops, p_comps, pc)
ON DUPLICATE KEY UPDATE c_maps = mapcount, c_stages = stagecount, c_bonuses = bonuscount, wr_m = maprecs, wr_s = stagerecs, wr_b = bonusrecs, wr_top = tops, p_top = p_tops, p_comp = p_comps, percomp = pc;

END$$ • are playerid and userID in the same table? are you looking to see if they are in the same row? – Malachi Apr 11 '14 at 1:44 • Yeah userID is just an INT to store the playerID from the input userauth. It added another query, but it prevents redundant joins to always redetermine the playerID from userauth for every query. Hopefully that made sense. – prmetme Apr 11 '14 at 2:19 • it's hard to see your parameters, I am used to SQL SERVER parameters they always start with a @. you should mark that as a variable/parameter somehow. – Malachi Apr 11 '14 at 2:31 ## 4 Answers the first thing that I see here is that it is going to be hard to read because you don't use standard ... "notation" like indentation and new lines for each column you are selecting in a SELECT statement. all of this is going to matter when you are going to be returning to the code to read it again or maintain it later. where you have this: WHERE stage = 0 AND playerid = userID  the playerid = userID part should really be in a join statement if possible. some of your SET statements look very similar, you should look at these and see if you can't create a Temp Table with them, or even a CTE (I don't remember if this is possible with MySQL or not) • MySQL does not support Common Table Expressions. – 200_success Apr 11 '14 at 2:56 • sounds like TEMP Tables can't be used more than once in a query either – Malachi Apr 11 '14 at 2:58 • Dang that Bug report is 8 years old and they still haven't fixed it. – Malachi Apr 11 '14 at 2:59 • Avoid temporary tables unless there's a good reason to use them. A workaround for the lack of Common Table Expressions is to create views instead. The main drawback is that creating views pollutes the global namespace. – 200_success Apr 11 '14 at 5:21 I have to agree with @Malachi about the lack of indentation. Your code is unreadable because of it. It would also be beneficial to insert a few spaces into the complex arithmetic expressions. Also take care to consistently use UPPERCASE for keywords such as FROM. For your benefit and the benefit of other reviewers, here is your code, with no changes other than indentation and capitalization. That's the first step towards understanding the code and making further improvements. CREATE PROCEDURE userstatsUpdate(IN userauth VARCHAR(32)) BEGIN DECLARE mapcount INT DEFAULT 0; DECLARE stagecount INT DEFAULT 0; DECLARE bonuscount INT DEFAULT 0; DECLARE mapmult FLOAT DEFAULT 0; DECLARE stagemult FLOAT DEFAULT 0; DECLARE bonusmult FLOAT DEFAULT 0; DECLARE pc FLOAT DEFAULT 0; DECLARE stagerecs INT DEFAULT 0; DECLARE bonusrecs INT DEFAULT 0; DECLARE maprecs INT DEFAULT 0; DECLARE tops INT DEFAULT 0; DECLARE p_comps FLOAT DEFAULT 0; DECLARE p_tops FLOAT DEFAULT 0; DECLARE counter INT DEFAULT 0; DECLARE ranknum INT DEFAULT 0; DECLARE userID INTEGER DEFAULT 0; SELECT playerID INTO userID FROM cs_players WHERE cs_players.userauth = userauth; WHILE counter < 10 DO SET ranknum = ( SELECT count(*) FROM cs_times INNER JOIN cs_maps ON cs_times.mapid=cs_maps.mapid WHERE stage = 0 AND rank = counter+1 AND playerid = userID AND active = 1 ); SET p_tops = p_tops + (((10-counter) + (10-counter+2)*30) * ranknum); SET counter = counter + 1; END WHILE; SELECT count(*), COALESCE((count(*) * ((difficulty+4)/5)), 0) AS points FROM cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid INTO mapcount, mapmult WHERE stage = 0 AND playerid = userID AND cs_maps.active = 1; SELECT count(*), COALESCE((count(*) * ((difficulty+4)/5)), 0) AS points FROM cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid INTO stagecount, stagemult WHERE stage > 0 AND cs_times.type = 0 AND playerid = userID AND cs_maps.active = 1; SELECT count(*), COALESCE((count(*) * ((difficulty+4)/5)), 0) AS points INTO bonuscount, bonusmult FROM cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 1 AND playerid = userID AND cs_maps.active = 1; SET maprecs = ( SELECT count(*) FROM cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND rank = 1 AND playerid = userID AND cs_maps.active = 1 ); SET stagerecs = ( SELECT count(*) FROM cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 0 AND rank = 1 AND playerid = userID AND cs_maps.active = 1 ); SET bonusrecs = ( SELECT count(*) FROM cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage > 0 AND cs_times.type = 1 AND rank = 1 AND playerid = userID AND cs_maps.active = 1 ); SET tops = ( SELECT count(*) FROM cs_times INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid WHERE stage = 0 AND rank <= 10 AND rank > 0 AND playerid = userID AND cs_maps.active = 1 ); SET @TotalStage = (SELECT sum(checkpoints) FROM cs_maps WHERE mtype = 0 AND active = 1); SET @TotalBonus = (SELECT sum(cp_bonus) FROM cs_maps WHERE active = 1); SET @TotalMaps = (SELECT count(*) FROM cs_maps WHERE active = 1); SET @div1 = ((mapcount*2) + stagecount + (bonuscount*1.5)); SET @div2 = ((@TotalMaps*2) + @TotalStage + (@TotalBonus*1.5)); SET pc = @div1/@div2; SET p_comps = (mapmult * 50) + (bonusmult * 20) + (stagemult * 5); INSERT INTO cs_userstats(playerid, c_maps, c_stages, c_bonuses, wr_m, wr_s, wr_b, wr_top, p_top, p_comp, percomp) VALUES (userID, mapcount, stagecount, bonuscount, maprecs, stagerecs, bonusrecs, tops, p_tops, p_comps, pc) ON DUPLICATE KEY UPDATE c_maps = mapcount, c_stages = stagecount, c_bonuses = bonuscount, wr_m = maprecs, wr_s = stagerecs, wr_b = bonusrecs, wr_top = tops, p_top = p_tops, p_comp = p_comps, percomp = pc; END$$

• Thanks. I sort of learned from unindented code so the bad habits just stuck with me, but yeah that definitely looks a lot better. I'll try to keep every procedure I write formatted like that. – prmetme Apr 11 '14 at 4:09

Four of your queries (for maprecs, stagerecs, bonusrecs, and tops) are similar to each other. Those queries could be combined into one.

The trick is to use the fact that COUNT(…) only counts the rows for which the expression being counted is non-NULL.

SELECT count(CASE WHEN stage = 0 AND rank = 1 THEN 1 ELSE NULL END CASE) AS maprecs,
count(CASE WHEN stage > 0 AND rank = 1 THEN 1 ELSE NULL END CASE) AS stagerecs,
count(CASE WHEN stage > 0 AND rank = 1 AND cs_times.type = 1 THEN 1 ELSE NULL END CASE) AS bonusrecs,
count(CASE WHEN stage = 0 AND rank BETWEEN 1 AND 10 THEN 1 ELSE NULL END CASE) AS tops
FROM cs_times
INNER JOIN cs_maps ON cs_times.mapid = cs_maps.mapid
WHERE
playerid = userID
AND cs_maps.active = 1
INTO maprecs, stagerecs, bonusrecs, tops;


In rewriting rank <= 10 AND rank > 0 as rank BETWEEN 1 AND 10, I've assumed that there are no values where 0 < rank < 1.

Note that according to the MySQL manual, SELECT … INTO queries should be written such that the INTO clause comes near the end. In several of your queries, you've put the INTO clause after the FROM clause and before the WHERE clause. Even if MySQL understands them, those queries are not standard practice, nor do they read naturally.

This loop is problematic. You should avoid running any query in a loop, both for performance and readability reasons.

WHILE counter < 10 DO
SET ranknum = (
SELECT count(*)
FROM cs_times
INNER JOIN cs_maps ON cs_times.mapid=cs_maps.mapid
WHERE stage = 0
AND rank = counter+1
AND playerid = userID AND active = 1
);

SET p_tops = p_tops + (((10-counter) + (10-counter+2)*30) * ranknum);
SET counter = counter + 1;
END WHILE;


The only purpose of this loop is to compute p_tops. Let's see if we can find a better way.

First, let's change the dummy iteration variable to r, where r = counter + 1, so that it matches the rank. I'll also rename ranknum to row_count to emphasize that it came from count(*).

SET r=1;
WHILE r <= 10 DO
SET row_count = (
SELECT count(*)
FROM cs_times
INNER JOIN cs_maps ON cs_times.mapid=cs_maps.mapid
WHERE stage = 0
AND rank = r
AND playerid = userID AND active = 1
);

SET p_tops = p_tops + (((10 - (r-1)) + (10 - (r-1)+2) * 30) * row_count);
SET r = r + 1;
END WHILE;


Next, let's simplify ((10 - (r-1)) + (10 - (r-1) + 2) * 30). That works out to (401 - 31 * r). Therefore,

SET p_tops = p_tops + (((10 - (r-1)) + (10 - (r-1)+2) * 30) * row_count);


can be written as

SET p_tops = p_tops + (401 - 31 * r) * row_count;


Now we can rewrite the whole loop as

SET p_tops = (
SELECT sum((401 - 31 * rank) * count(*))
FROM cs_times
INNER JOIN cs_maps ON cs_times.mapid=cs_maps.mapid
WHERE
stage = 0
AND playerid = userId AND active = 1
AND rank BETWEEN 1 and 10
);


I have no idea why you would want to calculate such a sum, but there it is. It should be equivalent to the original loop, assuming that rank consists only of integers.