For quite a long time I did not work anymore with PL/SQL and was quite happy when I had the chance to review some code at a customer. The status today: I am not that happy anymore 🙁 Let me explain why and show you some examples on what was discovered. Of course all of the identifiers have been obfuscated and this is not to blame anyone. It is more to make people aware of what you should not do and that you have to be as exact as possible when you do programming. Even more important: Code that you write must be maintainable and understandable by others.
We start with a simple “if-then-else-end if” block:
IF c <= d THEN IF c = d THEN l2 := l2 || l3; ELSE l2 := l2 || l3 || ','; END IF; END IF;
So what is wrong with this? The code does what it is supposed to do, true. But this is more complicated than it needs to be. Lets look at the first line:
IF c <= d
If we enter this “IF” then we know that c is either less than d or equal to d, correct? On line 3 we know that c is d if we enter that “IF”. What does this imply for line 6 (the “ELSE”)?
It implies that c is less than d when we enter the “ELSE”. But then we could also write it like this:
IF c = d THEN l2 := l2 || l3; ELSIF c < d THEN l2 := l2 || l3 || ','; END IF;
This is much more clear and less code. We are only interested if c is equal to d or less than d, that’s it. Then we should design the code exactly for that use case.
The next example is about the usage of a record. This is the code block:
... ll_col1 SCHEMA.TABLE.COLUMN1%TYPE; ll_col2 SCHEMA.TABLE.COLUMN2%TYPE; ll_col3 SCHEMA.TABLE.COLUMN3%TYPE; ll_col4 SCHEMA.TABLE.COLUMN4%TYPE; ... DECLARE TYPE MyRecord IS RECORD ( l_col1 SCHEMA.TABLE.COLUMN1%TYPE, l_col2 SCHEMA.TABLE.COLUMN2%TYPE, l_col3 SCHEMA.TABLE.COLUMN3%TYPE, l_col4 SCHEMA.TABLE.COLUMN4%TYPE ); rec1 MyRecord; BEGIN v_sql := 'SELECT col1, col2, col3, col4 FROM SCHEMA.TABLE WHERE col3 = ''' || ll_col3 || ''''; EXECUTE IMMEDIATE v_sql INTO rec1; ll_col1 := rec1.l_col1; ll_col2 := rec1.l_col2; ll_col3 := rec1.l_col3; ll_col4 := rec1.l_col4; EXCEPTION WHEN NO_DATA_FOUND THEN ll_col3 := 0; END;
Beside that declaring a new block inside an existing block is really ugly what is wrong here? There is the definition of a new record which is then used to fetch the data from the dynamic sql statement. Nothing wrong here. But then the fields of this record are written back to four variables which are valid in and outside of that block, why that? Probably because the record is only valid in the “declare-begin-end” block. But for what do I need the record then at all? Really confusing 🙂
The next one is more about how easy it is to understand code written by someone else. This is the code block:
SELECT COUNT (*) INTO l_count FROM USER_INDEXES WHERE INDEX_NAME = '' || l_index_name || ''; IF l_count > 0 THEN EXECUTE IMMEDIATE 'DROP INDEX ' || '' || l_index_name || ''; END IF;
I really had to read this carefully until I understood why it was written this way. Probably the developer had this intension: “I want to know if a specific index exists and if yes, then I want to drop it”.
But what he did actually code is: I want to know if there are multiple indexes with the same name. As the query is against user_tables the answer can not be more than 1, correct? But then the correct and much more easy to understand way of writing this would be:
SELECT COUNT (*) INTO l_count FROM USER_INDEXES WHERE INDEX_NAME = '' || l_index_name || ''; IF l_count = 1 THEN EXECUTE IMMEDIATE 'DROP INDEX ' || '' || l_index_name || ''; END IF;
This is just a small change and does not affect the functionality at all but it is much more clear. Even more clear would be something like this:
BEGIN SELECT 'index_exists' INTO l_exists FROM USER_INDEXES WHERE INDEX_NAME = '' || l_index_name || ''; EXCEPTION WHEN NO_DATA_FOUND THEN l_exists := 'index_does_not_exist'; END; IF l_exists = 'index_exists' THEN EXECUTE IMMEDIATE 'DROP INDEX ' || '' || l_index_name || ''; END IF;
Using our language we can make the code much more easy to understand. Beside that the concatenation in the where clause and in the execute immediate statement is not required so the final code could look like this:
BEGIN SELECT 'index_exists' INTO l_exists FROM USER_INDEXES WHERE INDEX_NAME = l_index_name; EXCEPTION WHEN NO_DATA_FOUND THEN l_exists := 'index_does_not_exist'; END; IF l_exists = 'index_exists' THEN EXECUTE IMMEDIATE 'DROP INDEX ' || l_index_name; END IF;
In the next post we’ll look at some other examples which can be improved by changing the way we think about what we code and how others may interpret it.