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;
