views:

124

answers:

2

I have a setup that looks like this.

class Checker
{   // member data
    Results m_results; // see below
 public:
    bool Check();
 private:
    bool Check1();
    bool Check2();
    // .. so on
};

Checker is a class that performs lengthy check computations for engineering analysis. Each type of check has a resultant double that the checker stores. (see below)

bool Checker::Check()
{   // initilisations etc.
    Check1();
    Check2();
    // ... so on
}

A typical Check function would look like this:

bool Checker::Check1()
{   double result;
    // lots of code
    m_results.SetCheck1Result(result);
}

And the results class looks something like this:

class Results
{   double m_check1Result;
    double m_check2Result;
    // ...
public:
    void SetCheck1Result(double d);
    double GetOverallResult()
    { return max(m_check1Result, m_check2Result, ...); }
};

Note: all code is oversimplified.

The Checker and Result classes were initially written to perform all checks and return an overall double result. There is now a new requirement where I only need to know if any of the results exceeds 1. If it does, subsequent checks need not be carried out(it's an optimisation). To achieve this, I could either:

  • Modify every CheckN function to keep check for result and return. The parent Check function would keep checking m_results. OR
  • In the Results::SetCheckNResults(), throw an exception if the value exceeds 1 and catch it at the end of Checker::Check().

The first is tedious, error prone and sub-optimal because every CheckN function further branches out into sub-checks etc.

The second is non-intrusive and quick. One disadvantage is I can think of is that the Checker code may not necessarily be exception-safe(although there is no other exception being thrown anywhere else). Is there anything else that's obvious that I'm overlooking? What about the cost of throwing exceptions and stack unwinding?

Is there a better 3rd option?

+10  A: 

I don't think this is a good idea. Exceptions should be limited to, well, exceptional situations. Yours is a question of normal control flow.

It seems you could very well move all the redundant code dealing with the result out of the checks and into the calling function. The resulting code would be cleaner and probably much easier to understand than non-exceptional exceptions.
Change your CheckX() functions to return the double they produce and leave dealing with the result to the caller. The caller can more easily do this in a way that doesn't involve redundancy.
If you want to be really fancy, put those functions into an array of function pointers and iterate over that. Then the code for dealing with the results would all be in a loop. Something like:

bool Checker::Check()
{
  for( std::size_t id=0; idx<sizeof(check_tbl)/sizeof(check_tbl[0]); ++idx ) {
    double result = check_tbl[idx]();
    if( result > 1 ) 
      return false; // or whichever way your logic is (an enum might be better)
  }
  return true;
}

Edit: I had overlooked that you need to call any of N SetCheckResultX() functions, too, which would be impossible to incorporate into my sample code. So either you can shoehorn this into an array, too, (change them to SetCheckResult(std::size_t idx, double result)) or you would have to have two function pointers in each table entry:

struct check_tbl_entry {
  check_fnc_t checker;
  set_result_fnc_t setter;
};

check_tbl_entry check_tbl[] = { { &Checker::Check1, &Checker::SetCheck1Result }
                              , { &Checker::Check2, &Checker::SetCheck2Result }
                              // ...
                              };

bool Checker::Check()
{
  for( std::size_t id=0; idx<sizeof(check_tbl)/sizeof(check_tbl[0]); ++idx ) {
    double result = check_tbl[idx].checker();
    check_tbl[idx].setter(result);
    if( result > 1 ) 
      return false; // or whichever way your logic is (an enum might be better)
  }
  return true;
}

(And, no, I'm not going to attempt to write down the correct syntax for a member function pointer's type. I've always had to look this up and still never ot this right the first time... But I know it's doable.)

sbi
+1 almost the same words even, but you typed faster :-)
Péter Török
@Péter: I might just have started typing earlier. `:)`
sbi
Ah, so _this_ is the secret! :-)
Péter Török
Thanks for that. Redesigning will be quite expensive because what I showed above is just an abstraction for a whole set of different implementations of Checker.
ramaseshan
@ramaseshan: Re-factoring doesn't come for free, but in the long-run it's good nonetheless. (But I know what some companies' heads think about long-term solutions hindering short-term revenue...) Post seemingly unsurmountable problems here, if you want. Maybe someone else can find a solution where you see none.
sbi
+2  A: 

Exceptions are meant for cases that shouldn't happen during normal operation. They're hardly non-intrusive; their very nature involves unwinding the call stack, calling destructors all over the place, yanking the control to a whole other section of code, etc. That stuff can be expensive, depending on how much of it you end up doing.

Even if it were free, though, using exceptions as a normal flow control mechanism is a bad idea for one other, very big reason: exceptions aren't meant to be used that way, so people don't use them that way, so they'll be looking at your code and scratching their heads trying to figure out why you're throwing what looks to them like an error. Head-scratching usually means you're doing something more "clever" than you should be.

cHao
If you return normally, all the destruction etc. has to happen, too, and if that is expensive, it's unlikely exceptions will add considerable overhead. I agree, however, that exceptions shouldn't be used for normal control flow.
sbi