Code I Regret: Refactoring as Penance
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','email@example.com') ,i_recipients => EMAIL_PKG.address( EMAIL_PKG.address( 'jill', 'firstname.lastname@example.org') ,'bob', 'email@example.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 :)