Defensive Programming with PowerBuilder – EMEC – Part II
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.
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.