tag:blogger.com,1999:blog-8677649049588007585.post8638393605811429822..comments2023-06-18T16:15:22.432+01:00Comments on PL/SQL Challenge: A Glimpse into Feuerstein Refactoring....End Result Better?Steven Feuersteinhttp://www.blogger.com/profile/16619706770920320550noreply@blogger.comBlogger4125tag:blogger.com,1999:blog-8677649049588007585.post-55200365885815112072014-02-23T17:13:51.703+00:002014-02-23T17:13:51.703+00:00Thanks Steven, I didn't post the code myself b...Thanks 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.<br />I 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.<br />BTW, c_quizzes_accepted and the other booleans are not constant variable, why do you name them like that?<br />Unknownhttps://www.blogger.com/profile/10689126574094744906noreply@blogger.comtag:blogger.com,1999:blog-8677649049588007585.post-15743025898042358662014-02-22T23:56:17.719+00:002014-02-22T23:56:17.719+00:00James, thanks SO much for pointing out the flaws r...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! SfSteven Feuersteinhttps://www.blogger.com/profile/16619706770920320550noreply@blogger.comtag:blogger.com,1999:blog-8677649049588007585.post-83293158359422596672014-02-20T23:35:28.062+00:002014-02-20T23:35:28.062+00:00James Su asked me to post this response. Thanks, J...James Su asked me to post this response. Thanks, James!<br /><br />In 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.<br /><br />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.<br /><br />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.<br /><br />There are similar names at local level and package level, for example: <br /><br />c_see_my_choices is VARCHAR2 at package level;<br /><br />c_quiz_answered is BOOLEAN at local level.<br /><br />When they are mixed in simple/searched case expression, it's not easy to read (at least at the first sight).<br /><br />I would prefer to restructure the main function bode in this way:<br /><br />l_return :=<br /><br /> CASE (info_type_in=c_see_answers AND NOT c_quizzes_accepted)<br /><br /> OR (info_type_in=c_see_results AND NOT c_results_accepted)<br /><br /> THEN FALSE<br /><br /> WHEN c_quiz_answered OR c_could_have_played THEN<br /><br /> player_can (CASE info_type_in<br /><br /> WHEN c_see_my_choices THEN l_required_info.pl_see_quiz_after<br /><br /> WHEN c_see_correctness THEN l_required_info.pl_see_answer_after<br /><br /> WHEN c_see_answers THEN l_required_info.pl_see_answer_after<br /><br /> WHEN c_see_results THEN l_required_info.pl_see_results_after<br /><br /> END)<br /><br /> ELSE<br /><br /> everyone_can (CASE info_type_in<br /><br /> WHEN c_see_my_choices THEN l_required_info.ev_see_quiz_after<br /><br /> WHEN c_see_correctness THEN l_required_info.ev_see_answer_after<br /><br /> WHEN c_see_answers THEN l_required_info.ev_see_answer_after<br /><br /> WHEN c_see_results THEN l_required_info.ev_see_results_after<br /><br /> END)<br /><br /> END;<br /><br />I can see player_can and everyone_can sharing some common logic, there's a potential to merge these two functions into one.<br /><br />Regards, James Su<br /><br />StevenFeuersteinhttps://www.blogger.com/profile/13931412532278395973noreply@blogger.comtag:blogger.com,1999:blog-8677649049588007585.post-11346530328952316982014-02-19T18:13:24.603+00:002014-02-19T18:13:24.603+00:00Enjoyed reading code. But new refactoring code has...Enjoyed reading code. But new refactoring code has hard coded SQL....Anonymoushttps://www.blogger.com/profile/13630417118337996577noreply@blogger.com