Recently I refactored some PL/SQL for sending emails – code that I wrote way back in 2004. The number of “WTF“‘s per minute has not been too high; however, I’ve cringed more times than I’d like…
1. Overly-generic parameter types
When you send an email, it will have at least one recipient, and it may have many recipients. However, no email will have more than one sender. Yet, I wrote the package procedure like this:
TYPE address_type IS RECORD
(name VARCHAR2(100)
,email_address VARCHAR2(200)
);
TYPE address_list_type IS TABLE OF address_type
INDEX BY BINARY_INTEGER;
PROCEDURE send
(i_sender IN address_list_type
,i_recipients IN address_list_type
,i_subject IN VARCHAR2
,i_message IN VARCHAR2
);
Why I didn’t have i_sender be a simple address_type, I can’t remember. Internally, the procedure only looks at i_sender(1) – if a caller were to pass in a table of more than one sender, it raises an exception.
2. Functional programming to avoid local variables
Simple is best, and there’s nothing wrong with using local variables. I wish I’d realised these facts when I wrote functions like this:
FUNCTION address
(i_name IN VARCHAR2
,i_email_address IN VARCHAR2
) RETURN address_list_type;
FUNCTION address
(i_address IN address_list_type
,i_name IN VARCHAR2
,i_email_address IN VARCHAR2
) RETURN address_list_type;
All that so that callers can avoid *one local variable*:
EMAIL_PKG.send
(i_sender => EMAIL_PKG.address('joe','joe@company.com')
,i_recipients => EMAIL_PKG.address(
EMAIL_PKG.address(
'jill', 'jill@company.com')
,'bob', 'bob@company.com')
,i_subject => 'hello'
,i_message => 'world'
);
See what I did there with the recipients? Populating an array on the fly with just function calls. Smart eh? But rather useless, as it turns out; when we need to send multiple recipients, it’s usually populated within a loop of unknown sized, so this method doesn’t work anyway.
Go ahead – face your past and dig up some code you wrote 5 years ago or more. I think, if you don’t go “WTF!” every now and then, you probably haven’t learned anything or improved yourself in the intervening years. Just saying 🙂
A somewhat mistaken attempt to format a number left-padded with up to six zeroes, made me cringe a little:
SELECT LPAD(number_column, 6, 000000) FROM ...
Yes, it does work (the domain of number_column is limited to positive integers), but not for the reason the writer thought, I think.
(Hint: what is the data type of the third parameter to LPAD?)
Seen in the wild:
... WHERE substr(amount,0, 1) != '-'
If you wanted to query a table of monetary transactions for any refunds (i.e. where the transaction amount is negative), how would you do it? Perhaps you’d think about avoiding problems that might occur if the default number format were to change, hm?
(before you say it: no, there is no index on amount, so it wasn’t a misguided attempt to avoid an index access path…)
An interesting column comment encountered:
MOOD_SWINGS_IND VARCHAR2(1) (Y/N) “indicates that the person was in a swinging mood at the time of the episode”
Looking at the column comments on this table, I can sympathise with the poor soul who after painstakingly describing 100 columns just decided that it just wasn’t worth it…
table audit_history (column comments in quotes):
id number(18) “Unique identifier”
aud_table_name varchar2(30) “audit record table”
audit_action varchar2(50) “audit action e.g. update”
aud_timestamp date “timestamp of the audit”
aud_user_id varchar2(100) “user performing the action”
aud_col_1 varchar2(4000) “audit column 1”
aud_col_2 varchar2(4000) “audit column 2”
aud_col_3 varchar2(4000) “audit column 3”
… (etc.) …
aud_col_99 varchar2(4000) “audit column 99”
aud_col_100 varchar2(4000) “audit column 100”
aud_col_101 varchar2(4000)
aud_col_102 varchar2(4000)
… (etc.) …
aud_col_139 varchar2(4000)
aud_col_140 varchar2(4000)
Tip: Don’t let anyone even think about using this kind of design for their change history auditing requirements. It might look elegant to them, but it is not good. Just, don’t.
Came across this in a form (6i) to be run on a 9i db. Not only is this code about 33 lines of code too long and issues any number of unnecessary database queries, its name is quite unrelated to its intended function. Needless to say it was easily replaced with a single call to INSTR.
PROCEDURE alpha_check
(ref_in IN VARCHAR2
,ref_out OUT VARCHAR2) IS
-- Procedure included to distinguish
-- ref_in between ID or reference.
l_alpha_char VARCHAR2 (1);
l_alpha_pos NUMBER;
l_found_pos NUMBER;
l_search_string VARCHAR2 (100) := ' ';
Â
CURSOR cur_get_next_alpha(N NUMBER) IS
SELECT SUBSTR(l_search_string,N,1)
FROM dual;
CURSOR cur_check_for_alpha(C VARCHAR2)IS
SELECT INSTRB(ref_in,C, 1)
FROM dual;
BEGIN
IF ref_in IS NULL THEN
ref_out := 'X';
RETURN;
END IF;
FOR I IN 1..LENGTH(l_search_string) LOOP
OPEN cur_get_next_alpha(I);
FETCH cur_get_next_alpha
INTO l_alpha_char;
CLOSE cur_get_next_alpha;
FOR J IN 1..LENGTH(ref_in) LOOP
OPEN cur_check_for_alpha(l_alpha_char);
FETCH cur_check_for_alpha
INTO l_found_pos;
CLOSE cur_check_for_alpha;
IF l_found_pos > 0 THEN
ref_out := 'N';
RETURN;
END IF;
END LOOP;
END LOOP;
ref_out := 'Y';
EXCEPTION
WHEN OTHERS THEN
pc_ref_out := 'X';
END;
Looks like it may have been copied from the same source as “As bad as it gets”.
Seen in the wild:
SELECT ROWID
BULK COLLECT INTO t_rowids
FROM my_table
WHERE ...
FOR UPDATE NOWAIT;
IF t_rowids.COUNT > 0 THEN
FORALL i IN t_rowids.FIRST..t_rowids.LAST
DELETE FROM my_table
WHERE ROWID = t_rowids(i);
END IF;
Of course, we couldn’t just do a “DELETE FROM my_table WHERE …”, could we…
