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

Bad practices in this procedure to calculate player statistics?

Submitted by: @import:stackexchange-codereview··
0
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.

``
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:

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)

Code Snippets

WHERE stage = 0 AND playerid = userID

Context

StackExchange Code Review Q#46867, answer score: 6

Revisions (0)

No revisions yet.