patternsqlMinor
Bad practices in this procedure to calculate player statistics?
Viewed 0 times
thisplayerbadprocedurestatisticspracticescalculate
Problem
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.
``
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 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.ac
``
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 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.ac
Solution
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:
the
some of your
where you have this:
WHERE stage = 0 AND playerid = userIDthe
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)Code Snippets
WHERE stage = 0 AND playerid = userIDContext
StackExchange Code Review Q#46867, answer score: 6
Revisions (0)
No revisions yet.