Code smell: it's that funky aroma that you notice when looking at code that tells you something's probably wrong somewhere.

Wikipedia has a list of common code smells; Jeff Atwood provides a more complete collection. And while some of these apply to Clarion programming, I think it's time we had our own list.

When I initially published this page I listed the following code smells:

• Short, cryptic labels. Man, this drives me crazy. I know some Clarion coders don't like to type, but we have code completion now. 
• Short, cryptic file names. DOS 8.3 is so last century.
• Huge chunks of code in embeds. The more lines of code, the bigger the reek. If you're trying to do something complex, you should probably be pulling it out into a source procedure or a class/set of classes.
• Dependence on black box products from unreliable vendors. Use of black boxes period is a cause for concern; it's always better if you can get source code.
• Use of implicit variables. I really wish these had never been put in the language. Being able to declare stuff inline is great; the problem is that spelling mistakes don't trigger compiler errors.
• Ignored compiler warnings. It's easy to let these pile up, but they're warning you of potential problems. Take heed.
• OMITs across embeds. This bit a lot of folks in the Legacy -> ABC migration. Also Clarion doesn't reliably generate routines in the same order, so depending on how you use OMITs you could remove code you actually want.
• Copy/paste code. Not only does this make maintenance more difficult, it suggests coder inexperience and/or laziness which is a strong indicator of other problems in the code base.
• Circular dependencies between DLLs. Because DLL unloading is indeterminate this can cause GPFs. But it also indicates that there's no good oversight of the application architecture.
• No tests. Okay, this an almost universal bad smell in Clarion code. How do you know the code works if you don't have tests? Probably by running the app and typing and clicking. Not automated, not easily repeatable. Not good.

Those are all still bad smells, although "Dependence on black box products from unreliable vendors" is a bit subjective and isn't so much a problem with code as it is with vendor choice.

I've sniffed a few more unpleasant odors lately, including:

• More than one procedure per module. Back in the day we did this to reduce generation time. Now it's just a pain, for several reasons:
  • When you get a compile error that can't be sourced back to the app, Clarion will take you to the source file. It can be a hassle finding out which procedure is involved. If you have one procedure per module it's dead easy - just use module view in the APP.
  • If you don't already use version control with Rick Martin's add-in you really should do so, and when you do that, one procedure per module makes a lot more sense. 
• Bad prototypes/parameter lists. The very old way of declaring procedures and functions is to put the data types in the prototype and the parameter names in the parameter list. For years now (since Clarion 4?) the standard has been for the prototype and the parameter list to be the same (except the parameter list cannot have a return type, which seems like a silly omission). Declare your procedures this way:
    Prototype: (string var1, byte var2),string
    Parameters: (string var1, byte var2)!,string
  Note the ! before the return type in the parameter list, to please the compiler. You can leave it off entirely but I like having it as a reminder of what the procedure returns. 
• Global data - I can't believe I missed this the first time around.Yes, sometimes you really do need it, but in general global data is trouble waiting to happen, especially unthreaded global data. When any code in your app can  modify data potentially used by any other code in your app, mayhem awaits.  
• Module data isn't as bad as global data, but it's not much better either. And it prevents you from using one procedure per module (see above).  
• Huge apps. Keep your apps small, and try to group procedures in apps by functionality or some other useful taxonomic principle. If you're getting to 100 procedures or generation/compile times are slowing down, split up the app. DLLs are your friend. 
• Short app and procedure names. You've already taken the advice to make your labels descriptive; do the same for your procedure and application names. 
• Long procedure names and labels that aren't helpful. Anybody can come up with a long label, but does the label mean something now and, more importantly, will it make sense to you months or years from now when you've forgotten what the code does? As is often said, naming is really hard. But even a little care now pay dividends in time.
• Using file aliases for anything other than browses (and maybe even then). Files are global structures (usually threaded, but still global). If you find yourself using aliases for anything other than a browse, chances are you have convoluted business logic that demands you look at more than one record from one file at a time, and it's also highly likely that logic is buried in an embed point. So now you're involving global data in your business logic and your business logic isn't encapsulated in one or more classes. This kind of code just gets uglier and messier with time. A better way is to read all of the data you need into memory (in that class or those classes), execute your business logic, and then write the data back out to the database at the end. And if you are just using aliases within browses, you should probably have a long hard think about whether self-referential files really are the best way to model your data. Sometimes that is the answer, but it can also add unnecessary complexity.