1: procedure post_auth_7048783549465082709 is 2: begin 3: APEX_UTIL.RESET_AUTHORIZATIONS; 4: 5: for c1 in ( 6: select count(*) cnt 7: from app_users 8: where username = :APP_USER 9: and role_name = 'ADMINISTRATOR' ) 10: loop 11: if c1.cnt > 0 then 12: :IS_ADMIN := 'TRUE'; 13: else 14: :IS_ADMIN := 'FALSE'; 15: end if; 16: end loop; 17: 18: for c1 in ( 19: select count(*) cnt 20: from app_users 21: where username = :APP_USER 22: and role_name = 'CONTRIBUTOR' ) 23: loop 24: if c1.cnt > 0 then 25: :IS_CONTRIBUTOR := 'TRUE'; 26: else 27: :IS_CONTRIBUTOR := 'FALSE'; 28: end if; 29: end loop; 30: 31: for c1 in ( 32: select count(*) cnt 33: from app_users 34: where username = :APP_USER 35: and role_name = 'TESTER' ) 36: loop 37: if c1.cnt > 0 then 38: :IS_TESTER := 'TRUE'; 39: else 40: :IS_TESTER := 'FALSE'; 41: end if; 42: end loop; 43: 44: for c1 in ( 45: select count(*) cnt 46: from app_users 47: where username = :APP_USER 48: and role_name = 'REPORTING' ) 49: loop 50: if c1.cnt > 0 then 51: :IS_REPORTING := 'TRUE'; 52: else 53: :IS_REPORTING := 'FALSE'; 54: end if; 55: end loop; 56: 57: for c1 in ( 58: select substr(lower(:APP_USER), instr(:APP_USER, '@')+1) email 59: from dual 60: ) 61: loop 62: if c1.email = 'our_domain.com' then 63: :is_employee := 'TRUE'; 64: else 65: :is_employee := 'FALSE'; 66: end if; 67: end loop; 68: 69: for c1 in ( 70: select count(*) cnt 71: from app_users 72: where username = :APP_USER 73: and role_name = 'SUPER ADMINISTRATOR' ) 74: loop 75: if c1.cnt > 0 then 76: :IS_SUPERADMIN := 'TRUE'; 77: else 78: :IS_SUPERADMIN := 'FALSE'; 79: end if; 80: end loop; 81: 82: end;
Don't look down below yet.
What do you think is wrong?
The more capable you are at analyzing this (and your own) code all by your lonesome, the more valuable you will be to your dev teammates and employer.
Right then. Let's take a look at how we might clean this up. Here are my concerns:
- Poor formatting: Even if you don't like my style of upper-casing all keywords, the lack of indentation is a significant impediment to reading code.
- Use of cursor FOR loop with a single row query (SELECT COUNT(*)): Cursor FOR loops are for fetching multiple rows of data. If you know for sure that there is just one row, using a cursor FOR loop just means you are being lazy, and not in a good way (you don't have to declare a record or variable to accept the fetched value).
- Use of COUNT(*) to determine if there is at least one row: COUNT(*) answers the question "How many have I got?" when the question being asked here is "Is there at least one row?". What if there are 10,000,000 rows? The logic works, but you could spend a few extra CPU cycles getting to the answer. And, as with the use of the cursor FOR loop, it's misleading and confusing.
- Repetitive code: If you need the same functionality again and again, don't copy and paste. Ugh. Instead, apply your relational database normalization skills to your code: create a nested subprogram and call that multiple times.
- SELECT FROM dual: No, please, no! If you are running 11.1 or higher, there is almost no reason left to use a SELECT FROM dual instead of a native PL/SQL assignment. Certainly this is not necessary for an expression involving SUBSTR.
- Too much code inside Application Express: whenever possible move your PL/SQL code to packages compiled into the database. It is easier to manage from there.
Did I miss anything? Let me know!
Here's my first pass on a cleanup of this procedure:
PROCEDURE post_auth_7048783549465082709 IS PROCEDURE set_value (role_in IN VARCHAR2, item_out OUT VARCHAR2) IS FUNCTION role_exists (role_in IN VARCHAR2) RETURN BOOLEAN IS BEGIN SELECT 'x' INTO l_dummy FROM app_users WHERE username = :app_user AND role_name = role_in; RETURN TRUE; EXCEPTION WHEN NO_DATA_FOUND THEN RETURN FALSE; END; BEGIN item_out := CASE WHEN role_exists (role_in) THEN 'TRUE' ELSE 'FALSE' END; END; BEGIN APEX_UTIL.RESET_AUTHORIZATIONS; set_value ('ADMINISTRATOR', :is_admin); set_value ('CONTRIBUTOR', :is_contributor); set_value ('TESTER', :is_tester); set_value ('REPORTING', :is_reporting); set_value ('SUPER ADMINISTRATOR', :is_superadmin); :is_employee := CASE WHEN SUBSTR (LOWER ( :app_user), INSTR ( :app_user, '@') + 1) = 'our_domain.com' THEN 'TRUE' ELSE 'FALSE' END; END;
This rewrite covers the first 5 concerns I listed. Gone are the cursor FOR loops, the COUNT(*), the repetitive code, the SELECT FROM dual. I shift from IF statements to CASE expressions, allowing for more concise and cleaner code.
Now let's take a look at how we might tackle concern #6 ("Whenever possible move your PL/SQL code to packages compiled into the database."). The app_post_authenticate procedure is a database object. It has pretty much the same code as my first rewrite, but instead of referencing host variables directly inside the procedure, I move them into the parameter list.
CREATE OR REPLACE PROCEDURE app_post_authenticate ( app_user_in IN VARCHAR2, admin_out OUT VARCHAR2, contributor_out OUT VARCHAR2, tester_out OUT VARCHAR2, reporting_out OUT VARCHAR2, super_admin_out OUT VARCHAR2, is_employee_out OUT VARCHAR2) IS PROCEDURE set_value (role_in IN VARCHAR2, item_out OUT VARCHAR2) IS FUNCTION role_exists (role_in IN VARCHAR2) RETURN BOOLEAN IS BEGIN SELECT 'x' INTO l_dummy FROM app_users WHERE username = app_user_in AND role_name = role_in; RETURN TRUE; EXCEPTION WHEN NO_DATA_FOUND THEN RETURN FALSE; END; BEGIN item_out := CASE WHEN role_exists (role_in) THEN 'TRUE' ELSE 'FALSE' END; END; BEGIN APEX_UTIL.RESET_AUTHORIZATIONS; set_value ('ADMINISTRATOR', admin_out); set_value ('CONTRIBUTOR', contributor_out); set_value ('TESTER', tester_out); set_value ('REPORTING', reporting_out); set_value ('SUPER ADMINISTRATOR', superadmin_out); is_employee_out := CASE WHEN SUBSTR (LOWER (app_user_in), INSTR (app_user_in, '@') + 1) = 'our_domain.com' THEN 'TRUE' ELSE 'FALSE' END; END;
And then in Application Express, I have only this left:
PROCEDURE post_auth_7048783549465082709 IS BEGIN app_post_authenticate ( :app_user, :is_admin, :is_contributor, :is_tester, :is_reporting, :is_superadmin, :is_employee); END;
Sure, you could argue that by moving all the code to the database, you no longer can see what is going on in your application.
I would counter with: how often do you need to see this logic? Probably rarely. And most of the logic involved is already in the database: the app_users table, the literal values needed to "match up" with those rows in the table, etc. In addition, besides setting values of application items, there is no UI-specific logic in the procedure. That's another reason to move the code out of the application.