Process impossible options (defensive programming)

Share this topic:



Link to this posting

Postby Ursego » 21 Feb 2013, 11:52

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

When your code processes a number of pre-defined options (like statuses or activity codes) then don't forget: the list of possible options can grow in the future, and you (or other developers) can forget to process the newborn option(s) producing a hidden bug. We can prevent that by forcing the code to complain: "Developer, THINK how to process the new option!". Two simple things should be done for that:

1. In the choose case/switch construction, add new case for EACH option which currently exists in the system. If an option is NOT processed then create a case for it anyway and write a comment like "do nothing", "irrelevant" or "not applicable" to let developers know you don’t process them intentionally and not by mistake.

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

Shortly, see two the following fragments. In the 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 status is 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

C#:
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 status is listed explicitly in a specially created 'case'): ***

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
   // do nothing
case else
   // Error message or exception saying that the status is not processable
end choose

C#:
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 ("No case defined for customer status " + custStatus);
}

So, if a new status (for example, 'deceased') will be added to the business after many years then the code fragment itself will force the then developers to update the logic. And that will usually happen in a pretty early stage of development or unit test! But even if the exception will be thrown in the 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 process it. If the new status requires no special treatment, then simply add it to the "do nothing" case. 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 "fall" into case else/default. 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

C#:
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 is described here: http://forum.powerbuilder.us/viewtopic.php?f=2&t=1
   f_throw(PopulateError(0, "Unprocessable is_curr_entity " + nvl("'" + is_curr_entity + "'", "NULL") + "."))
end choose

C#:
Code: Select all
switch (_currEntity)
{
   case Entity.Car:
      this.Retrieve(_carId);
      break;
   case Entity.Bike:
      this.Retrieve(_bikeId);
      break;
   default:
      throw new Exception ("No case defined for entity " + _currEntity);
}

Don't use Boolean flags (including CHAR variables with possible values like 'Y' and 'N' or '1' and '0') if there can be more than 2 options. Let's say, you have one class for both cars and bikes (because these entities are processed is a very similar way). Sometimes you want to know in which of two the modes the object (the instance) is created - in the car mode or in the bike mode. So, you could think about a Boolean flag _isCarMode (C#)/ib_is_car_mode (PB) which will be initialized to true or false depending on the mode and used this way:

*** 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

C#:
Code: Select all
if (_isCarMode)
{
   this.Retrieve(_carId);
}
else // Bike Mode
{
   this.Retrieve(_bikeId);
}

The best solution in this situation is to create a variable of an enum type (in PB, which doesn't support enums until version 12, create 2 the already described constants n_entity.CAR and n_entity.BIKE) notwithstanding that there are only 2 possible options - see the previous "GOOD code". So, if one day you want to use the discussed class also for boats, simply add Boat to Entities enumeration (or create n_entity.BOAT constant), initialize _currEntity/is_curr_entity with it, run the application and... follow your own instructions, written months or years ago!

Sometimes it makes sense NOT to follow this advice - for example, if there are really many options but you process one or two of them.
User avatar
Ursego
Site Admin
 
Posts: 112
Joined: 19 Feb 2013, 20:33

Return to Coding Style

Who is online

Users browsing this forum: Bing [Bot] and 1 guest


Process impossible options (defensive programming)

Share this topic:


If you think that this site is not too bad, please LIKE it in Facebook. Thanks!





cron
free counters

eXTReMe Tracker