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.

There have been many FizzBuzz questions lately, and here is one more! Granted, it is pretty straightforward to do FizzBuzz with SQL using calculations.

So instead, I'm going to do a FizzBuzz which will not only be functional, but demonstrate some of the core functions of SQL in general and MySQL PostgreSQL in particular. To make it more fun, we will work with 5 keywords instead of 2: Fizz, Buzz, Woof, Ping, Plop, and they will be user-assigned.

There will be multiple scripts so just bear with me. Please note I'm using Pascal notation throughout.

Create the original schema

DROP SCHEMA IF EXISTS PhrancisFizzBuzz CASCADE;
CREATE SCHEMA PhrancisFizzBuzz;
-- this table will hold the list of numbers to check for FizzBuzz
CREATE TABLE PhrancisFizzBuzz.Number(
    n INT NOT NULL
); 
-- the following tables will hold multiplier values based on table PhrancisFizzBuzz.Number
CREATE TABLE PhrancisFizzBuzz.Fizz(
    Number INT,
    Word VARCHAR(4) NOT NULL DEFAULT 'Fizz'
);
CREATE TABLE PhrancisFizzBuzz.Buzz(
    Number INT,
    Word VARCHAR(4) NOT NULL DEFAULT 'Buzz'
);
CREATE TABLE PhrancisFizzBuzz.Woof(
    Number INT,
    Word VARCHAR(4) NOT NULL DEFAULT 'Woof'
);
CREATE TABLE PhrancisFizzBuzz.Ping(
    Number INT,
    Word VARCHAR(4) NOT NULL DEFAULT 'Ping'
);
CREATE TABLE PhrancisFizzBuzz.Plop(
    Number INT,
    Word VARCHAR(4) NOT NULL DEFAULT 'Plop'
);

Populate FizzBuzz tables

Now I will start creating procedures to do the work.

CREATE OR REPLACE FUNCTION PhrancisFizzBuzz.fncPopulateFizzBuzz(
    prmMaxNumber INT, -- for example 100
    prmFizz INT,
    prmBuzz INT,
    prmWoof INT,
    prmPing INT,
    prmPlop INT
) RETURNS VOID
AS -- begin literal string with escape $$
    $$
BEGIN
SET SEARCH_PATH = PhrancisFizzBuzz;
-- initialize and populate list of initial numbers
TRUNCATE TABLE Number;
INSERT INTO Number (n)
    SELECT * FROM GENERATE_SERIES(1,  prmMaxNumber);
-- populate the FizzBuzz tables with multiples
-- based on user parameter choice
TRUNCATE TABLE Fizz; 
INSERT INTO Fizz
    SELECT n
    FROM Number
    WHERE COALESCE(prmFizz,0) <> 0 AND n % prmFizz = 0;
INSERT INTO Buzz
    SELECT n
    FROM Number
    WHERE COALESCE(prmBuzz,0) <> 0 AND n % prmBuzz = 0;
INSERT INTO Woof
    SELECT n
    FROM Number
    WHERE COALESCE(prmWoof,0) <> 0 AND n % prmWoof = 0;
INSERT INTO Ping
    SELECT n
    FROM Number
    WHERE COALESCE(prmPing,0) <> 0 AND n % prmPing = 0;
INSERT INTO Plop
    SELECT n
    FROM Number
    WHERE COALESCE(prmPlop,0) <> 0 AND n % prmPlop = 0;
END;
$$ -- end literal string
LANGUAGE plpgsql;

For testing:

SET SEARCH_PATH = PhrancisFizzBuzz;
SELECT fncPopulateFizzBuzz(100,3,5,0,0,0);
SELECT * FROM Number
    LEFT JOIN Fizz ON Number.n = Fizz.Number
    LEFT JOIN Buzz ON Number.n = Buzz.Number 
ORDER BY Number.n;

Result sample:

enter image description here

Total query runtime: 17 ms.
100 rows retrieved.

FizzBuzz into a table with JOIN

This displays result set into a table with a row for each word:

