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.