Defensive programming. Defuse time bombs!


Link to this posting

Postby Ursego » 21 Feb 2013, 11:52

Resilient code is code that is designed to last, and to be safely reused by others. Code that is not susceptible to being broken by unexpected use cases. Defensive programming is producing of resilient code that robustly and gracefully handles cases of unintended use, and is resilient to future changes.

Many developers stop work as soon as their code passes a few basic tests to confirm that it produces the "right result" in a given use case. They do not consider the other possible ways in which the code might be used in the future. How their code will respond to common change, such as a table column switched from required to nullable, or introduction of a new customer status? In the short-term, this approach is attractive; those developers get things done faster. However, if the code is designed to be used for more than just a few months, then it is very likely that such changes can and will occur, and the inevitable result is broken code or, even worse, code that silently starts to behave differently, or produce different results. When this happens, the data integrity is threatened, as is the validity of the reports on which critical business decisions are often based. At this point, years later (long after the original developer has forgotten what she/he was doing, or even left the company), begins the painstaking process of troubleshooting and fixing the problem.

Would it not be easier to prevent all this troubleshooting from happening? Would it not be better to spend a little more time and effort during original development, to save considerably more time on troubleshooting, bug fixing, retesting, and redeploying? After all, many of the problems that cause our code to break are very common; they repeat over and over again in different teams and on different projects. This is what defensive programming is all about: we learn what can go wrong with our code, and we proactively identify and eliminate potential vulnerabilities during development rather than after the event when the problems have already occurred.

Why am I writing about defensive programming in a site, dedicated to elegant code? Because code with ticking time bomb is not looking elegant to me. In most cases, code elegance has nothing to do with cosmetic look.

Validate methods' parameters

If there is a limitation on what is allowed to be passed to a method, throw an exception if an invalid value is supplied in run-time.

Classical examples of such constraints: not null, greater than zero, future date, expire date not later than effective date, range of values, list of possible values, pointer references an existing (instantiated) object.

A method should be as foolproof as is reasonably possible, so that, when used incorrectly, it doesn't keep that sad fact secret dark.

Don't worry that the parameters validation section makes the code longer (while we try to keep functions shorter): that section should be visually divided from the beginning of the "interesting part" by an empty line or comments, so it is not taken into account when the function length is measured.

Process impossible options

In code branching constructions, process all existing options. Throw an exception if an unexpected option is supplied in run-time.

We are talking about if-s, and about such constructions as switch, case and choose case (different keywords are used in different programming languages).

When you are processing a number of pre-defined options (like order statuses or vehicle types) then keep in mind, that the list of existing options can grow in the future to reflect changes in the business requirements. You (or other developers) can forget to process the newborn option(s) while they require a special treatment, or should be treated in the same way as one of the existing options. That can produce a hidden bug. We can prevent it by forcing the code to complain: "Developer, THINK how to process the new option - maybe, it should not be ignored!".

To process impossible options, two simple things should be done:

STEP 1: In the code branching construction, create a case for EACH option which currently exists in the system. If a few options are treated in a same way, simply process them ALL in one case. For all the options which are NOT processed, create a case anyway, and write a comment like "do nothing", "irrelevant" or "not applicable". That comment will let developers (and future yourself) know that you did not process them intentionally rather than forgot about them or didn't know they existed.

STEP 2: Add the default / else section. It doesn't have any business logic since it will never be executed if everything goes fine; its only purpose is to signal about an unexpected option by throwing an exception.

If you have an if construction, you can add additional else if sections, or change the whole construction to switch / case (i.e. switch to switch :lol: ).

For example, 4 customer statuses exist in the system: ACTIVE, PENDING, INACTIVE and DELETED. But the piece of code processes only 2 of them - ACTIVE and PENDING:

*** BAD code: ***

Code: Select all
switch (custStatus) {
   case CustStatus.ACTIVE:
      [code fragment processing active customers]
      break;
   case CustStatus.PENDING:
      [code fragment processing pending customers]
      break;
}

