Now, of course, if I had designed my database without any flaws, fully taking into account all possible directions in which the website could go, anticipating all possible user requests, etc., then we would not have encountered any bugs in the process of applying our code base to this new championship.
Ha. Ha. Ha.
We found many, many bugs, ranging from drawbacks in the design (worst, deepest impact, etc.) to ridiculous examples of hard-coding (the PL/SQL Championship competition_id = 2. So, sure, you could find "2"s in my code. What can I say? At least I admit it. Perhaps this is my own personal form of confessional therapy: dumping all this on you!).
So we did lots of work before the championship was held, and our efforts paid off with an error-free competition (well....not quite. With all players starting at the same time, there was a noticeable delay and some timeouts right at the beginning, but then it all settled down).
But then it was time to report on the results, award prizes, generate certificates, display results on the Quiz Details page....and we found many more bugs!
One of the steps we'd taken previously was to build in much more flexibility about specifying how and when players can see results. So now each competition event (daily quiz, championship, etc.) has a section that looks like (these are the settings for a championship):
But of course I then needed to write the code that would correctly combine these settings with the "state of play" and do the right thing.
It was hard for me to sort out all the logic, but I pushed my way through it, did some testing, seemed OK. And then I (deep shame) copy/pasted it for another function that had slightly different settings. I said to myself: "You have got to refactor this." to which I replied, "Yeah, right, will do. Someday."
You will find the code below. Feel free to read it, of course, but here's my overall take on it: besides the obvious awfulness of the copied code, those complex Boolean expressions are really, really difficult to understand and maintain. In trying to fix a problem right after the playoff, I introduced two more by getting mixed up on AND vs OR and where the parentheses should go.
Oh and then there are the comments:
"/* 2.5 I do not see what this will do. */"
and"
/* 2.5 I do not see what value this adds.*/".
Real confidence builders! So like I say, feel free to check out this code, but what I mostly want to do is show you the refactored version and do a little reality check with anyone who wants to take the time: Is it easier to read? Do you think it was worth doing? Do you see a better way to do this?
And yes, sure, I should provide more of an explanation to what is going on here, but:
a. I don't have the time, and
b. I "pride" myself on writing self-documenting code. In other words, I am too lazy to write comments.
Thanks! Steven Feuerstein
Original UGLY Code
FUNCTION results_available_for (comp_event_id_in IN INTEGER,
user_id_in IN INTEGER)
RETURN BOOLEAN
IS
c_user_id CONSTANT INTEGER
:= NVL (user_id_in, qdb_user_mgr.guest_user_id) ;
c_can_play CONSTANT BOOLEAN
:= can_play (comp_event_id_in, user_id_in) ;
l_comp_event qdb_comp_events%ROWTYPE;
l_competition qdb_competitions%ROWTYPE;
l_return BOOLEAN := FALSE;
BEGIN
/* Might be null from reviewer */
IF comp_event_id_in IS NOT NULL
THEN
l_comp_event := qdb_comp_event_mgr.one_comp_event (comp_event_id_in);
l_competition :=
qdb_comp_event_mgr.competition_for_comp_event (comp_event_id_in);
IF qdb_utilities.trace_is_on
THEN
qdb_utilities.trace_activity (
'results_available_for sra-pas-sa',
l_comp_event.pl_see_results_after
|| '-'
|| l_competition.players_accept_scores
|| '-'
|| l_comp_event.scores_accepted);
END IF;
/* Precedence to access: ANSWERED CLOSED RANKED */
l_return :=
CASE /* Can always see results of "Solve Problem" quiz */
WHEN qdb_content.free_form_question (
qdb_quiz_mgr.single_question_id_for_compev (
comp_event_id_in))
THEN
TRUE /* If answered, then can view if results are available after
answered or scores have been accepted. */
WHEN ex_u_qdb_compev_answers (comp_event_id_in, c_user_id)
THEN
( (l_comp_event.pl_see_results_after =
qdb_competition_mgr.c_resavail_answered)
OR ( l_comp_event.pl_see_results_after =
qdb_competition_mgr.c_resavail_closed
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed)
OR ( l_comp_event.pl_see_results_after =
qdb_competition_mgr.c_resavail_ranked
AND ( l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_ranked
OR ( l_comp_event.ignore_results =
qdb_config.c_yes
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed))))
or ( l_comp_event.pl_see_results_after =
qdb_competition_mgr.c_quiz_accepted
AND ( l_competition.players_accept_quizzes =
qdb_config.c_no
OR ( l_competition.players_accept_quizzes =
qdb_config.c_yes
AND l_comp_event.quizzes_accepted =
qdb_config.c_yes)))
/* Not answered but could play...can only see if closed/ranked*/
WHEN c_can_play
THEN
l_comp_event.comp_event_status IN (qdb_competition_mgr.c_compev_ranked,
qdb_competition_mgr.c_compev_closed)
/* Not a player - what can everyone see? */
WHEN comp_event_closed_for_user (comp_event_id_in, c_user_id)
THEN
( l_comp_event.ev_see_results_after =
qdb_competition_mgr.c_resavail_closed
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed)
OR ( l_comp_event.ev_see_results_after =
qdb_competition_mgr.c_resavail_ranked
AND ( l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_ranked
OR ( l_comp_event.ignore_results =
qdb_config.c_yes
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed)))
/* 2.5 I do not see what value this adds. */
/* Can you see it when it's ranked */
WHEN l_comp_event.pl_see_results_after IN (qdb_competition_mgr.c_resavail_closed,
qdb_competition_mgr.c_resavail_answered,
qdb_competition_mgr.c_resavail_ranked)
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_ranked
THEN
TRUE
ELSE
FALSE
END;
END IF;
RETURN l_return;
END results_available_for;
FUNCTION results_available_for_sql (comp_event_id_in IN INTEGER,
user_id_in IN INTEGER)
RETURN PLS_INTEGER
IS
BEGIN
RETURN CASE
WHEN results_available_for (comp_event_id_in, user_id_in)
THEN
1
ELSE
0
END;
END;
FUNCTION can_see_quiz (comp_event_id_in IN INTEGER, user_id_in IN INTEGER)
RETURN BOOLEAN
IS
c_user_id CONSTANT INTEGER
:= NVL (user_id_in, qdb_user_mgr.guest_user_id) ;
c_can_play CONSTANT BOOLEAN
:= can_play (comp_event_id_in, user_id_in) ;
l_comp_event qdb_comp_events%ROWTYPE;
l_competition qdb_competitions%ROWTYPE;
l_return BOOLEAN := FALSE;
BEGIN
/* Might be null from reviewer */
IF comp_event_id_in IS NOT NULL
THEN
l_comp_event := qdb_comp_event_mgr.one_comp_event (comp_event_id_in);
l_competition :=
qdb_comp_event_mgr.competition_for_comp_event (comp_event_id_in);
IF qdb_utilities.trace_is_on
THEN
qdb_utilities.trace_activity (
'can_see_quiz sqa-pas-sa',
l_competition.pl_see_quiz_after
|| '-'
|| l_competition.players_accept_quizzes
|| '-'
|| l_comp_event.quizzes_accepted);
END IF;
/* if played, quizzes are accepted and can see after quiz taken. */
/* Precedence to access: ANSWERED CLOSED RANKED */
l_return :=
CASE
WHEN qdb_content.free_form_question (
qdb_quiz_mgr.single_question_id_for_compev (
comp_event_id_in))
THEN
/* Can always see results of "Solve Problem" quiz */
TRUE
WHEN ex_u_qdb_compev_answers (comp_event_id_in, c_user_id)
THEN
/* If answered, then can view if results are available after
answered or quizzes have been accepted. */
( (l_comp_event.pl_see_quiz_after =
qdb_competition_mgr.c_resavail_answered)
OR ( l_comp_event.pl_see_quiz_after =
qdb_competition_mgr.c_resavail_closed
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed)
OR ( l_comp_event.ev_see_results_after =
qdb_competition_mgr.c_resavail_ranked
AND ( l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_ranked
OR ( l_comp_event.ignore_results =
qdb_config.c_yes
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed))))
WHEN c_can_play
THEN
/* Not answered but could play...can only see if closed/ranked*/
l_comp_event.comp_event_status IN (qdb_competition_mgr.c_compev_ranked,
qdb_competition_mgr.c_compev_closed)
WHEN comp_event_closed_for_user (comp_event_id_in, c_user_id)
THEN
/* Not a player - what can everyone see? */
( l_comp_event.ev_see_quiz_after =
qdb_competition_mgr.c_resavail_closed
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed)
OR ( l_comp_event.ev_see_results_after =
qdb_competition_mgr.c_resavail_ranked
AND ( l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_ranked
OR ( l_comp_event.ignore_results =
qdb_config.c_yes
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_closed)))
WHEN l_comp_event.pl_see_quiz_after IN (qdb_competition_mgr.c_resavail_closed,
qdb_competition_mgr.c_resavail_answered,
qdb_competition_mgr.c_resavail_ranked)
AND l_comp_event.comp_event_status =
qdb_competition_mgr.c_compev_ranked
THEN
/* 2.5 I do not see what this will do. */
/* Can you see it when it's ranked */
TRUE
ELSE
FALSE
END;
END IF;
RETURN l_return;
END;
The Refactored Code
FUNCTION can_show_information (
info_type_in IN VARCHAR2,
comp_event_id_in IN qdb_comp_events.comp_event_id%TYPE,
user_id_in IN INTEGER)
RETURN BOOLEAN
IS
c_user_id CONSTANT INTEGER
:= NVL (user_id_in, qdb_user_mgr.guest_user_id) ;
c_could_have_played CONSTANT BOOLEAN
:= can_play (comp_event_id_in, c_user_id) ;
c_quiz_answered CONSTANT BOOLEAN
:= ex_u_qdb_compev_answers (comp_event_id_in, c_user_id) ;
c_quizzes_accepted BOOLEAN;
c_results_accepted BOOLEAN;
c_closed_or_ranked BOOLEAN;
c_ranked_or_voided BOOLEAN;
c_is_solve_problem BOOLEAN;
c_closed_for_player BOOLEAN;
CURSOR required_info_cur
IS
SELECT c.players_accept_quizzes,
c.players_accept_scores,
ce.comp_event_status,
ce.ignore_results,
ce.quizzes_accepted,
ce.scores_accepted,
ce.pl_see_quiz_after,
ce.pl_see_answer_after,
ce.pl_see_results_after,
ce.ev_see_quiz_after,
ce.ev_see_answer_after,
ce.ev_see_results_after,
ce.tm_member_results_preview,
ce.author_can_play,
ce.hide_quizzes,
ce.is_private,
ct.text comp_type,
ct.comp_type_id
FROM qdb_competitions c, qdb_comp_events ce, qdb_comp_types ct
WHERE c.competition_id = ce.competition_id
AND c.comp_type_id = ct.comp_type_id
AND ce.comp_event_id = comp_event_id_in;
l_required_info required_info_cur%ROWTYPE;
l_return BOOLEAN;
PROCEDURE get_required_info
IS
BEGIN
OPEN required_info_cur;
FETCH required_info_cur INTO l_required_info;
CLOSE required_info_cur;
c_quizzes_accepted :=
l_required_info.players_accept_quizzes = qdb_config.c_no
OR ( l_required_info.players_accept_quizzes = qdb_config.c_yes
AND l_required_info.quizzes_accepted = qdb_config.c_yes);
c_results_accepted :=
l_required_info.players_accept_scores = qdb_config.c_no
OR ( l_required_info.players_accept_scores = qdb_config.c_yes
AND l_required_info.scores_accepted = qdb_config.c_yes);
c_ranked_or_voided :=
l_required_info.comp_event_status =
qdb_competition_mgr.c_compev_ranked
OR ( l_required_info.ignore_results = qdb_config.c_yes
AND l_required_info.comp_event_status =
qdb_competition_mgr.c_compev_closed);
c_closed_or_ranked :=
/* Also returns TRUE if I took it. So just go with status.
comp_event_closed_for_user (comp_event_id_in, c_user_id)*/
l_required_info.comp_event_status IN (qdb_competition_mgr.c_compev_closed,
qdb_competition_mgr.c_compev_ranked);
c_closed_for_player :=
comp_event_closed_for_user (comp_event_id_in, c_user_id);
c_is_solve_problem :=
l_required_info.comp_type_id =
qdb_competition_mgr.c_solve_problem_ct_id;
END;
FUNCTION everyone_can (moment_in IN VARCHAR2)
RETURN BOOLEAN
IS
BEGIN
RETURN CASE moment_in
WHEN qdb_competition_mgr.c_resavail_never
THEN
FALSE
WHEN qdb_competition_mgr.c_quiz_accepted
THEN
c_quizzes_accepted
WHEN qdb_competition_mgr.c_result_accepted
THEN
c_results_accepted
WHEN qdb_competition_mgr.c_resavail_closed
THEN
c_closed_or_ranked
WHEN qdb_competition_mgr.c_resavail_ranked
THEN
c_ranked_or_voided
ELSE
FALSE
END;
IF qdb_utilities.trace_is_on
THEN
qdb_utilities.trace_activity (
'can_show_information for everyone moment ' || moment_in,
l_return);
END IF;
END;
FUNCTION player_can (moment_in IN VARCHAR2)
RETURN BOOLEAN
IS
l_return BOOLEAN;
BEGIN
l_return :=
CASE moment_in
WHEN qdb_competition_mgr.c_resavail_never
THEN
FALSE
WHEN qdb_competition_mgr.c_quiz_accepted
THEN
c_quizzes_accepted
WHEN qdb_competition_mgr.c_result_accepted
THEN
c_results_accepted
WHEN qdb_competition_mgr.c_resavail_answered
THEN
c_quiz_answered
WHEN qdb_competition_mgr.c_resavail_closed
THEN
c_closed_or_ranked
OR ( info_type_in = c_see_correctness
AND l_required_info.players_accept_quizzes =
qdb_config.c_yes)
WHEN qdb_competition_mgr.c_resavail_ranked
THEN
c_ranked_or_voided
ELSE
FALSE
END;
RETURN l_return;
END;
BEGIN
IF comp_event_id_in IS NULL
THEN
/* A reviewer may be reviewing an unscheduled quiz... */
l_return := info_type_in = c_see_answers;
ELSE
get_required_info;
/* Can always see stuff for a "solve problem" quiz. */
l_return := c_is_solve_problem;
IF NOT l_return
THEN
CASE info_type_in
WHEN c_see_my_choices
THEN
/* For this choice ONLY, the acceptance status of quizzes does not apply */
l_return :=
CASE
WHEN c_quiz_answered OR c_could_have_played
THEN
player_can (l_required_info.pl_see_quiz_after)
ELSE
everyone_can (l_required_info.ev_see_quiz_after)
END;
WHEN c_see_correctness
THEN
l_return :=
CASE
WHEN c_quiz_answered OR c_could_have_played
THEN
player_can (l_required_info.pl_see_answer_after)
ELSE
everyone_can (l_required_info.ev_see_answer_after)
END;
WHEN c_see_answers
THEN
/* Control display of overall answer, choice explanations, etc. */
l_return :=
CASE
WHEN NOT c_quizzes_accepted
THEN
FALSE
WHEN c_quiz_answered OR c_could_have_played
THEN
player_can (l_required_info.pl_see_answer_after)
ELSE
everyone_can (l_required_info.ev_see_answer_after)
END;
WHEN c_see_results
THEN
/* Display stats, rankings, % correct, etc. */
l_return :=
CASE
WHEN NOT c_results_accepted
THEN
FALSE
WHEN c_quiz_answered OR c_could_have_played
THEN
player_can (l_required_info.pl_see_results_after)
ELSE
everyone_can (
l_required_info.ev_see_results_after)
END;
END CASE;
END IF;
END IF;
RETURN l_return;
END;
/* And now specialized programs for different scenarios. */
FUNCTION can_see_my_choices (comp_event_id_in IN INTEGER,
user_id_in IN INTEGER)
RETURN BOOLEAN
IS
BEGIN
RETURN can_show_information (c_see_my_choices,
comp_event_id_in,
user_id_in);
END;
FUNCTION can_see_correctness (comp_event_id_in IN INTEGER,
user_id_in IN INTEGER)
RETURN BOOLEAN
IS
BEGIN
RETURN can_show_information (c_see_correctness,
comp_event_id_in,
user_id_in);
END;
FUNCTION can_see_answers (comp_event_id_in IN INTEGER,
user_id_in IN INTEGER)
RETURN BOOLEAN
IS
BEGIN
RETURN can_show_information (c_see_answers,
comp_event_id_in,
user_id_in);
END;
FUNCTION can_see_results (comp_event_id_in IN INTEGER,
user_id_in IN INTEGER)
RETURN BOOLEAN
IS
BEGIN
RETURN can_show_information (c_see_results,
comp_event_id_in,
user_id_in);
END;
FUNCTION results_available_for (comp_event_id_in IN INTEGER,
user_id_in IN INTEGER)
RETURN BOOLEAN
IS
BEGIN
RETURN can_see_results (comp_event_id_in, user_id_in);
END results_available_for;
Enjoyed reading code. But new refactoring code has hard coded SQL....
ReplyDeleteJames Su asked me to post this response. Thanks, James!
ReplyDeleteIn PROCEDURE get_required_info, you use an explict cursor instead of select into. This allows you to declare a %ROWTYPE based on the cursor without declaring a record type or a bunch of variables. But you also miss the NO_DATA_FOUND and TOO_MANY_ROWS exception. Maybe these two will never happen in your case.
Since CURSOR required_info_cur and l_required_info are only used in get_required_info, you may want to move them into get_required_info from can_show_information.
The trace_activity is never reached in everyone_can. The l_return in everyone_can is actually referencing the one in its caller can_show_information. You need to declare a local l_return and assign to it, like player_can.
There are similar names at local level and package level, for example:
c_see_my_choices is VARCHAR2 at package level;
c_quiz_answered is BOOLEAN at local level.
When they are mixed in simple/searched case expression, it's not easy to read (at least at the first sight).
I would prefer to restructure the main function bode in this way:
l_return :=
CASE (info_type_in=c_see_answers AND NOT c_quizzes_accepted)
OR (info_type_in=c_see_results AND NOT c_results_accepted)
THEN FALSE
WHEN c_quiz_answered OR c_could_have_played THEN
player_can (CASE info_type_in
WHEN c_see_my_choices THEN l_required_info.pl_see_quiz_after
WHEN c_see_correctness THEN l_required_info.pl_see_answer_after
WHEN c_see_answers THEN l_required_info.pl_see_answer_after
WHEN c_see_results THEN l_required_info.pl_see_results_after
END)
ELSE
everyone_can (CASE info_type_in
WHEN c_see_my_choices THEN l_required_info.ev_see_quiz_after
WHEN c_see_correctness THEN l_required_info.ev_see_answer_after
WHEN c_see_answers THEN l_required_info.ev_see_answer_after
WHEN c_see_results THEN l_required_info.ev_see_results_after
END)
END;
I can see player_can and everyone_can sharing some common logic, there's a potential to merge these two functions into one.
Regards, James Su
James, thanks SO much for pointing out the flaws regarding l_return; there must be a good quiz somewhere in THAT mistake. And also narrowing the scope of the cursor-related declarations. I am going to apply those and then take a closer look at your deeper restructuring. Thanks again! Sf
ReplyDeleteThanks Steven, I didn't post the code myself because this blog always does a "LTRIM" on each line of the code, and I thought you were able to format it.
ReplyDeleteI missed the first "WHEN" keyword after "CASE", but I believe you got the idea. In the original version you called player_can/everyone_can many times with different parameters, in my version these two actions appear only once and you can clearly see what kind of parameter is used in each case.
BTW, c_quizzes_accepted and the other booleans are not constant variable, why do you name them like that?