I come from a Java background, and one of the areas I find to be most troublesome when working with Progress is handling the scope of variables. Consider the following snippet:
IF FALSE THEN DO: DEFINE VARIABLE i AS INTEGER INITIAL 4.END.DISP i.
This is fairly bothersome to me. The variable defined in a block doesn't lose scope. What really gets me is that information contained in a block that will never fire is able to get out.
It makes sense to me that you want to limit the scope of variables to as little as necessary to get the job done. I fix a lot of bugs that are caused by using file-scoped variables as local variables, and by local variables that are not used properly. The majority of these bugs wouldn't exist if variables were declared where they are used and then disappear at the end of the scope where they are declared.
My question is: what techniques do you guys use to prevent "scope-bugs" from cropping up?
Edit: and a semi-related question, where can I go to report this particular issue? I don't know if I would call it a bug report or a feature request (as it may be intended behavior for it to be the way it is right now, although I personally consider it a harmful mechanic.)
Having a variable definition in an ID FALSE DO block is just plain nonsense and should never be done. Having a variable definition in an internal procesure or method is perfectly sensible and limits scope exactly as you want. Scope to the procedure or class when appropriate, but scope to the internal procedure or method when possible.
Consulting in Model-Based Development, Transformation, and Object-Oriented Best Practice http://www.cintegrity.com
Obviously it being in a block that will always be false is nonsense. But every IF block is expected to not be entered sometimes, correct? If that wasn't the case, it wouldn't be an IF block. In other words, while one that is explicitly false is rather silly, bear in mind it's merely a placeholder for the fact that every IF block has potential to evaluate to false.
I completely agree with your statement "scope to the internal procedure or method when possible" when juxtaposed to your previous statement, "Scope to the procedure or class when appropriate." However, there are times when even being scoped to the internal procedure or method is too much exposure. I want the variable to have only as much exposure as it needs. So if I have a method or internal procedure with an IF or FOR or other DO block, and I have a need to use a variable inside that block and only in that block (not before, not after) I should be able to define that variable in the block, and have it go away at the end of the block. As it stands, that variable can be used for the rest of the procedure.
The implications of this are serious. Consider something like this
/* sum the tax paid on orders for the last year, sorted by customer */
You may want to use "taxsum" in another loop later. You shouldn't have to pick a different variable name just because it's been used already: doing so is simply going to be awkward and clunky. A variable defined in a scope should be limited to that scope. In any event, the variable is accessible to everything after that for loop. What's worse is that it contains the value of the taxsum for the last customer in the loop! This is a frightening prospect, and a nightmare to debug when it happens.
No, the point is that blocks don't scope variables, so putting a definiton inside any block is meaningless ... the IF FALSE just makes it particularly nonsensical. Divide functional units areound internal procedures or methods and they do scope variables and buffers. Problem solved.
This is fairly bothersome to me. The variable defined in a block doesn'tlose scope. What really gets me is that information contained in a blockthat will never fire is able to get out.
This is fairly bothersome to me. The variable defined in a block doesn't
lose scope. What really gets me is that information contained in a block
that will never fire is able to get out.
ABL variable scoping is only at the procedure level - so that includes classes, methods, user-defined functions, and internal and external procedures. Blocks have other scoping properties, but variable scope is not one of them. This is something that just is; I have no idea how necessary you find it - and there have been times when I've wanted this too - but if it's a pressing need, then I'd suggest getting in touch with Product Management and logging an enhancement request.
It makes sense to me that you want to limit the scope of variables to aslittle as necessary to get the job done. I fix a lot of bugs that are causedby using file-scoped variables as local variables, and by local variablesthat are not used properly. The majority of these bugs wouldn't exist ifvariables were declared where they are used and then disappear at the end ofthe scope where they are declared.
It makes sense to me that you want to limit the scope of variables to as
little as necessary to get the job done. I fix a lot of bugs that are caused
by using file-scoped variables as local variables, and by local variables
that are not used properly. The majority of these bugs wouldn't exist if
variables were declared where they are used and then disappear at the end of
the scope where they are declared.
I believe there are some 3rd party products like ProLint that can help you. Coding standards, naming conventions and code reviews will also help with this. But a lot of the time this is result of lazy or slipshod programming (and I've bitten myself in this way many times . The COMPILE .. PREPROCESS option shows buffer and transaction (I think) scope; not sure whether it also shows variable scope, and whether it could be enhanced to do so.
But the ABL is an old language, and is - unlike many others - almost completely backwards compatible, and so features implemented 20-some years ago are still in effect now; this affects how sweeping changes can be, to a degree.
Progress just does not work that way. You can place DEFINE statements wherever you want, provided you place them before the first reference to it. Placing them inside conditional blocks is useless and misleading. Good practice is to place DEFINE statements at the top of the procedure / function / whatever.
I do have some issues though with the default scoping of record buffers. If you have an internal procedure that references a buffer, that buffer is then scoped to the whole procedure.
define input parameter piCustNum as integer no-undo.
find customer where customer.custnum = piCustNum no-lock no-error.
define buffer customer for customer.
Great questions. Progress programmers tend to be quit lazy reducing variable- and bufferscopes.That makes maintaining their code often to a nightmare.
I always scope variables that are used only in a part of a routine by an 'extract method refactoring pattern'. Scopes of the buffers are also alll on the routines.
It is also a good habit to define variables with 'no-undo' by default (see docs for the reasons).
First step (extract calcCustTax routine):
FUNCTION calcCustTax RETURNS LOGICAL PRIVATE (BUFFER customer FOR customer):
DEFINE VARIABLE taxsum AS DECIMAL NO-UNDO INITIAL 0.0. DEFINE VARIABLE lastYear AS DATE NO-UNDO .
DEFINE BUFFER order FOR order. /* not absolutely necessary because of the for - scope, but not bad too (imagine a leave is added in the loop */ DEFINE BUFFER order-line FOR order-line. /* not absolutely necessary because of the for - scope, but not bad too (imagine a leave is added in the loop */ DEFINE BUFFER tt-cust-tax FOR tt-cust-tax.
lastYear = getOneYearAgoToday(). /* assume such function exists */
FOR EACH order NO-LOCK WHERE order.cust-no EQ customer.cust-no AND order.order-date GT lastYear: FOR EACH order-line NO-LOCK WHERE order-line.order-no EQ order.order.no: taxsum = taxsum + ( order-line.price * customer.tax-rate ). END. /* order line */ END. /* order */
CREATE tt-cust-tax. /* our temp-table records */ ASSIGN tt-cust-tax.cust-no = customer.cust-no tt-cust-tax.taxsum = taxsum.END FUNCTION.
/* code herebelow should also be placed in a routine with the customer buffer locally scoped */
FOR EACH customer NO-LOCK WHERE customer.active: calcCustTax(BUFFER customer).END. /* customer */
Second step (remove temp taxsum):
DEFINE BUFFER order FOR order. DEFINE BUFFER order-line FOR order-line. DEFINE BUFFER tt-cust-tax FOR tt-cust-tax.
CREATE tt-cust-tax. /* our temp-table records */ ASSIGN tt-cust-tax.cust-no = customer.cust-no.
FOR EACH order NO-LOCK WHERE order.cust-no EQ customer.cust-no AND order.order-date GT lastYear: FOR EACH order-line NO-LOCK WHERE order-line.order-no EQ order.order.no: ASSIGN tt-cust-tax.taxsum = tt-cust-tax.taxsum + ( order-line.price * customer.tax-rate ). END. /* order line */ END. /* order */
/* code herebelow should also be placed in a routine for scopingreasons */
Note also that removing temp lastYear (replacing the temp with the functioncall in the where-clause) is not desired for performancereasons.
Houtzager ICT consultancy & development
Variables are scoped to their containing procedure. The scoping mechanism is to create internal procedures and functions, and for classes, methods.
I can look at most ABL code and tell I didn't write it, merely by noting the number / size of internal procedures / functions.
I actually was hoping to submit a request for it, but hours of searching psdn and the primary site has yeilded nothing.
However, the point you bring up about the impact such a feature would have on legacy code is significant. I'm not a big fan of it when a new release comes out (in any language) and code breaks. That alone makes me realize that I actually don't want it implemented, even though I really do want the feature.
I like the idea of refactoring out into a different procedure, function or method, but there are times where it seems silly to do so. You know, when it's small enough that it doesn't really warrent it's own method, but it still requires the use of a variable. So it's unfortunate that there isn't a better way to get around this, but I suppose it would also have the benefit of lending itself to code reuse, which is a good thing.
Thanks for your time in this response!
The scoping of buffers is a whole 'nother ball of wax, although it is certainly a point along the same lines and is also a fairly significant source of headache in maintaining our (particularly large) legacy codebase.
You state that "Good practice is to place DEFINE statements at the top..." This is actually contrary to my experience in other languages. To quote Joshua Bloch, "If a variable is declared before it is used, it's just clitter - one more thing to distract the reader who is trying to figure out what the program does." However, in light the circumstances, I have to admit that defining at the top would be the most appropriate course of action. The key, it seems, is to keep methods short to eliminate the possibility of variable corruption. This is something that is considered a best practice across most languages anyway.
If I may ask, what command did you use to acquire that "listing file"?
Thanks for your insight on buffers!
Take a look at COMPILE ... LISTING ... in the Help.
My variables in real code NO-UNDO due to local coding standards. The same is true with "no function calls in queries."
This is the route I've been taking with a lot of my code. It's usually not for the purposes of variable scoping (typically it's because there's a lot of copy/paste code that really should have been broken out into a separate routine to begin with) but it would serve the purpose.
There are times, however, where it would seem simply overkill to do this. In this example (which I admit is contrived) you essentially have a single line of code doing work. If you expand it out to the loops you still only have a handful. It seems to me it is a little... small to be broken out into a separate routine. But as I mentioned in other responses, it appears to be the best course of action here.
I was also unaware you could pass a buffer as a parameter; this is probably due to another local coding standard. I'll have to do some experimenting with that, as I could see it being quite useful. Right now our current method is to pass in ROWIDs and just do another find, which I must admit I'm not the biggest fan of this either, but I think it beats passing in the find criteria and doing another (potentially index-failing) search. I can only imagine that the ROWID searches are quite fast considering 1) They're already in the local cache being the record was just found recently and 2) The tight coupling with the database that ABL provides only further decreases this extra seek time.
Thanks for your step-by-step. I do have a follow-up question though: what is the purpose of defining a buffer with the same name as the default buffer? Are there potential pitfalls with this methodology?
Thanks! I'll look into that. Although there are plenty of things I can say I don't care for with ABL, one of the things I am fond of is the insight the compiler provides with the XREF utility. This human-readable Listing file also looks to be useful. Thanks again.
what is the purpose of defining a buffer with the same name as the default buffer?
Scoping to the (internal)procedure and not having to change code. This is a double-edged sword ...
Are there potential pitfalls with this methodology?
Mainly confusion about whether you're using a named buffer or not. And there's the real risk of breaking code that depends on bleeding scope, and because the name in the internal procedure is the same as the default, it's not immediately obvious what the scope is.
A function can be only one line of code. An IP too, for that matter. Likewise a method. It is just a question of slicing up the problem into meaningful units. If there is a variable or buffer scoping issue, then it is big enough to be an IP or method.
Some people like defining buffers with the same name as the table since the code then reads like it is working on the table but one actually has a scoped buffer. I dislike doing this since it makes it less obvious that one has a local buffer so I always use a different name. In fact, if I had multiple buffer definitions in multiple IPs, I would be inclined to name them differently so that the scope was obvious. Helps avoid surprises too.