Defensive Programming with PowerBuilder - EMEC - Part II

[Former Member]

In the first part of this article we were focused on how to produce "Easy to Maintain" code, now it's time to focus on effectiveness.

What is an "Effective code" ?

An effective code is a script that offer the best performances in balance with the readability and maintainability of its code.

PowerBuilder Specificities

As you certainly already known, PowerBuilder is an interpreted language where PowerScript are compiled into Pseudo-Compiled code invoking the PowerBuilder Primitives buried in their Run-times DLL. This mean that there is no other possibilities to improve their performances than rewriting them in another language like C, C++, Java or .Net, depending of the version used.

As the overall performances of PowerScript are good, this will certainly not worth the investment in time and money for the average business management application usually made with PowerBuilder!

So what, there is no possibilities to improve the effectiveness of our PowerScript ? Yes, of course by applying the following commandments :

1. Never you will invoke unnecessary code and you will quit the script ASAP, and vice versa

2. The shorter the script will be, better the performances will be

3. The right data type you will use

4. The KISS principle you will apply

5. Dynamic call you will avoid as much as possible

At first sight, this seems so obvious, but did you look a little bit more further in your code ?

Check the following code that is similar to the majority of the code found in average PowerBuilder application:

of_SetReportLayout ( integer ai_mode, datawindow adw_report, string as_parameters ) return integer

integer li_rc = 1

choose case ai_mode

     case CST_PREVIEW, CST_LIST, CST_FORM

          ii_mode = ai_mode

     case else

          li_rc = -1

end choose

if li_rc = 1 then

     if isvalid( adw_report ) and not isnull( adw_report ) then

          if parent.dynamic of_SetDataobject( adw_report ) = 1 then

               if len( as_parameters ) > 0 then

                   li_rc = parent.dynamic of_Refresh( adw_report, as_parameter )

               else

                    li_rc = -1

               end if

          else

               li_rc = -1

          end if

     else

          li_rc = -1

     end if

end if

return li_rc

Try to figure out what is not good with this script ?

Yeah, you got it : all commandments are transgressed, but let's take a look at it a little bit deeper...

1. Wrong checking order of parameters during their validation

The first problem with this code is the way it validate the received parameters values. Indeed, it transgress the first commandment because it should have tested the validity of the specified datawindow before anything, and it does not quit the script ASAP.

This problem is often stated in production codes because we naturally follow the order of apparition of the parameters  in the method's prototype being edited while coding the parameters validation section.

To fulfill the 1st commandment, we should think about what parameter is susceptible to allow us to quit directly the script in case it is not validated.

Most of the time, it will concern object passed as parameter because usually the rest of the code depend of its validity.

Using this technique you will quit the script ASAP and by the way avoid to call unnecessary code.

2. Code is too long for what it does

Although, this has not impact on the real performance of the generated code, it makes it difficult to read and to understand, so the code is not Effective.

3. Code structure is too complex

For the same reason than in point 2 above, the IF structure is too complex. This is coming from a problem of "separation of concern" where the code issue checks things that could or should have been done inside the called methods themselves (delegation) as the validation was not properly done before.

4. Usage of a single point of exit of the code and of useless variable

This technique coming from "another age" has nothing to do on such relatively short method except for a temporary debugging purpose, at most !

Forcing the code to run until the last line of the code even if every parameters are wrong !

Furthermore, the usage of the li_rc variable does not provide any advantage because the only value it could have is 1 or -1.

5. Usage of dynamic call

This technique should be avoided as much as possible although it could be acceptable in a generic framework implementation which is visibly not the case here.

6. Wrong datatype used for return code

This problem is due to the way PowerBuilder Compiler handle integer number. In fact, it always cast integer data type to long internally to avoid rounding issues, basically speaking, leading to automatic performance degradation by design. So instead of using integer we should always use long as return values for our methods.This is a major issue as everybody everywhere use integer value for this, me included (but I tried to heal it..) !

So like me, try to take the habit to use long as return value for every new methods, and when editing existing one still using integer, change them directly!

The following version of the code is far more Effective :

of_SetReport ( integer ai_mode, datawindow adw_report, string as_parameters ) return long

if parent.dynamic of_SetDataObject( adw_report, ai_mode ) = -1 then return -1

if parent.dynamic of_Refresh( adw_report, as_parameter ) = -1 then return -1

return 1

You will have noticed that:

• The name of the method has been modified to of_SetReport instead of of_SetReportLayout as it describe more what it really does and as it does two things, the name should have be more general.
• Only the fifth commandment has been transgressed just because we did not known what type of parent this script has, so the dynamic call can be acceptable
• We use simple and straight code leading to exit the script as soon as possible and thereby, avoiding to call unnecessary code.
• All the needed checks are delegated inside the called methods (delegation)
• Notice the change in the prototype of the of_SetDataobject method where now the datawindow parameter is the first one now, leading to test it naturally as the first parameter
• We do not use useless variable

Pushing to the extreme the code could have been modified this way:

of_SetReport ( integer ai_mode, datawindow adw_report, string as_parameters ) return long

w_report     lw_parent

lw_parent = this.getParent()

if isnull( lw_parent ) or not isvalid( lw_parent ) then return -1

if lw_parent.of_SetDataObject(  ai_mode, adw_report ) = -1 then return -1

if lw_parent.of_Refresh( adw_report, as_parameter ) = -1 then return -1

return 1

or even :

of_SetReport ( integer ai_mode, datawindow adw_report, string as_parameters ) return long

if isnull( w_report ) or not isvalid( w_report ) then return -1

if w_report.of_SetDataObject(  ai_mode, adw_report ) = -1 then return -1

if w_report.of_Refresh( adw_report, as_parameter ) = -1 then return -1

return 1

But truely speaking, I prefer to first solution that present the best balance between Maintainability and Effectiveness.

This articles as demonstrated, once again, that there exists multiples solutions to solve encountered problems but that the solution, in PowerBuilder, is always driven by the described five commandments above in order to produce EMEC code.

It has also pictured that improving the code is almost always linked to the quality of the overall design of the solution.

This is why Code Reviewing sessions should be done regularly, along Refactoring one when needed and as soon as possible.

Hope this will help in your everyday tasks as PowerBuilder consultant.

Regards,