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 go to 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 doing this is to put the data types in the prototype and the parameter names in the parameter list. Somewhere around Clarion 4 I believe this was changed so the prototype and the parameter list are the same (except the parameter list doesn't allow a return type, which is silly). So do it this way:
Prototype: (string var1, byte var2),stringParameters: (string var1, byte var2)!,string
Note the ! before the return type in the parameter list. That makes it compilable and it leaves behind a reminder of what this procedure returns. - Global data - can't believe I missed it 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).