As you see, INACTIVE and DELETED statuses are not even mentioned. We can save the situation - now, INACTIVE and DELETED statuses are listed explicitly (even though not processed in the business logic):

*** GOOD code: ***

Code: Select all
switch (custStatus) {
   case CustStatus.ACTIVE:
      [code fragment processing active customers]
      break;
   case CustStatus.PENDING:
      [code fragment processing pending customers]
      break;
   case CustStatus.INACTIVE:
   case CustStatus.DELETED:
      // do nothing
      break;
   default:
      throw new Exception ("Invalid customer status " + custStatus + ".");
}

So, if a new status (for example, SUSPENDED) will be added to the business after many years, the code fragment itself will force the then developers to update the logic if needed. And that will usually happen in a pretty early stage of development or unit test! But even if the exception will be thrown in production - it's better than long living with a hidden bug (which can cause a lot of headache, and even be very expensive). Maybe, the new status must be treated in a special way, or in a same way as one of the existing statuses. Anyway, you have to decide (or discuss with business analysts) how to deal with it. If the new status requires no special treatment, then simply add it to the "do nothing" section. I remember getting a chain of such errors when I was building an application for a football association (six or seven - it was funny!) after adding a new code value - that allowed me to fix possible bugs even before unit test! The messages chain was similar to a penalty shootouts - and I won with no goal conceded!

Processing of impossible options bring two more important advantage (in addition to defensive programming):

1. Looking at a switch / case construction, developers see the whole picture (not only its fragment!), so they don't have to guess which options are processed in the else / default fragment. As people say, a half-truth is often a falsehood...

2. Global Text Search will find ALL the places in the application where the searched code is processed. If the code is processed in else/default sections, then it will not be found, so you are in trouble and must waste time to investigate, having good chance not to find EVERYTHING you want.

Since we cannot use else / default section for business code, that naturally brings us to the following idea:

Don't use Boolean flags for non-Boolean entities, even if only 2 options exist.

We are talking only about entities, for which the 3rd option is theoretically possible (like CustomerStatus) - NOT about states which are Boolean by nature, when no 3rd option could exist (like loanPaidOff).

For example, we have a Transport Details screen which can be open in 2 modes - to display data for cars or for motorcycles. That mode is defined by a Boolean instance variable isCarMode (false = motorcycle mode):

*** BAD code: ***

Code: Select all
if (isCarMode)
   ...do something...
else
   ...do something else...

The best solution in this situation is to create a set of constants (enum if your language allows) even though there are only 2 options. So, the mode switching variable will not be Boolean anymore:

*** GOOD code: ***

Code: Select all
switch (currEntity) {
   case TransportType.CAR:
      ...do something...
      break;
   case TransportType.MOTORCYCLE:
     ...do something else...
      break;
   default:
      throw new Exception ("Invalid entity " + currEntity + ".");
}

If, one day, you want to use that class also for boats, simply add the BOAT constant to TransportType, pass that constant to the fragment, run the application and... follow your own instructions, written years ago!

You will enjoy an additional advantage of using constants instead of Boolean - you will see what is the meaning of false: isCarMode tells you what true is, but gives no clue what false is - who knows, maybe it's airplane, train or Harry Potter broom?

Of course, processing of impossible options makes sense only if you have a small number of pre-defined options (like only a few statuses). The advice is not applicable if there are tens or hundreds of options, or their number is unknown. For example, if there are 200 insurance coverage types in the application and you are processing only one of them, nobody expects you to list 199 others in the else!

In SQL statements, make sure the correct number of records is affected.
User avatar
Ursego
Site Admin
 
Posts: 131
Joined: 19 Feb 2013, 20:33



IF you want to ((lose weight) OR (have unbelievable (brain function AND mental clarity))) THEN click:




cron
free counters

eXTReMe Tracker