SQL refactoring example

About a year ago I answered a question on the SAP Community Q&A site and have it on my posts-to-write list ever since to turn it into a blog post.

I feel that I learn a lot from seeing applied examples of concepts and rules – for example from this great presentation from Kevlin Henney that includes a “talk-through” a refactoring example. So, this is my example for others to review.

The premise of the question is rather common: a piece of code that was used on Oracle should be made to run on SAP HANA.
The OP had already managed to translate several parts of the Oracle code to work on SAP HANA but got stuck with an SQL Script error message.
The SAP HANA version used by the OP did not support BETWEEN as a check condition in an IF-clause. 
Current SAP HANA versions meanwhile have support for this but the code suffered from much more than just lack of syntactic sugar.
Here’s the original user-defined scalar function:

create FUNCTION "KABIL_PRACTICE"."F_KEEP_DATE"( TDATE DATE, TTODAY DATE) 
RETURNS RETVALUE CHAR
LANGUAGE SQLSCRIPT   
SQL SECURITY INVOKER AS
/******************************************************************************
NAME: F_KEEP_DATE
PURPOSE: RETURNS T/F WHETHER OR NOT THE DATE MEETS THE CRITERIA TO MAINTAIN DATA.
REVISIONS:
VER DATE AUTHOR DESCRIPTION
--------- ---------- --------------- ------------------------------------
1.0 6/18/2008 1. CREATED THIS FUNCTION.
******************************************************************************/
BEGIN
RETVALUE := 'F';
IF :TDATE BETWEEN ADD_DAYS (TO_DATE (:TTODAY, 'YYYY-MM-DD'), -30) AND :TTODAY 
OR  TO_CHAR( EXTRACT (MONTH FROM ADD_DAYS (TO_DATE (:TDATE, 'YYYY-MM-DD'), 1)))<> 
TO_CHAR( EXTRACT (MONTH FROM TO_DATE (TDATE, 'YYYY-MM-DD')))
THEN 
RETVALUE := 'T';
END IF;
--RETURN :RETVALUE;
END ;

Step 1: remove the fluff

First, remove the useless fluff of comments. The change history of the code is captured in the code repository, so keeping a log of changes is pointless here.
This implies that there is a code repository, of course. SAP HANA comes with the repository for XS (classic) and git support for XSA out of the box and both options allow for distributed development and change tracking of development resources.
Just removing the comments and adding line-breaks and some indentation gets us to this:

create FUNCTION "KABIL_PRACTICE"."F_KEEP_DATE"
                    ( TDATE DATE, TTODAY DATE) 
RETURNS RETVALUE CHAR
LANGUAGE SQLSCRIPT   
SQL SECURITY INVOKER AS
BEGIN
    RETVALUE := 'F';
    
    IF :TDATE BETWEEN ADD_DAYS (TO_DATE (:TTODAY, 'YYYY-MM-DD')
                               ,-30) 
              AND :TTODAY 
       OR TO_CHAR(EXTRACT(MONTH FROM ADD_DAYS 
                          (TO_DATE (:TDATE, 'YYYY-MM-DD'), 1)))
           <> TO_CHAR(EXTRACT(MONTH FROM 
                           TO_DATE (:TDATE, 'YYYY-MM-DD')))
    THEN 
        RETVALUE := 'T';
    END IF;
END;

Step 2: Find out what the code should do

 It’s a single IF statement, so the job of this function is to encapsulate the condition check.
Let’s focus on that and find out what are the conditions checked here:

:TDATE BETWEEN ADD_DAYS (TO_DATE (:TTODAY, 'YYYY-MM-DD'),-30) 
           AND :TTODAY 
OR 
   TO_CHAR(EXTRACT(MONTH FROM ADD_DAYS 
                          (TO_DATE (:TDATE, 'YYYY-MM-DD'), 1)))
<> TO_CHAR(EXTRACT(MONTH FROM 
                           TO_DATE (:TDATE, 'YYYY-MM-DD')))

We see that there are in fact two conditions of which either one needs to be true to make the whole function true.
Still, with all the data type conversion in place, it is hard to see, what is being checked here. 
So, let’s get rid of them, where we can.

Step 3: remove type conversions where possible

Something I see quite a lot in converted code is that date values get converted into text values and back again. This is probably due to practices on Oracle, but on SAP HANA, date values can be used directly without the need for conversion or data format specification. A date does not have a specific format, this only comes into play when the display should be printed to the screen. More on this here.

:TDATE BETWEEN ADD_DAYS (:TTODAY, -30) AND :TTODAY 
OR 
   EXTRACT (MONTH FROM ADD_DAYS (:TDATE, 1)) 
<> EXTRACT (MONTH FROM :TDATE) 

Now, we can see, that the first condition checks whether :TDATE is within the past 30 days.

The second condition checks if the day after :TDATE is still in the same month as :TDATE.
That means, this condition checks if :TDATE is the last day of the month.

Summarizing, the function returns 'T' (TRUE) when the provided date is either within the last 30 days or when the provided date is the last day of the month.

Checking the Datetime functions in SAP HANA we find a couple of useful functions that we can use to make this code clearer. 

Step 4: make use of built-in functions

The LAST_DAY function returns the last day of the month for the date it is used on, saving us the manual steps of extracting the month and checking if the next day would be a different month.

   :TDATE BETWEEN ADD_DAYS (:TTODAY, -30) AND :TTODAY 
OR :TDATE = LAST_DAY(:TDATE) 

Step 5: Pick names that help understanding the code

Removing the TTODAY variable is a bit tricky here; and we should remove it in order to make the function easier to understand. 

This variable could, in fact, be used as a flexible reference or anchor date. In this case, it should be renamed to something like reference_date.

But I have the suspicion that it really is always only used with today’s date. This allows us to remove the parameter altogether. 

We also should pick a better name for both the parameter and the function itself and change the return value to the more commonly used integer type as a substitute for a BOOLEAN.

The original comment indicated that the purpose of the function is to check whether data was still in a time window during which it can be modified.

I am not really happy with dataInEditPeriod for the function name but it sure is more obvious than F_KEEP_DATE.
Indicating the object type (‘F‘ for function? ‘SP‘ for stored procedure?) in the object name does not make it easier to understand the code.
If I can access the database via SQL I can easily look up what type any object has.
The remaining “KEEP_DATE” part is problematic as it refers to the intended usage scenario of the function (determining whether data should be kept). This says something about what the function shall be used for instead of what it does. 
My proposed alternative dataInEditPeriod tries to convey what the function does on the semantic level of the data model. Here, we are dealing with data that has a time frame in which it can be changed: “EditPeriod” and the function checks if the data is in this period.

As for the parameter, how about checked_day?

Finally, swap the clumsy RETVALUE for result and we get this:

create FUNCTION "dataInEditPeriod"(checked_day DATE) 
RETURNS result INTEGER
LANGUAGE SQLSCRIPT   
SQL SECURITY INVOKER AS
BEGIN
    result := 0;
    IF     (:checked_day BETWEEN ADD_DAYS (current_date, -30) 
                                 AND current_date) 
       OR  (:checked_day = LAST_DAY(:checked_day))
    THEN 
        result := 1;
    END IF;
END;

Step 6: review how the function gets used

The calling code would then read like this:

begin
    if "dataInEditPeriod" (:CC_DATE) = 1
    then
        ...sql statements...
    else 
        ...sql statements...;
    end if;
end;

That is not perfect, but I’d say it is heaps better to understand than your starting position.