Monday, April 14, 2008

Fancy code

I was reminded about this blog's motto when I watched a piece of production code last week. It was located as a subfunction at the end of the declaration section of a monster procedure containing 1500 lines. At least some form of modularization has kicked in, was my first thought after seeing the smallish function. This thought was immediately followed by a "and quite a clear name too". Then I looked at what the function was doing. I'll let you experience the same. Here it is:

FUNCTION strip_transaction_rolled_back
( p_sqlerrm IN VARCHAR2
) RETURN VARCHAR2 DETERMINISTIC
IS
SUBTYPE msg_code_type IS VARCHAR2(9);
cn_ora_ CONSTANT VARCHAR2(4) := 'ORA-';
cn_transaction_rolled_back CONSTANT msg_code_type := cn_ora_||'02091';
BEGIN
RETURN
( CASE
WHEN ( (INSTR( str1 => p_sqlerrm
, str2 => cn_transaction_rolled_back
) = 1
)
AND (INSTR( str1 => p_sqlerrm
, str2 => cn_ora_
, pos => (1 + LENGTH( ch => cn_transaction_rolled_back
)
)
) != 0
)
)
THEN
SUBSTR( str1 => p_sqlerrm
, pos => (INSTR( str1 => p_sqlerrm
, str2 => cn_ora_
, pos => (1 + LENGTH( ch => cn_transaction_rolled_back
)
)
)
)
)
ELSE
p_sqlerrm
END
);
END strip_transaction_rolled_back;


The function accepts an error message and returns the same error message, unless the error message starts with ORA-02091 and is followed by another ORA-message. In which case the first ORA-02091 message is stripped off.

You can argue about the layout and all the superfluous brackets, but that's a matter of personal style I want to leave out of the discussion. Three aspects struck me the most:

1. the deterministic clause
2. the subtype
3. the use of named notation for the parameters of the Oracle-functions instr and substr.

1. The deterministic clause says that the outcome of the function will be the same given the same set of inputs. You need this one when you want to define a function based index or a materialized view that is marked REFRESH FAST or ENABLE QUERY REWRITE (just read about that one) or a function based virtual column. But we are dealing with a subfunction of a packaged procedure ... I did an impact analysis for the function to convince myself that the function was only used inside the procedure, and that the code wasn't copied elsewhere. And I got convinced.

2. Then the subtype. This one allows for tidier coding when you want to use a type throughout your package, and there is a chance the datatype may change some time in the future. Maintenance can then be reduced by only changing the datatype of the subtype instead of all the occurences scattered throughout the package. You can also nicely restrict the subtype to a smaller range. But here in a subfunction, with its only use being two lines below? I don't get the added value of this one either.

3. Using named notation for parameters is generally nice, since you immediately document your code somewhat in the process. Here it is applied to the Oracle functions INSTR and SUBSTR. The formal name of the parameters isn't even documented, as you can see here for INSTR and here for SUBSTR. The names used in the documentation are not the real names as can be seen by describing the sys.standard package:

rwijk@ORA11G> desc sys.standard
... ...
FUNCTION INSTR RETURNS BINARY_INTEGER
Argumentnaam Type In/Out Standaard?
------------------------------ ----------------------- ------ --------
STR1 VARCHAR2 IN
STR2 VARCHAR2 IN
POS BINARY_INTEGER IN DEFAULT
NTH BINARY_INTEGER IN DEFAULT
... ...
FUNCTION SUBSTR RETURNS VARCHAR2
Argumentnaam Type In/Out Standaard?
------------------------------ ----------------------- ------ --------
STR1 VARCHAR2 IN
POS BINARY_INTEGER IN
LEN BINARY_INTEGER IN DEFAULT
... ...


But what's the use of using the undocumented names of the parameters of Oracle functions? I didn't even know them.

So, we have a subfunction created by a developer that has "above average" knowledge of PL/SQL: not every developer uses the deterministic clause, a subtype and the named notation for Oracle-functions. But each use has the effect to confuse the next developers maintaining the code. At least that was the effect it had on me. Add to that the clumsy use of "INSTR(...) = 1" and "INSTR(...) != 0" constructs, which can be rewritten using the LIKE operator for enhanced readability and you end up with the code above.

All of which could have been avoided when you would clearly phrase what it was supposed to do: in case the error message starts with 'ORA-02091' and contains another ORA-message, then strip off the first ORA-02091 message, else leave the error message as is. Or in code:

FUNCTION strip_transaction_rolled_back
( p_sqlerrm IN VARCHAR2
) RETURN VARCHAR2
IS
BEGIN
RETURN
CASE
WHEN p_sqlerrm LIKE 'ORA-02091%ORA-%'
THEN
SUBSTR(p_sqlerrm,INSTR(p_sqlerrm,'ORA-',1,2))
ELSE
p_sqlerrm
END
;
END strip_transaction_rolled_back;


You can add some comments too about what the function does, but it is almost unnecessary now. And that is exactly what the motto is about. Keep it simple.

6 comments:

  1. Then you lose the wow effect :-) Maybe he/she is paid for the number of lines of code produced :-)

    ReplyDelete
  2. It seems (so-called) hard-core developers are like accounts people. They don't like something when it is simple.

    ReplyDelete
  3. You seem to have lost the deterministic clause. Why?

    ReplyDelete
  4. To yas: I know for fact that we are not paid for the number of lines of code produced here :-). The wow effect is probably closer to the truth.

    To Arne: I dropped the deterministic clause, because it has no added value and only leads to confusion. The "And I got convinced" should be read as "And I got convinced that the code was only used here, that it wasn't copied elsewhere, and so that the deterministic clause is unnecessary."

    ReplyDelete
  5. Maybe the original author specified DETERMINSTIC so that the function wouldn't have to logically derive the return value if/whenever it was called again with the same parameter value. Since it could just return previously calculated results, then maybe he could save a fraction of a second of CPU time within a year or two if it gets called enough.

    ReplyDelete
  6. Rick,

    I answered your question in this separate post.

    Regards,
    Rob.

    ReplyDelete