Strategies for Successful Development

Process and technology for small teams building real software

Development “Red Lights”

Posted by S4SD (Seth Morris) on 2008/09/17

Development Red Lights are those things which should stop a programmer in his or her tracks for a second—and maybe a third—look. When you see one, pull another developer over and prove that it’s the right thing to do.

None are wrong and all have uses. They aren’t in the "never do this" category. They are tricks, odd work-around, obscure language and platform features, or even sometimes-correct decisions that won’t break anything, but have some undesirable implication. Many are nightmares waiting to happen to some poor maintenance engineer.

I encourage you to make your own list for your team. Post it. Carry a copy into code reviews and ask the developer to explain why anything on the list is still in the code. There are often good reasons, but at least put a comment on every one.

The list doesn’t need to be long. And ideally you’ll never see anyone try to use something on the list. But it’s well worth having it around.

Here are some of the items that start on my list. After these I add domain- and project-specific items. Include red lights specific to technologies you’re using and any specific to the project’s existing code. This isn’t the place for a list of code suggestions or gotchas; keep it to unreasonable-but-happens code and design that might be necessary sometimes.

If you’re a lead, it’s important that you create your own list and customizing it to your team. I’ve kept my samples short.

Design

  • Classes with "Controller," "Manager," or similar suffixes

These classes often indicate that procedural code has been shoehorned into an object design without any refactoring. They can be appropriate, but check each one and make sure it isn’t actually just wrapping procedural design.

If you want procedural code, go ahead and use it, but make it clear. Don’t try to disguise it as objects.

  • Loose coupling

If you have loosely coupled events (best identified as events where there is no visible relationship between the caller and the callee until run-time), stop and demonstrate that you will use the coupling in a way that makes sense. It’s easy to loosely couple events "on principle" and lose the ability to trace the logic forward. Loose coupling is often appropriate, but it comes at a high cost in maintainability as a system evolves. It is also easier to put loose coupling in than to take it out. Err on the side of the simplest and most easily corrected.

C++

  • Non-public Inheritance

Virtual inheritance is almost always undesirable. It either indicates an inadvertent diamond inheritance or a has-a relationship (composition) modeled as an is-a (inheritance). Consider composition or creating an interface.

Private inheritance means the class is inheriting implementation instead of interface. This is usually undesirable. Again, consider whether composition is a better alternative.

Protected inheritance is just plain wrong. It makes peoples’ heads hurt. Don’t do it.

  • Uncommon Operators

Avoid strange operators and casts, particularly the pointer-to-member operators, >* and .*. These raise the bar for maintenance considerably and can generally be designed-away.

Also be suspicious of overloading the function call operator, operator(). These often indicate gold-plating or overdesign.

In general, operator overloading (other than assignment and maybe comparison) should be limited to classes that behave like data types rather than functional classes. As a rubric, if you have operators as well as functional members (methods) re-evaluate the design and make sure you really mean it.

  • Implicit Casts

Be very careful creating implicit casts. Any c’tor with one parameter creates an implicit cast the compiler will call to coerce a type; this can lead to unexpected behavior that hides a code mistake. Treat c’tors with the same respect you would a cast and consider an explicit cast instead, both in the interface and in the usage.

If a class creates a implicit cast, consider using an OutputDebugString() in debug mode to track it. You can’t search for implicit casting, that’s why it’s implicit 🙂

Not every one-argument c’tor is a warning sign. Watch for conversions from a type that is conceptually similar to the class implementing the c’tor. You want to flag the Red Light when the c’tor may catch a developer using a different class in place of this one.

  • Empty statement blocks

Check that these aren’t a too-clever way to combine logic into a loop test.

Similarly, check that any artificial scope has a reason to exist and is clearly documented.

C#

I don’t yet have a good starter list for C#, VB.NET, or the .NET framework.

Any suggestions?

I have a few items I use for JavaScript code, but they’re pretty obscure. I don’t know if I’ll post them.


Listening to: The Bonhoffs – Manhattan Sleeps – Shiraz

 

Advertisements

One Response to “Development “Red Lights””

  1. […] augment coding standards with something less draconian: a “stop” list (https://s4sd.wordpress.com/2008/09/17/development-red-lights/). This is a list of statement, idioms, and techniques which require stopping, stepping back, and […]

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

 
%d bloggers like this: