Question more efficient query to modify

TomBascom

Curmudgeon
Ok so if I devide my 1 procedure into two procedure to do each updates is this better ?

You are only showing tiny little bits of hypothetical code so it is difficult to say if that would be "better" or not.

But there is a lot to be said for using a small carefully crafted procedure for the update.
 
So I have a browse wich directly connect to the customer table (without temp-table in between).

The user can select one row on the browse and by button update a value to validate the row.

This is my actual code that you said is write in a bad way:
Code:
DEFINE BUFFER updCustomer FOR Customer. /* Buffer for update */
DEFINE BUFFER bcCustomer  FOR Customer. /* Buffer for reading next rows in the tables */

1 DO FOR updCustomer TRANSACTION :
1    FIND updCustomer WHERE ROWID(updCustomer) = ROWID(Customer) EXCLUSIVE-LOCK NO-WAIT NO-ERROR.
1    IF AVAILABLE updCustomer THEN
1        updCustomer.validate = true.
1 END.

IF Customer.international = TRUE THEN DO:
    FIND bcCustomer WHERE bcCustomer.country = Customer.country
                    AND   bcCustomer.position = Customer.position + 1 NO-LOCK NO-ERROR.
                    
    IF AVAILABLE bcCustomer AND some other condition THEN DO:
2        DO FOR updCustomer TRANSACTION :
2            FIND updCustomer WHERE ROWID(updCustomer) = ROWID(bcCustomer) EXCLUSIVE-LOCK NO-WAIT NO-ERROR.
2            IF AVAILABLE bcCustomer THEN DO:
2                updCustomer.validate = true.
2            END.
2        END.
    END.
END.

So I was thinking of put 1 and 2 in two other procedures ?

Also I didn't write the code for managing error.
 

TomBascom

Curmudgeon
For what I see here, no, I would not create two different procedures. I would create a single "updateCustomer_validate" procedure since that code is reused without any changes.

You have "Customer", "bcCustomer" and "updCustomer". It might be because I lack an understanding of the code that this is taken from but I do not understand what purpose bcCustomer serves nor where Customer gets any initial values from. Obviously this is just a small part of something else and, so far, all of the code that you have been showing is written in a way that implies that there will be numerous side effects and scoping issues. So I will guess that you have previously done a "FIND Customer". I'm not sure what you are trying to do with finding customer.position + 1 but I kept it around for illustration.

I think that would replace the code that you have shown with:

Code:
procedure updateCustomer_validate:

  define input parameter customerRowid as rowid no-undo.
  define input parameter custValidate as logical no-undo.

  define buffer updCustomer for customer.

  define variable 

  do for updCustomer transaction:

    find updCustomer exclusive-lock where rowid(  updCustomer ) = customerRowid no-wait no-error.
    if available( updCustomer ) then
     assign
      updCustomer.validate = custValidate
    .
   else
    do:
      /* perhaps handle available() = false... */
    end.

  return.

end.

/* at some point FIND Customer must have occurred... */

run updateCustomer( rowid( Customer ), true ).

if customer.international = true then
  do:

    find Customer no-lock where Customer.country = Customer.country
           and Customer.position = Customer.position + 1 no-error.
                    
    if available( Customer ) and ( some other condition ) then
      do:
        run updateCustomer( rowid( Customer ), true ).
      end.

  end.
 
@TomBascom Thank you for your precious advice.
In fact I use the customer table instead of the real table I work with. And maybe my mistake was there to not show you the real code.

I work on application that manage a signature workflow. The aim is to allow some user to make a request for buing something and there demand as to go thourgh a number of approval/signature before they can buy the items list.

So the scenario is as below:
1. The user how can give is approval open the application and have in front of him a browse widget with the list of buying demand that ask for is approval.
2. He select one of them and can by using two buttons give is approval or refused it.
3. My code is when he it the approval button.
4. The table that contain the signature info work like this :
name : DAWFLW
Fields: a. danum, that like the signature to the buying request
b. numsig, for each buying request there is 3 or 4 signature. Because I have to call of them in a certain order I need this field
c. orisig, the use that can sig
d. stasig, status of the signature (0 waiting for it, 1 approved, 2 refused)
And many more field.

My code is as below:
Code:
DO:

DEFINE VARIABLE cDero    AS CHARACTER    NO-UNDO. /* Liste of user in case of proxy signature */
DEFINE VARIABLE iNextSig AS INTEGER      NO-UNDO. /* Number of the next signatory guy */

DEFINE BUFFER updDAWFLW FOR DAWFLW. /* Buffer for update */
DEFINE BUFFER bcDAWFLW  FOR DAWFLW. /* Because the table is directly link to a Browse I need a buffer for reading other line */
/* We applied the signature */

IF DAWFLW.orisig <> c-uticod THEN /* If proxy signature we build a list of usercod to mail them at the end */
    cDero = DAWFLW.orisig + ",":U + c-uticod.

DO FOR updDAWFLW TRANSACTION :
    FIND updDAWFLW WHERE ROWID(updDAWFLW) = ROWID(DAWFLW) EXCLUSIVE-LOCK NO-WAIT NO-ERROR.
    ASSIGN
        updDAWFLW.utisig = c-uticod
        updDAWFLW.datsig = TODAY
        updDAWFLW.hmssig = STRING(TIME, "HH:MM:SS")
        updDAWFLW.stasig = 1
        updDAWFLW.remarq = FI-Remarq:SCREEN-VALUE. /* Info write done by the signatory guy */
