Process impossible options (defensive programming)


Link to this posting

Postby Ursego » 21 Feb 2013, 11:52

Process in choose case/switch constructions all existing options. Throw an exception if an unexpected option is supplied to the construction in the run-time.

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

Two simple things should be done for that:

1. In the choose case/switch construction, create a case for EACH option which currently exists in the system. For all the options which are NOT processed by this piece of logic, create a case anyway, and write a comment like "do nothing", "irrelevant" or "not applicable" to let developers (and future yourself) know that you did not process them intentionally rather than forgot about them.

2. Add case else/default section that will signal about an unexpected option (throw an exception, display an error message or whatever).

In the following example, I will use 4 customer statuses: ACTIVE, PENDING, INACTIVE and DELETED, but only ACTIVE and PENDING customers are processed currently by the business logic:

*** BAD code (INACTIVE and DELETED statuses are not even mentioned): ***

PB:
Code: Select all
choose case ls_cust_status
case n_cust_status.ACTIVE
   [code fragment processing active customers]
case n_cust_status.PENDING
   [code fragment processing pending customers]
end choose

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

*** GOOD code (INACTIVE and DELETED statuses are listed explicitly even though not processed): ***

PB:
Code: Select all
choose case ls_cust_status
case n_cust_status.ACTIVE
   [code fragment processing active customers]
case n_cust_status.PENDING
   [code fragment processing pending customers]
case n_cust_status.INACTIVE, n_cust_status.DELETED
   // do nothing
case else
   f_throw(PopulateError(0, "Unprocessable customer status " + nvl("'" + ls_cust_status + "'", "NULL") + "."))
   // f_throw() is described here: http://forum.powerbuilder.us/viewtopic.php?f=2&t=1
   // nvl() is described here: http://forum.powerbuilder.us/viewtopic.php?f=4&t=5
end choose

Java:
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 ("Unprocessable customer status " + custStatus);
}

So, if a new status (for example, SUSPENDED) will be added to the business after many years, then 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 be potentially very expensive). Maybe, the new case must be treated in a special way, and 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" (case else/default) 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!

Never write business code in case else/default section. If a few options must be treated in a same way, physically list them ALL in one case. There are three reasons for that:

1. case else/default section can now be used for reporting an error as described above.

2. Global Text Search will find ALL places in the application where the searched option is processed. If the option is processed in a case else/default section, 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.

3. Looking at a choose case/switch 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...

We discussed choose case/switch-es, but the conception works also for if-s. Usually you are forced to replace the if construction with choose case/switch to prevent terrible heap of else if-s:

*** BAD code: ***

PB:
Code: Select all
if is_curr_entity = n_entity.CAR then
   this.Retrieve(il_car_id)
else
   this.Retrieve(il_bike_id)
end if

Java:
Code: Select all
if (currEntity == Entity.CAR)
   this.retrieve(carId);
else
   this.retrieve(bikeId);

*** GOOD code: ***

PB:
Code: Select all
choose case is_curr_entity
case n_entity.CAR
   this.Retrieve(il_car_id)
case n_entity.BIKE
   this.Retrieve(il_bike_id)
case else
   f_throw(PopulateError(0, "Unprocessable is_curr_entity " + nvl("'" + is_curr_entity + "'", "NULL") + "."))
end choose

Java:
Code: Select all
switch (currEntity) {
   case Entity.CAR:
      this.retrieve(carId);
      break;
   case Entity.BIKE:
      this.retrieve(bikeId);
      break;
   default:
      throw new Exception ("Unprocessable entity " + currEntity);
}

Don't use Boolean flags (including CHAR variables with possible values like 'Y' and 'N' or '1' and '0') - even if only 2 options exist:

*** BAD code: ***

PB:
Code: Select all
if ib_is_car_mode then
   this.Retrieve(il_car_id)
else
   this.Retrieve(il_bike_id)
end if

Java:
Code: Select all
if (_isCarMode)
   this.retrieve(_carId);
else
   this.retrieve(_bikeId);

The best solution in this situation is to create a set of constants (enum or whatever) even though there are only 2 possibilities - see the previous "GOOD code" which is pared with this "BAD code" too. So, if one day you want to use the class also for boats, simply add the BOAT constant, pass it to the fragment, run the application and... follow your own instructions, written many years ago! In some situations, 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 is true, but gives no clue what false is - who knows, maybe it's airplane or train?

Of course, the trick, described in this topic, makes sense only if you have a small number of pre-defined options (like only a few statuses). It's not applicable if there are tens or hundreds options, or their number is unknown - for example, they are taken from an application (not a catalog) table which can be changed by non-administrative users.

If you make processing impossible options your habit, you can add the following fragment to your resume (as I did):

Adept at defensive programming – writing code which automatically raises alarms in case of problems (to prevent bugs, which would be hard to detect and investigate, as early as possible), and so ensures foolproofing during the whole app life - from initial unit testing to future changes.
User avatar
Ursego
Site Admin
 
Posts: 120
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