CODEWITHSUNDEEP
c++exception
dantuch
dantuch

Reputation: 9293

Is this way of exceptions handling good practice?

Would you change anything in this code?

class LocalPort {
public:
    LocalPort(int portNumber) {
      innerPort = new ACMEPort(portNumber);
   }

    void Open() {
      try {
          innerPort->Open();
      } 
      catch (DeviceResponseException& e) {
          throw PortDeviceFailure(e);
      } 
      catch (ATM1212UnlockedException& e) {
         throw PortDeviceFailure(e);
      } 
      catch (GMXError& e) {
         throw PortDeviceFailure(e);
      }
   } 
private: 
    ACMEPort* innerPort;
};

/////////

try {
    LocalPort* port = new LocalPort(12);
    port->Open();
} 
catch (bad_alloc& e) {
    ReportError(e);
    logger.Log("Wyjątek alokacji pamięci", e);
    delete port;
}
catch (PortDeviceFailure& e) {
    ReportError(e);
    logger.Log(e.getMessage(), e);
    delete port;
}

What I tried to above do was to make code below look and act better.

try {
    ACMEPort* port = new ACMEPort(12);
    port->Open();
} 
catch (bad_alloc& e) {
    ReportPortError(e);
    logger.Log("Wyjątek alokacji pamięci", e);
    delete port;
}
catch (DeviceReponseException& e) {
    ReportPortError(e);
    logger.Log("Wyjątek odpowiedzi urządzenia", e);
    delete port;
} 
catch (ATM1212UnlockedException& e) {
    ReportPortError(e);
    logger.Log("Wyjątek odblokowania", e);
    delete port;
} 
catch (GMXError& e) {
    ReportPortError(e);
    logger.Log("Wyjątek odpowiedzi urządzenia");
    delete port;
}
catch (...) {
    ReportPortError(0);
    logger.Log("Wyjątek nieznanego pochodzenia");
    delete port;
}

Did I succeed? Is the first better than the second? What do you think?

Upvotes: 1

Views: 588

Answers (3)

Björn Pollex
Björn Pollex

Reputation: 76866

It seems like you have badly designed exception classes. Why don't you simply add a member-function getMessage to your exception-hierarchy (preferably in the base-class all your exceptions derive from - std::exception provides the method what that returns an error message). This way, you could simply catch all exceptions in one statement, and handle them all in the same way.

class DeviceResponseException : public std::runtime_error {
public:
    DeviceResponseException() : std::runtime_error("Some error message") {}
};

class ATM1212UnlockedException : public std::runtime_error {
public:
    ATM1212UnlockedException() : std::runtime_error("Some other error message") {}
};

Then in your code, you can do this:

try {
    std::auto_ptr<ACMEPort> port(new ACMEPort(12));
    port->Open();
}
catch( std::runtime_error & e) {
    ReportPortError(e);
    logger.Log(e.what(), e);
}

Since your exception-classes all derive from std::runtime_error, this catch-clause catches them all. This is OK, since the handling is the same for all cases. If you have specific exceptions that require special handling, you add additional catch-clauses.

Also, instead of calling delete port in the exception handler, you should use an std::auto_ptr or boost::scoped_ptr or something of the kind. Read about RAII.

Upvotes: 7

ralphtheninja
ralphtheninja

Reputation: 133128

I'd move the exception handlers into the class and let the class handle errors instead of forcing all callers to handle it. This way you can do clean up of the pointer (if you really have to have a pointer for some reason) by checking the result value of Open():

class LocalPort {
public:
    LocalPort(int portNumber) {
      innerPort = new ACMEPort(portNumber);
   }

    bool Open() {
      try {
          innerPort->Open();
          return true;
      } 
      catch (DeviceResponseException& e) {
          throw PortDeviceFailure(e);
      } 
      catch (ATM1212UnlockedException& e) {
         throw PortDeviceFailure(e);
      } 
      catch (GMXError& e) {
         throw PortDeviceFailure(e);
      }
      catch (bad_alloc& e) {
         ReportError(e);
         logger.Log("Wyjątek alokacji pamięci", e);
      }
      catch (PortDeviceFailure& e) {
         ReportError(e);
         logger.Log(e.getMessage(), e);
      }
      return false;
   } 
private: 
    ACMEPort* innerPort;
};

LocalPort* port = new LocalPort(12);
if (!port->Open()) {
  delete port;
}

Upvotes: 0

user2100815
user2100815

Reputation:

Yes, I would:

  • have one single exception class and thus:
  • not translate exceptions from one type to another
  • catch the exceptions much higher up in the code and thus:
  • not end up writing more exception handling code than real code
  • and I would not create objects using new, and if I did I'd use smart pointers

Basically, I think you have completely misunderstood how exceptions should be used.

The best single change you could make to your code is to replace:

ACMEPort* port = new ACMEPort(12);
port->Open();

with:

ACMEPort port(12);
port.Open();

and similarly elsewhere - the need for catching exceptions then goes away.

Upvotes: 2

Related Questions