CREATE OR REPLACE FUNCTION PhrancisFizzBuzz.fcnTableFizzBuzz(
    prmMaxNumber INT, -- for example 100
    prmFizz INT,
    prmBuzz INT,
    prmWoof INT,
    prmPing INT,
    prmPlop INT
    ) RETURNS VOID
    AS 
$$
    BEGIN
    SET SEARCH_PATH = PhrancisFizzBuzz;
    -- call populate function with input parameters
    PERFORM fncPopulateFizzBuzz(
        prmMaxNumber,
        prmFizz,
        prmBuzz,
        prmWoof,
        prmPing,
        prmPlop
    );
    DROP TABLE IF EXISTS tmpFizzBuzzResults;
    CREATE TEMP TABLE tmpFizzBuzzResults AS
    SELECT    num.n AS Number,
        Fizz.Word AS Fizz,
        Buzz.Word AS Buzz,
        Woof.Word AS Woof,
        Ping.Word AS Ping,
        Plop.Word AS Plop
    FROM Number AS num
        LEFT JOIN Fizz ON Fizz.Number = num.n
        LEFT JOIN Buzz ON Buzz.Number = num.n
        LEFT JOIN Woof ON Woof.Number = num.n
        LEFT JOIN Ping ON Ping.Number = num.n
        LEFT JOIN Plop ON Plop.Number = num.n
    ;
    END;
    $$
LANGUAGE plpgsql;

For testing:

SET SEARCH_PATH = PhrancisFizzBuzz;
SELECT fcnTableFizzBuzz(100,3,5,7,11,13);
SELECT * FROM tmpFizzBuzzResults ORDER BY Number ASC;

Output sample:

enter image description here

Total query runtime: 78 ms.
100 rows retrieved.

I did try to work in RETURN TABLE with the function but could not get it to work. Any advice on that is welcome.


Last but not least, this generates the classic FizzBuzz:

CREATE OR REPLACE FUNCTION PhrancisFizzBuzz.fcnClassicFizzBuzz(
      prmMaxNumber INT,
      prmFizz INT,
      prmBuzz INT,
      prmWoof INT,
      prmPing INT,
      prmPlop INT
)
RETURNS VOID
AS
$BODY$
BEGIN

-- populate tables based on user input
-- using previously created function
SET SEARCH_PATH = PhrancisFizzBuzz;
    PERFORM fncPopulateFizzBuzz(
        prmMaxNumber,
        prmFizz,
        prmBuzz,
        prmWoof,
        prmPing,
        prmPlop
    );
DROP TABLE IF EXISTS tmpFizzBuzzResults;
CREATE TEMP TABLE tmpFizzBuzzResults AS
    WITH cteFizzBuzzConcat AS(
          SELECT    Number.n AS Number,
            CONCAT(Fizz.Word, Buzz.Word, Woof.Word, Ping.Word, Plop.Word) AS Word
          FROM Number
        LEFT JOIN Fizz ON Number.n = Fizz.Number
        LEFT JOIN Buzz ON Number.n = Buzz.Number
        LEFT JOIN Woof ON Number.n = Woof.Number
        LEFT JOIN Ping ON Number.n = Ping.Number
        LEFT JOIN Plop ON Number.n = Plop.Number
    )
    SELECT    Number
    ,    (CASE 
        WHEN Word <> '' THEN Word
        -- WHEN Word IS NOT NULL THEN Word
        ELSE CAST(Number AS VARCHAR)
        END) AS Value
    FROM cteFizzBuzzConcat;
END;
$BODY$ LANGUAGE plpgsql;

-- For testing:

    SET SEARCH_PATH = PhrancisFizzBuzz;
    SELECT fcnClassicFizzBuzz(100,3,5,7,11,13);
    SELECT Value FROM tmpFizzBuzzResults ORDER BY Number ASC;

Execution:

Total query runtime: 53 ms.
100 rows retrieved.

Output sample:

enter image description here

Update

I ran the last function over 100,000 numbers and it executed in 3.5 seconds.

share|improve this question
8  
About time... +1 for the pure effort –  Alex L Jul 23 at 1:45
4  
Finally someone steps up to do more than two words. Next we need to have a rule for primes, rules that apply when N appears as a factor OR a digit, repetition of words when a rule applies more than once to a number, etc. At least you have a design that might be extensible enough! –  David K Jul 23 at 6:37
    
Note: concat() was introduced in PostgreSQL 9.1. –  200_success Jul 23 at 10:18
5  
@DavidK Pretty soon we'll have created base-onomatopoeia (as opposed to that god awful base-10 used by mainstream mathematicians). –  Pharap Jul 23 at 11:23

2 Answers 2

up vote 22 down vote accepted

Proliferation of tables

Database schemas should not be designed such that you need to create more tables to accommodate more data. The same principle applies here. Instead of separate tables Fizz, Buzz, Woof, Ping, and Plop, you should have a single NoiseDefs table. A classic unparameterized FizzBuzz should be much simpler…

WITH NoiseDefs(multiple, noise) AS (
    VALUES (3, 'Fizz')
         , (5, 'Buzz')
), CombinedNoises AS (
    SELECT n
         , string_agg(noise, '' ORDER BY multiple) AS noise
        FROM generate_series(1, 100) AS n
            LEFT OUTER JOIN NoiseDefs
                ON n % multiple = 0
        GROUP BY n
)
SELECT n
     , coalesce(noise, CAST(n AS TEXT)) AS noiseOrNum
    FROM CombinedNoises
    ORDER BY n;

Note the use of VALUES and string_agg().

Prefer TEXT over VARCHAR(n) in PostgreSQL

In PostgreSQL, there is not much point to using VARCHAR(n) rather than TEXT — all it does is cause problems if the string length exceeds the limit. You might as well allow noises with more than four characters.

Misuse of temporary tables in functions

The way your functions interact with temporary tables is non-obvious. Each one of them manipulates temporary tables behind the scenes, and the caller needs to have knowledge of that behaviour in order to use the functions. In other words, it's nuts that SELECT fcnClassicFizzBuzz(100, 3, 5, 7, 11, 13); returns nothing, and you have to retrieve the results separately using SELECT Value FROM tmpFizzBuzzResults ORDER BY Number ASC;. (I think you may have been aware of this problem when you said I did try to work in RETURN TABLE with the function but could not get it to work.)

The fcnClassicFizzBuzz() function populates the Fizz, Buzz, Woof, Ping, and Plop tables, but does not clean up after itself. Therefore, it's leaving junk in those tables as a side-effect. Repeated calls to fcnClassicFizzBuzz() will result in increasingly incorrect output. The only recourse for the user is to issue TRUNCATE Fizz; TRUNCATE Buzz; TRUNCATE Woof; TRUNCATE Ping; TRUNCATE Plop; — you didn't even provide a convenience function to undo fncPopulateFizzBuzz().

If you want to let the multiples be parameterized, it seems fair to also let their associated noises be user-specified as well.

CREATE OR REPLACE FUNCTION FlexibleFizzBuzz(
    max INTEGER,
    n1 INTEGER, noise1 TEXT,
    n2 INTEGER, noise2 TEXT,
    n3 INTEGER, noise3 TEXT,
    n4 INTEGER, noise4 TEXT,
    n5 INTEGER, noise5 TEXT
) RETURNS TABLE(n INTEGER, noiseOrNum TEXT) AS $BODY$
    BEGIN
        RETURN QUERY
        WITH NoiseDefs(multiple, noise) AS (
            VALUES (n1, noise1)
                 , (n2, noise2)
                 , (n3, noise3)
                 , (n4, noise4)
                 , (n5, noise5)
        ), CombinedNoises AS (
            SELECT num
                 , string_agg(noise, '' ORDER BY multiple) AS noise
                FROM generate_series(1, max) AS num
                    LEFT OUTER JOIN NoiseDefs
                        ON num % multiple = 0
                GROUP BY num
        )
        SELECT num
             , coalesce(noise, CAST(num AS TEXT))
            FROM CombinedNoises
            ORDER BY num;
    END;
$BODY$ LANGUAGE plpgsql;

