Re: Bad Code Alert

[Alan Campin <alan0307d@xxxxxxxxx>]

Birgitta, must disagree strongly.

One. Not checking the open status. The error that needs to be reported is
the error that is returned from the Open statement, not from the fetch. I
again reemphasize , each statement except a declare needs to be error
checked and processing stopped if an error is detected.

Two. I specifically that warning should be reported as errors. You used as
an example of a warning that not enough variables provides. How is that
less of a disaster? Your program is expecting three variables but you only
load two. That could cause all kinds of problems. If a specific warning
message should be ignore, it should specifically be coded. You just cannot
ignore warning message. They are indicators of bigger problems.



On Sun, Mar 18, 2018 at 11:20 PM, Birgitta Hauser <Hauser@xxxxxxxxxxxxxxx>
wrote:

> Alan,
>
> Not sure what was your problem.
> First it was a code snippet with the focus on how to handle NULL values in
> an embedded SQL RPG Program.
> And second I added a comment (//Handle SQL Error) where to handle an SQL
> error.
> I could have included another check after the OPEN, but If the OPEN fails,
> the FETCH will fail, too.
>
> If you already want to include an error handling, than do it correctly.
> If you want to check the SQL State, have check the status group (the first
> 2
> characters) and not the complete SQLSTATE (may be in exceptions).
> If the first 2 characters are 00 than everything is ok, that does not mean
> it must be always 00000.
> If the first 2 characters are 01 than a warning occurs. That means data is
> returned, but something should be considered (for example SQLSTATE 01503 -
> The number of result columns is larger than the number of variables
> provided.)
> If the first 2 characters are 02 than no record is returned.
> All other status groups mark an SQL Error.
>
> From the IBM i Knowledge Center:
> The class code of an SQLSTATE value indicates whether the SQL statement was
> executed successfully (class codes 00 and 01) or unsuccessfully (all other
> class codes).
>
> For checking an SQL Error, I prefer the SQLCODE because it is easier to
> check for < *Zeros and = 100, than substringing the SQLSTATE.
>
> Instead of throwing an Dummy SQL Message, I'd suggest to trap the real SQL
> Error Message. ... whether you want add the SQLSTATE or not is your
> decision.
> DCL-S LocErrMsg   VarChar(256);
> ...
> If        %Subst(SQLSTATE: 1: 2) <> '00'
>    and %Subst(SQLSTATE: 1: 2) <> '01'
>    and %Subst(SQLSTATE: 1: 2) <> '02';
>    Exec SQL  Get Diagnostics Condition 1 :LocErrMsg = MESSAGE_TEXT;
>    YourErrorMsgHandling(%Trim(LocErrMsg) + ' ' + SQLSTATE);
> End-If;
>
> We even check the SQLCODE in the SQL Error Handling Procedure. If an error
> occurred an escape message is sent, otherwise the procedure is left
> immediately.
>
> Mit freundlichen Grüßen / Best regards
>
> Birgitta Hauser
>
> "Shoot for the moon, even if you miss, you'll land among the stars." (Les
> Brown)
> "If you think education is expensive, try ignorance." (Derek Bok)
> "What is worse than training your staff and losing them? Not training them
> and keeping them!"
> „Train people well enough so they can leave, treat them well enough so they
> don't want to.“ (Richard Branson)
>
>
> -----Original Message-----
> From: RPG400-L <rpg400-l-bounces@xxxxxxxxxxxx> On Behalf Of Alan Campin
> Sent: Sonntag, 18. März 2018 23:56
> To: RPG programming on the IBM i / System i <rpg400-l@xxxxxxxxxxxx>
> Subject: Bad Code Alert
>
> Brigitta, this is not a personal attack on you. I see this error all over
> the place at my company and I keep trying to get it stopped.
>
> Recently the following code snippet was presented.
>
> Exec SQL Declare CsrC01 cursor for
>                      Select TS1, VarChar1, TS2, Decimal1
>                          From ...
>                         Where ...;
>
> Exec SQL Open CsrC01;
>
> DoU 1=0;
>     Exec SQL  Fetch Next From CsrC01
>                     into :LocTS1 :LocTS1Ind, :MyVarChar1,
>                          :LocTS2 :LocTS2Ind, :MYDec1;
>     If    SQLCODE = 100
>        or SQLCODE < *Zeros;
>        //Handle SQL Error;
>        Exec SQL Close CsrC01;
>        Leave;
>     EndIf;
>
> This code represents a severe problem. The problem. No SQL error checking.
>
> It is absolutely critical that SQL statement be error checked. If an error
> occurs on a File I/O, an error is thrown but unless the error is critical
> the only thing that happens in SQL is that an error code is returned in
> SQLSTATE (I use SQLSTATE. SQLCODE can also be used).
>
> This is all that happens. If an SQL error is not checked the program will
> simply continue on. I cannot tell you the number of times that I have gone
> into SQL programs running in production that I determined  where failing
> and
> had been failing for years. The effect from those errors was minor to
> severe.
>
> The above code should look like the following.I am using my standard error
> handler for these examples because it throws an exception message and stops
> processing. . Note that I have no error checking on Declare. It does not
> generate any machine code and does not need to be error checked.
>
> dcl-c SQL_STATE_NO_ROW '02000';
> dcl-c SQL_STATE_OK '00000';
>
> Exec SQL Declare CsrC01 cursor for
>             Select TS1, VarChar1, TS2, Decimal1
>               From ...
>               Where ...;
>
> Exec SQL Open CsrC01;
> If SqlState <> SQL_STATE_OK;
>   // Report error and stop processing.
>   XVERRH_Throw('CPF9898':
>                CPF_MESSAGE_FILE_NAME:
>                'Open Cursor CsrC01 Failed! SqlState = ' + SqlState); EndIf;
>
> DoU 1=0;
>   Exec SQL  Fetch Next From CsrC01
>               into :LocTS1 :LocTS1Ind, :MyVarChar1,
>                    :LocTS2 :LocTS2Ind, :MYDec1;
>   Select;
>     When SqlState = SQL_STATE_NO_ROW;
>       Exec SQL Close CsrC01;
>       Leave;
>     When SqlState = SQL_STATE_OK;
>       // process record.
>     Other;
>       // Report error and stop processing.
>       XVERRH_Throw('CPF9898':
>                    CPF_MESSAGE_FILE_NAME:
>                    'Fetch Next From CsrC01 failed! Sql State = ' +
> SqlState);
>   EndSl;
> EndDo;
>
> I want to emphasize again the importance of stopping processing. If an
> error
> occurs, we must stop processing. If we continue, we are just compounding
> the
> problem.
>
> Please note also that I am stopping processing for both warning and
> errors.It has been my experience that warning error are always masking some
> form of programming error and need to be investigated.
>
> I appreciate their may be certain rare condition were we want processing to
> continue after a warning but they are the exception.
>
> I again apologize if I have hurt anyone feeling but I felt it was important
> to bring this to the groups attention. SQL error checking is absolutely
> critical.
> --
> This is the RPG programming on the IBM i (AS/400 and iSeries) (RPG400-L)
> mailing list To post a message email: RPG400-L@xxxxxxxxxxxx To subscribe,
> unsubscribe, or change list options,
> visit: https://lists.midrange.com/mailman/listinfo/rpg400-l
> or email: RPG400-L-request@xxxxxxxxxxxx
> Before posting, please take a moment to review the archives at
> https://archive.midrange.com/rpg400-l.
>
> Please contact support@xxxxxxxxxxxx for any subscription related
> questions.
>
> Help support midrange.com by shopping at amazon.com with our affiliate
> link:
> http://amzn.to/2dEadiD
>
> --
> This is the RPG programming on the IBM i (AS/400 and iSeries) (RPG400-L)
> mailing list
> To post a message email: RPG400-L@xxxxxxxxxxxx
> To subscribe, unsubscribe, or change list options,
> visit: https://lists.midrange.com/mailman/listinfo/rpg400-l
> or email: RPG400-L-request@xxxxxxxxxxxx
> Before posting, please take a moment to review the archives
> at https://archive.midrange.com/rpg400-l.
>
> Please contact support@xxxxxxxxxxxx for any subscription related
> questions.
>
> Help support midrange.com by shopping at amazon.com with our affiliate
> link: http://amzn.to/2dEadiD