Don't make the mistake of thinking that all tips are good tips. Programming has a dark side: techniques that seem good at first but gradually and insidiously drain your time or hurt your company's infrastructure. These techniques lead you down the path to the dark side.

In the article "Pattern Recognition: Ease Modern RPG Programming" (February 2007, article ID 20815 at SystemiNetwork.com), I explained that patterns are used in programming. Not all patterns are good. Anti-patterns are patterns that initially seem effective, but over time you learn that they lead you into traps. For this article, I polled members of the System i community and asked for a list of the classic traps that they've experienced. I include their comments herein (some have been edited slightly for clarity and punctuation).

When I say "traps," I mean something that appears to work properly after it has been coded and tested. Code that doesn't compile or that fails immediately during testing might be considered a "mistake," but I do not consider it a "classic trap."

The Improbability Factor

How many times have you written code and known that there's a possibility of an error, but you didn't code for that error because you didn't think it would ever happen? Or perhaps you thought it was unlikely and would require too much time to prevent. This anti-pattern is called "the improbability factor."

Many times, code runs fine when data comes in as expected. What about the situation when it doesn't? You get called at 2:00 a.m., that's what! — Ray Marsh

You have to test properly, preferably with repeatable, documented test cases. — Simon Coulter

We have procedures for creating test libraries. They are often very time-consuming. Because we have a separate machine for development that is a copy of the production machine, and time is always short, we often skip making the test library. I find that this is a classic trap. It's like setting a time bomb. In the future, a case that is not in production at the moment may present itself, and the program will fail. —David Foxwell

The divide-by-zero error in a production program is a classic trap. Isn't it programming 101 to check the divisor to make sure that it's not zero before you divide? Unless you've hard-coded a constant, you can never be sure! — Paul E. Musselman

I see some programmers trying to convert old programs from RPG III to RPG IV, and many times they convert a MOVE or MOVEL to an EVAL incorrectly. Either they didn't check exactly what the code was doing, or they didn't check the size of variables. And they obviously didn't test it, because the program either bombs out with an error message or simply produces something other than the correct value. — Glenn Gundermann

Even if they're expecting 10 or 20 buckets in an array, some programmers size it for 99. Then they fail to check whether it ever exceeds 99. When it does, they get an array index error. Or, they size the index of an array with the same number of digits as the buckets in the array — like 99 buckets and a two-digit index. Now when the number hits 100, it doesn't fit in the index. You get a "target too small to hold result" error. — Jim Franz

One classic trap is not checking SQLSTT or SQLCOD after every SQL statement and therefore causing errors to go undetected, making it harder to determine the cause of an error in production. — Kerri Bednarchuk

RPG programmers almost never check for I/O errors on CHAIN and friends. With the slow acceptance of referential integrity, this is probably okay, but as soon as somebody sticks a constraint on a master file, boom! RI errors are reported as I/O errors, and programs fall over. — Buck Calabro

Even without constraints, there's a good chance of an error occurring on file operations. All you need is to try to read a locked record, and if you haven't coded error handling, your program crashes.

I think it's a classic mistake not to have generic error-handling routines for unexpected errors. — Glenn Gundermann

Indeed, instead of the program crashing, an error handler should take control, perhaps create a program dump and print the job log, and in any event, notify the IT staff. Why make the user try to explain what happened (which will, no doubt, frustrate that user) when the system can easily generate diagnostics for you at the time of the crash?

Copy-and-Paste Programming

The "copy-and-paste programming" anti-pattern occurs when you copy and paste code (perhaps with small changes) instead of writing a reusable routine in a service program. Many programmers feel that this copy-and-paste approach is a good idea because they can develop code quickly. They argue that it's faster to copy and paste than to design code for reuse. That might be true for the initial development but not for long-term maintenance. It's easy to change your business rules if only one copy of the code exists but difficult if the code is repeated many times.

Programmers should place reusable code in callable components instead of embedding the same code in many applications. I think we've become better at this than, say, 20 years ago, but in web applications I've seen quite a bit of JavaScript and CSS code embedded in HTML files instead of being placed in a common source directory and referenced from HTML files. — Nathan Andelin

I catch programmers hard-coding variable lengths instead of referencing another field. I find this in DSPF/PRTF as well as RPG programs. — Glenn Gundermann

Too often, programmers copy and paste code and then forget to make all the necessary changes in the pasted code. — Jim Suiter

Programming by Accident

"Programming by accident" is an anti-pattern that occurs when programmers neglect to think their code all the way through before they write it. Then, when testing, they find cases that don't work, and they go back and add more code to handle the situations that they didn't originally account for. This technique can result in convoluted code.

Design your program (on paper, with pseudocode, a flowchart, anything!) before coding it! — Simon Coulter

Programming by Permutation

One of my personal favorites, "programming by permutation" means that programmers write code that they don't quite understand. Then, when it doesn't work, they change the code here and there by trial and error until something works. This method usually results in a routine that works but that contains extraneous code that doesn't make much sense. The routine is often difficult to follow because the programmers didn't quite understand what they were trying to do.

Reinventing the Wheel