Avoid the use of temporary tables altogether if possible. There's less cleanup to worry about, and less wasted I/O. To cache the results, let the caller do it:

CREATE TEMPORARY TABLE FizzBuzzResults10000 AS
    SELECT *
        FROM FlexibleFizzBuzz(10000, 3, 'Fizz', 5, 'Buzz', 7, 'Woof', 11, 'Ping', 13, 'Plop');

This implementation runs in about half the time as your original code.

Function overloading

It would be nice to provide convenience functions, so that some of the parameters can be omitted. For example:

CREATE OR REPLACE FUNCTION FlexibleFizzBuzz(
    max INTEGER,
    n1 INTEGER, noise1 TEXT,
    n2 INTEGER, noise2 TEXT
) RETURNS TABLE(n INTEGER, noiseOrNum TEXT) AS $BODY$
    BEGIN
    RETURN QUERY SELECT * FROM FlexibleFizzBuzz(
        max,
        n1, noise1,
        n2, noise2,
        NULL, NULL,
        NULL, NULL,
        NULL, NULL);
    END;
$BODY$ LANGUAGE plpgsql;

I don't believe that PL/pgSQL functions can take a variable number of arguments, though.

Hungarian notation

As you can infer from my implementation above, I'm not a fan of the prefixes that you used, such as fnc…, prm…, tmp…, and cte…. Rather, I would use CapitalizedNames for table-like objects, such as tables, views, and functions that return rows. For scalars, use lowercaseNames. I see it as an advantage that you can replace a temporary table with a view, for example. (PostgreSQL identifiers are actually case-insensitive unless you "quote" them, but having that convention in the source code should be sufficient.)

share|improve this answer
    
Getting a few function errors but should be able to fix them easily. Thanks again –  Phrancis Jul 25 at 6:36
3  
This solution is Works on My Machine-certified™ (PostgreSQL 9.0). Feel free to correct any problems you find, though. –  200_success Jul 25 at 6:48

I think your scripts could use a bit more vertical whitespace; maybe it's the uppercasing, but I find this:

BEGIN
SET SEARCH_PATH = PhrancisFizzBuzz;
-- call populate function with input parameters
PERFORM fncPopulateFizzBuzz(
    prmMaxNumber,
    prmFizz,
    prmBuzz,
    prmWoof,
    prmPing,
    prmPlop
);
DROP TABLE IF EXISTS tmpFizzBuzzResults;
CREATE TEMP TABLE tmpFizzBuzzResults AS
...

Would be a bit easier like this:

BEGIN

    SET SEARCH_PATH = PhrancisFizzBuzz;
    -- call populate function with input parameters
    PERFORM fncPopulateFizzBuzz(
        prmMaxNumber,
        prmFizz,
        prmBuzz,
        prmWoof,
        prmPing,
        prmPlop
    );

    DROP TABLE IF EXISTS tmpFizzBuzzResults;

    CREATE TEMP TABLE tmpFizzBuzzResults AS
    ...

I like to indent BEGIN...END blocks, too.


I don't get this, is that a Tab?

SELECT    Number.n AS Number,

You'll want a space there instead:

SELECT Number.n AS Number,

That's in the cteFizzBuzzConcat; you have a similar query in tmpFizzBuzzResults, but with a different alias:

SELECT    num.n AS Number,

Which is inconsistent because it's the same field, taken from the same source.


You're storing a lot of VARCHAR(4) non-values that I would like to see as NULL values. For 100000 numbers you're storing 400000 x4 useless, empty characters; you should null-coalesce them later, when you're concatenating them.


One more thing - I'll quote a notorious Monkey here:

The only sensible review for it is: don't use SQL to do FizzBuzz

;)

share|improve this answer
4  
Oh come on, 3.5 secs to run 100K records has got to be an argument in favor of SQL doing FizzBuzz ;) Thanks for the review @Mat'sMug –  Phrancis Jul 23 at 4:28
    
I agree, but personally I don't like a new line after a BEGIN it reminds me of people adding a new line right after the Curly's in an if statement. –  Malachi Jul 23 at 13:20

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.