[SOLVED] BREAK keyword or BY phrase missing for aggregate ex

Posted by Joseph Kreifels on 14-Jun-2016 12:49

I seem to be having an issue. Did I no something wrong here? Bare in mind this is draft code. It will look bad.

for each ap-ledger where
           ap-ledger.company     = "ffi"       and
           ap-ledger.bank-acc    = "99"        and
           ap-ledger.trans-date  = 05/26/2016  and
           break by ap-ledger.vend-num:

   if ap-ledger.ap-amt-type <> "p" then next.

   find vendor where vendor.vend-number = ap-ledger.vend-number no-lock no-error.

   if not avail vendor then next LOOP.

   if vendor.vend-name <> "Jeffery Lewis" then next LOOP. /* */

   display ap-ledger with 2 columns 1 down.

    temp-amount = temp-amount + ap-ledger.amount.

     if ap-ledger.amount < 0 then asd-count = asd-count + 1.

  if last-of(ap-ledger.vend-num) then do:

     if asd-count > 0 then
       final-amount = tmp-amount.
     else final-amount = ap-ledger.amount.

     final-amount = 0.
     asd-count = 0.


All Replies

Posted by Mike Fechner on 14-Jun-2016 12:53

To me it looks like the END in this block here:

   temp-amount = temp-amount + ap-ledger.amount.

    if ap-ledger.amount < 0 then asd-count = asd-count + 1.


terminates the FOR EACH so that the IF LAST-OF (ap-ledget.vend-num) is outside of the FOR EACH with the break by.

Posted by fred.hill on 14-Jun-2016 12:58

And there appears to be an extra "and" just before the no-lock...

Posted by Joseph Kreifels on 14-Jun-2016 12:58

That was the problem. I've been rearranging and rewriting this code for about an hour now. So that must have been a left-behind stray "end" . I was going crazy because I could write a much smaller block of code using the same "break by" and it would work.


Posted by Joseph Kreifels on 14-Jun-2016 13:00

Thanks Fred. The reason there is an extra "and" is because I was editing the code after putting it here, and left that there. It's not in the code editor. As Mike pointed out, there was a premature "end"  in my code

Posted by Marco Mendoza on 14-Jun-2016 15:01

You must change your code, in some cases the last-of() could be skipped.

Example: What if the last-of(ap-ledger.vend-num)  register have not a "p" on the ap-amt-type? You are doing a "next" and the last-of will never be executed.

Usually I have 2 solutions to that issue, include all the conditionals in the "for each", or create a block and instead next, do a leave, example:



 if ap-ledger.ap-amt-type <> "p" then LEAVE inside1L.

 .... more code ....


if last-of(ap-ledger.vend-num) then do:

 ... more code ....

Posted by smat-consulting on 14-Jun-2016 15:53

Good observation, Marco!

I apologize, Joseph, for using your draft code to make the below points... coding-style is just a pet-peeve of mine... I do hope, though, that maybe the one or the other reader might pick up a point or too, or at least start thinking about it in a different way, as it would surely make their life easier... :)

When I was at University, many moons ago, they taught us the concept of a Dykstra Diagram - basically any block should have only one entrance and one exit. I adhere to that ideology all my career-life. And it has saved me from many bugs like the one Marco pointed out. These things are sooooo hard to find, as sometimes stuff works, and sometimes it doesn't...

So, my recommendation is to use IF THEN ELSE blocks, and always make things go through the whole block until the very end. If it is conditional, place it in a THEN or ELSE block...

Also, being a nitpicking virgo, I choose to always comment my END statements - that way I can easily see where one block ends and where the next. And, together with careful indentation, I easily see if there's an end too much or too little, and where. Again, saved me from many bugs. Or, the other way around, found many bugs in others code due to not adhering to these simple best practices...

I'd start the loop like this:

for each ap-ledger  no-lock

 where ap-ledger.company     = "ffi"      

 and   ap-ledger.bank-acc    = "99"        

 and   ap-ledger.trans-date  = 05/26/2016

 break by ap-ledger.vend-num


 <whatever code>

 if last-of(ap-ledger.vend-num)

  then do:  /* last-of vend-num */

   if asd-count > 0

    then final-amount = tmp-amount.

    else final-amount = ap-ledger.amount.

   final-amount = 0.

   asd-count = 0.

   end.     /* last-of vend-num */

 end.     /* for each ap-ledger  no-lock */

Placing WHERE, AND and OR underneath each other, it becomes clear what the conditions structure is, and it is easy to put another line in, or take one out. Placing the THEN into the same column as the ELSE makes it easier to associate the ones that belong together.

Posted by smat-consulting on 14-Jun-2016 15:56

And, of course, you realized already, that you set the final-amount to 0 before doing anything with it, in the LAST-OF() THEN block...

Posted by pkrikke on 15-Jun-2016 10:46

One other thing – I notice that you are using a “BREAK BY…” and later “LAST-OF()”.   No problem with that – those work very nicely.   However, you also have some “NEXT” statements in your code, and the combination of a LAST/LAST-OF/FIRST/FIRST-OF and NEXT statements can lead to subtle bugs.  Simply put, in this case if you “NEXT” a record that is also a LAST-OF(), the last-of() block will never execute in that case.  You may not care in this instance, but sooner or later you will.  Trust me.  J
In case that isn’t clear, here’s a simple example where customer “12345” will never have totals displayed, because you’ll have skipped the record that was LAST-OF() for that customer.
Customer   Invoice#   InvoiceType
12345      A111111    B
12345      A111112    C
12345      A111113    B
12345      A111114    B
12345      A111115    C
73541      A111116    C
73541      A111116    B
73541      A111116    B
FOR EACH foobar NO-LOCK BREAK BY foobar.Customer:
    IF foobar.InvoiceType = “C” THEN NEXT.    /*will skip invoice A111115 – which is last-of() for customer 12345*/
    /*do some processing*/
    IF LAST-OF(foobar.Customer) THEN DO:
        /*display totals or something*/

Posted by Brian K. Maher on 15-Jun-2016 10:52

Well, just to make it clear, that “subtle bug” is a logic flaw in the application developers code and not in the ABL itself.  If you NEXT a record and also need to check FIRST/LAST-OF then you need to be sure your code is logically correct.

Posted by Joseph Kreifels on 15-Jun-2016 13:35

I actually caught that while testing. Thank for suggesting it though. Shows you know your stuff!

I ended up moving  the "  <> "p"  " to my WHERE at the end. 

Posted by Joseph Kreifels on 15-Jun-2016 13:39

This was draft code. Which is why I never actually used "final-amt" I just opened the terminal and thrown in some code. I was trying to implement some ideas off the top of my head to solve a ongoing issue.

Needless to say, after a few more hours of writing and enhancing the code above, I have created a well done solution that worked in the said program I was fixing.

Posted by Marco Mendoza on 17-Jun-2016 13:56

the <> "p" and vendor ...

This thread is closed