There's little point in rewriting a routine that's readily available. If the code already exists, call it; don't write a new routine.

Programmers should use suitable code libraries rather than always writing their own. — Simon Coulter

Some programmers are quick to jump to the decision to rewrite a program or module from scratch when all that is needed is a simple fix. If the original code is ugly, confusing, or simply outdated, these programmers choose to reinvent the wheel rather than make the simple fix. Their tendency to always rewrite often makes seeing even the simple fix more difficult. — David Wright

Reinventing the Square Wheel

Even worse than reinventing the wheel is when a programmer does a poor job of rewriting an existing routine. "Reinventing the square wheel" is the same the as the reinventing the wheel anti-pattern, except that the new routine does not work as well or offers fewer features than the original one — much like opting for a square wheel when a perfectly legitimate round one could be used.

In my opinion, moving applications to another platform or to a language that works on any platform is often merely a reinvention of the square wheel. You end up with a less efficient or less robust environment and often gain nothing. At best, you can do business the same as you did before. At worst, your business suffers when things don't work as well as they used to.

The most classic trap I've witnessed, and the one that ends up costing the most, is broadly scoped efforts to migrate applications off the System i or to rewrite everything to work under platform-agnostic environments. — Nathan Andelin

Another less broad but common example of reinventing the square wheel is the use of homegrown date routines to add a number of days, months, or years to a given date instead of using the date functionality built into RPG. Using homegrown routines requires more work and is usually less correct and offers fewer capabilities.

Caching Failure

When you set a variable based on a particular condition and then forget to reset that variable later, it's called a "caching failure." We've all run programs and had them detect an error in the text that we've typed. But don't you hate it when you fix the error and still can't proceed because the program doesn't turn off the error condition after you've fixed it? Often, you have to exit the program and start all over again.

Another instance of a caching failure occurs when data from a previous record gets carried over to new records that you write because you forgot to clear the fields.

Forgetting to clear, reset, or initialize fields in a program that updates and writes to the same file can cause some fields in newly created records to be incorrectly populated with data from the last record retrieved. — Jeremy Sellick

Premature Optimization

Far too often, I see RPG programmers write code that's hard to read or is coded in an obscure manner to make it perform better. In the majority of cases, such coding is a bad idea. Although your code may run faster, the gains in performance are outweighed by the cost of maintaining the code. Don't get me wrong — there will always be times when your code does not perform well enough, and you have to rethink parts of your code to improve performance. However, coding for performance before you encounter a performance problem is "premature optimization" and is an anti-pattern.

Don't focus on performance when writing code. Focus on correctness and maintainability. Analzye performance once the code works properly. — Simon Coulter

We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. (Knuth, Donald, "Structured Programming with go to Statements," ACM Computing Surveys 6, 1974, 4:268.)

Magic Numbers

How many times have you seen numbers hard-coded in programs without understanding why? Perhaps you see a date multiplied by 10,000.0001, and you wonder why someone would do that. Or you see a number of seconds being divided by 3,600. What does 3,600 mean? Or a FOR loop that loops from 1 to 52. What's the significance of 52?

Unexplained numeric literals in a program are called "magic numbers" and are an anti-pattern. Instead of coding a number such as 3,600 in your calculations, code a named constant named SECONDS_PER_HOUR, and then use the name SECONDS_PER_HOUR in your calculation to make it clear what the number stands for. Likewise, using a named constant called CARDS_PER_DECK is much clearer than coding 52.

Numbers such as 10,000.0001 (there are several variations) were historically used for flipping dates to put the year at the start of the date format or to flip it to put the year at the end. These numbers are magic numbers, and they're a bad idea. But that's not the only problem with flipping dates this way! It also performs poorly and relies on the numeric overflow rules of the RPG III MULT opcode. As you change your code to use EVAL or free-format expressions, these numbers stop working. This technique was never a good idea, but with today's date operations, it's downright silly.

Stop using the 10,000.0001 date multiplication. I did a performance test on this method in the '90s and found that it took 18,000 times longer to do the date multiplication than moving the date to a data structure to reformat it. — Steve Morrison

Complacency, Not Changing with the Times

Perhaps the most common problem in our community today is that many people have not kept up to date with current software engineering practices. Some programmers hang on to outdated techniques and are unwilling to update their skills or learn new techniques.

My biggest "classic trap" would probably be the tendency to continue to do things the way they've always been done. This covers a wide range, from a "rewrite" of an old RPG/400 program, which consists simply of replacing every RPG III source line with the equivalent RPG IV line (yeah, it works, but really, why bother?), to the tendency to use standard source "templates" as a basis for new programs and then never bother to update the templates to take advantage of newer, better techniques. I've seen lots of templates (which I have nothing against in principle) that use horrible, old, ugly code that would take an hour or two to update, but it's never done. So instead, you get ugly template source mixed with new fancy source. — Rory Hewitt

Unfortunately, what Rory describes is all too common. In fact, I see people writing new systems using a technique that will make their systems harder to maintain for no better reason than "that's what the example looked like." Ouch! The failure to update your examples or templates results in a proliferation of bad coding techniques. Instead of five or 10 routines that are out of date and need fixing, the problem can multiply into dozens or hundreds.

