Is this way of exceptions handling good practice?

Question

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?

@Space_C0wb0y: Agreed. We should have an auto-migrate for codereview, ideally. — Lightness Races in Orbit, May 05 '11 at 12:26
@dantuch: Well you never `delete port` in the only scope that `port` exists. You have attempted to do it in the `catch` handlers (but not for the case where no exception was thrown; presumably that's done later in your code, which you didn't include in your thus-inaccurate example), but as far as I can tell that won't compile (or, you have another variable named `port` further up the scope hierarchy that you're accidentally `delete`ing instead, probably invoking UB). Summary: either a double-free for something else, or a compile error. — Lightness Races in Orbit, May 05 '11 at 12:36
@JohnSaunders Because it contains working code, for which he is asking for a **code review**. As such, it is too localized for SE, but a perfect fit for said other site. If it was reworked to ask a specific question rather than "What would you change in this code?" it could fit here. — Keith Pinson, Nov 27 '12 at 20:11
@Kazark: so, if he reduced the code to just a sample of the technique he's using, would you be ok with the question? I would. Granted, it might be better to break it down into smaller questions, but still, taking his code as a sample of a pattern, rather than as a block of code to review, I think it's ok (IMHO). — John Saunders, Nov 27 '12 at 21:12
@JohnSaunders Mmm, good point. Yes. At that point the question becomes a general question and not a specific request for a code review (though it might still be good to do some rephrasing in light of the trimmed-down code since his phrasing makes it look like a request for review). — Keith Pinson, Nov 28 '12 at 01:42

score 7 · Accepted Answer · edited May 23 '17 at 12:19

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.

RAII would take away all those `delete`s and `bad_alloc` but rest will remain the same? — dantuch, May 05 '11 at 12:29
so I can simply make a new exception class, that all those small exception classes will inherit from? I want to group exceptions that are similar to each other, caused by the same component — dantuch, May 05 '11 at 12:58
@dantuch: You can group exceptions by deriving from the same class. You exception hierarchy can have more than two levels. — Björn Pollex, May 05 '11 at 13:22
One problem. According to the standard std::Exception does not take any parameters in its constructor. So it is usually better to derive from one of the exceptions that have been derived from std::exception. Personally I always use std::runtime_error. This works exactly like you want std::exception to work above. — Martin York, May 05 '11 at 15:10

score 2 · Answer 2 · 2011-05-05T12:26:41.963

2

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.

edited May 05 '11 at 12:26

answered May 05 '11 at 12:21

Can you provide some (really) quick changes that can improve my code? Don't mind my quiestion if it would take some time. – dantuch May 05 '11 at 12:23
1

You should not just have one single exception class. You should however have one singe base-class for all exception classes. – Björn Pollex May 05 '11 at 12:23
@Space Matter of opinion. I have got on very well for many years with a single exception class. – May 05 '11 at 12:26
@Space_Cowb0y: I usually have zero/one exception class per library. Unless there is some very specific type of exception that can be caught and handled separately for correction purposes. So though the concept of building an exception hierarchy is nice in practice it is rare (and we have std::exception for the common base). – Martin York May 05 '11 at 15:13

score 0 · Answer 3 · answered May 05 '11 at 13:03

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;
}

Is this way of exceptions handling good practice?

3 Answers3