END.

/* We search for the type of buying request */
FIND FIRST VAFDAI WHERE VAFDAI.danum = SACDPE.acddemnum NO-LOCK NO-ERROR.

IF NOT AVAILABLE VAFDAI THEN DO: /* standard buying request: 3 signatory */
    IF DAWFLW.numsig < 3 THEN DO: /* the signatory is not the last of the list */
        
        FIND bcDAWFLW WHERE bcDAWFLW.danum  = DAWFLW.danum
                      AND   bcDAWFLW.numsig = DAWFLW.numsig + 1 NO-LOCK NO-ERROR.
        /* Send email to the next seignatory */
        RUN 2-prc-envoiemail("nextsig":u,bcDAWFLW.orisig). 
    END.
    ELSE DO: /* Last signatory */
        RUN 3-prc-DaToCde. /* procedure to transform the request into an order */
        /* Send email to the requester */
        RUN 2-prc-envoiemail("accept":u,SACDPE.dpeutidem).
    END.
END.

ELSE DO: /* buying request on project: 4 signatory */
    IF DAWFLW.numsig < 4 THEN DO:
    
        FIND bcDAWFLW WHERE bcDAWFLW.danum  = DAWFLW.danum
                      AND   bcDAWFLW.numsig = DAWFLW.numsig + 1 NO-LOCK NO-ERROR.
                      
        IF AVAILABLE bcDAWFLW AND bcDAWFLW.cansig AND bcDAWFLW.numsig = 2
            AND (bcDAWFLW.orisig = c-uticod OR bcDAWFLW.orisig = SACDPE.dpeutidem) THEN DO:
            DO FOR updDAWFLW TRANSACTION :
                FIND updDAWFLW WHERE ROWID(updDAWFLW) = ROWID(bcDAWFLW) NO-ERROR.
                ASSIGN
                    updDAWFLW.utisig = updDAWFLW.orisig
                    updDAWFLW.datsig = TODAY
                    updDAWFLW.hmssig = STRING(TIME, "HH:MM:SS")
                    updDAWFLW.stasig = 1
                    updDAWFLW.remarq = ""
                    updDAWFLW.autsig = TRUE.
            END.

            /* Mail to inform the request or the actual user that is approval is automaticly applied for the next signature request on this buying demand */
            RUN 2-prc-envoiemail("autsig":u, bcDAWFLW.orisig).

            FIND bcDAWFLW WHERE bcDAWFLW.danum  = DAWFLW.danum
                          AND   bcDAWFLW.numsig = DAWFLW.numsig + 2 NO-LOCK NO-ERROR.

            /* Sending mail to the next next signatory */
            RUN 2-prc-envoiemail("nextsig":u, bcDAWFLW.orisig).
        END.
        ELSE DO:
            /* Sending mail to the next signatory */
            RUN 2-prc-envoiemail("nextsig":u, bcDAWFLW.orisig).
        END.
    END.
    ELSE DO:
        RUN 3-prc-DaToCde.
        /* sending mail to inform the requester that is demand is approved */
        RUN 2-prc-envoiemail("accept":u, DAWFLW.orisig).
    END.
END.

IF cDero <> "" THEN
    RUN 2-prc-envoiemail("dero":u, cDero).

END.

So this it . You have all the info now
 

TomBascom

Curmudgeon
Actually, no, that isn't all of your code. It is just a somewhat larger snippet. But no matter -- I'm not going to go through your whole application fixing all of its problems in a discussion forum.

What I can tell you is that this code exhibits a lot of very poor record scoping right along with not much in the way of error detection and handling and very likely suffers from lots of hard to analyze side-effects. Handling transactions as we have been discussing, with named buffers strong-scoped to FOR blocks, will help.

You can also get control of a lot of globally scoped free references through a simple trick -- in every internal procedure or function definition add a named buffer for every table referenced using the plain table name:

define buffer dawflw for dawflw.

This will prevent references inside a user defined function or internal procedure from "borrowing" dawflw from the global context.

It may seem strange but is is essentially the same as:
Code:
define variable xyzzy as integer no-undo.

procedure test:
  define variable xyzzy as integer no-undo.  /* comment out this line to see side effects, keep it to avoid them... */
  xyzzy = 2.
end.

xyzzy = 1.
message "a" xyzzy.
pause.

run test.
message "b" xyzzy.
pause.

When you first start doing this you will discover just how much casual borrowing you are doing. You will have to re-find the record within the udf/ip which means that you will need to pass parameters sufficient to the task (rowid or WHERE criteria). It also means that when you find dawflw within an ip or udf that the global context will not be modified. If you are currently *depending* on that side effect then things will break and you will need to communicate the changes via output parameters or return values or some other means.

If you stop and think about it this "scope" stuff is really "Coding 101". We all should have learned better back in our mis-spent youth ;) Sadly there is a lot of code out there that wasn't written with such things in mind. But that doesn't mean that we should keep doing it that way.
 

TomBascom

Curmudgeon
To be clear -- in spite of my comments above I still have a lot of bad habits in my own code. I borrow free reference buffers far too often and abuse variables scoped to the procedure block with abandon. I tell myself that I have "reasons". But mostly I'm just being a lazy slob.

I do work very hard to do updates the way that I have described. That technique saves a *lot* of heartache.
 
I understand what you mean.

One question.
1542269666568.png
When you select a row on a browse and wanna do some update on it by clicking on a button, how can you pass parameter like ROWID in a input parameter ?
 
Top