Even the IBM manuals, where many of us learn new techniques, contain a lot of outdated code that fails to take advantage of modern coding paradigms.

I don't expect shops to stop taking care of normal business and concentrate on nothing but modernizing old code. That's not efficient, at least not if the old code still serves its purpose! But when the time comes that you do have to update the outdated code, take the time to refactor it into something more modern.

Some programmers are hesitant to ever rewrite a program or module. If the original code is ugly, confusing, or simply outdated, these programmers often invest far more time attempting to retrofit a fix or enhancement to the existing program than it would take to start from scratch. Their tendency to never rewrite often makes seeing even the simple fix more difficult. — David Wright

Don't hesitate to throw out old, crappy code when making enhancements. Don't live with known code defects. Avoid software entropy. — Simon Coulter

I'm sure all will agree that this is a classic trap: forgetting to or not realizing that it is time to divide and conquer. Some processing tasks are much easier to accomplish, manage, and maintain in steps. — Mark Villa

Race Hazard

The "race hazard" anti-pattern comes up when you fail to see the consequence of executing your code with a particular sequence of events. On i5/OS, where database logic is extremely common, this anti-pattern often manifests itself in the form of incorrect lock handling.

Unfortunately, because reproducing the effects of many simultaneous users in a test environment is difficult, race hazards often remain hidden until a program makes it into production. One common example of this scenario is when you have a file in RPG open for update and use the READ or READE opcode without the (N) operation extender. In this case, RPG requires an exclusive lock on the record before reading it, and when it can retrieve the record, it holds that lock until you release it by reading another record, unlocking the current record, or updating the current record. Typically, this error occurs not in testing but rather in production, when two programs are contending for the same record.

Another common mistake occurs when locking records unnecessarily — for example, when loading a subfile and using READ when you should have used READ(n) when the file is opened for update. — Glenn Gundermann

I agree with Glenn. When you're filling a subfile, you're reading a whole group of records, and locking the records at that point provides no value. Worse, if another program is holding a record lock (even if it's a record that the user will never attempt to update), the program can freeze up while waiting for the record or even crash with an "unable to allocate record" error. Instead of locking, use READ(n) while filling the subfile. Then, read the record with a lock only when the user has specifically requested that the fields be updated.

Another similar mistake occurs when you try to use existing records to determine the highest value used. For example, an order number might just be an incrementing number in your shop. To get the next available number, you might use SETGT followed by READP to get the highest number and then try to add one to it. This technique won't work correctly if two people run the code at the same time, because they'll end up with the same order number!

Using SETGT and READP to determine the highest value for a partial key, where the file could be updated elsewhere. — Dave Smith

More Classic Traps

I've received several other good suggestions for classic traps, but I don't know which anti-pattern to classify them as. The following traps are not exact quotes from the original authors; I have paraphrased them for brevity.

The USER parameter of RTVJOBA or positions 254-263 of the RPG PSDS provide the user component of the job identifier, and not the current user profile of the job. Instead, use the CURUSER parameter of RTVJOBA or positions 358-367 of the PSDS. That way, your program works properly when run by server jobs, such as the i5/OS HTTP or database servers, as well as interactive and batch jobs. — Glenn Gundermann

When you modify code, don't forget to modify the comments related to the code. — Emily Smith

Avoid using characters such as $, @, #, and § in variable names, procedures, subroutines, fields, record formats, or file names. The use of these characters makes it difficult to reuse your code in other countries or languages, and even if you don't need to do that today, who knows what the future might hold? — Birgitta Hauser

Never create a service program with EXPORT(*ALL); it makes it difficult to maintain backward compatibility in your service program and therefore requires more time spent in impact analysis anytime you make a change. Instead, use binder language (which is simple and quick to learn) to describe the service program's interface and signature. — Barbara Morris

Similar to the last point, use either CONST or VALUE on your subprocedure parameters anywhere appropriate. Forcing values to be input-only means that you spend less time on impact analysis when your service program is changed.

Make sure that you understand the difference between optional parameters and omissible parameters in IBM APIs. Coding for the wrong circumstance might not cause an error immediately but will lead to problems down the road. — Carlos Balestrazzi

If your RPG routines allow optional parameters with options(*nopass) and/or omissible parameters with options(*omit), make sure you know when to check %PARMS and when to check %ADDR(PARM)=*NULL. If a given parameter is defined with both *nopass and *omit, make sure you check %PARMS before %ADDR(). — Kerri Bednarchuk

Don't use numbered indicators in RPG. Use a named indicator. They're easier to read. — Kerri Bednarchuk

Beware the Dark Side

All the classic traps in this article are ones that I see regularly, both from programmers in my shop and in questions posted to public forums on the Internet. Everyone has made these mistakes at one time or another. Be vigilant for these common traps, and you'll write classy code instead of falling into classic traps.

Scott Klement is the editor of the System iNetwork Programming Tips e-mail newsletter, a System iNEWS technical editor, and a Forum Pro on the System iNetwork. He is also the IT manager at Klement Sausage Co., Inc.