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.

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$$
share|improve this question
    
are playerid and userID in the same table? are you looking to see if they are in the same row? –  Malachi Apr 11 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 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 at 2:31
add comment

4 Answers 4

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)

share|improve this answer
2  
    
sounds like TEMP Tables can't be used more than once in a query either –  Malachi Apr 11 at 2:58
1  
Dang that Bug report is 8 years old and they still haven't fixed it. –  Malachi Apr 11 at 2:59
2  
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 at 5:21
add comment

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$$
share|improve this answer
    
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 at 4:09
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.