Welcome to Software Development on Codidact!
Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.
Multiple catches with almost the same code.
I find myself often writing code like this.
try {
// code
}
catch( FailedReadException const & ex) {
file.close();
std::cerr << "Read failure " << ex.readFailure() << std::endl;
std::lock_guard<std::mutex> lock(mutexFile);
errorState = true;
}
catch( std::runtime_error const & ex) {
file.close();
std::cerr << "Read failure " << ex.what() << std::endl;
std::lock_guard<std::mutex> lock(mutexFile);
errorState = true;
}
catch(...) {
file.close();
std::cerr << "Read failure " << "unexpected error" << std::endl;
std::lock_guard<std::mutex> lock(mutexFile);
errorState = true;
}
I'd like to make it easier to read by factoring all that common code. I could use a function for that. But that moves the code away from where it is used. I'd rather have it there in the catch for easier reading. Best I've come up with is this:
try {
// code
}
#define CATCH(reason) {\
file.close(); \
std::cerr << "Read failure " << reason << std::endl; \
std::lock_guard<std::mutex> lock(mutexFile); \
errorState = true; \
}
catch( FailedReadException const & ex)
CATCH(ex.readFailure())
catch( std::runtime_error const & ex)
CATCH(ex.what())
catch(...)
CATCH("unexpected error")
#undef CATCH
Is there a better way to do this?
2 answers
One solution would be to catch a more generic base class, like std::exception. If all your exceptions derive from that, you should be safe.
Another solution I like better is using destructors to do clean-up instead of lots of try-catches. The more clean up is done automatically by destructors, the better. I recommend you watch this CppCon talk titled Declarative Control Flow. The video shows a very convenient way of using destructors with macros like SCOPE_EXIT, SCOPE_FAIL and SCOPE_SUCCESS from folly/ScopeGuard.h. You can basically write declarative code like:
file.open();
SCOPE_EXIT{ file.close(); };
SCOPE_FAIL{ errorState = true; };
The macros create an anonymous object, which execute the block in its destructor either always or conditionally depending on the macro. This is the most natural way I tend to do error-handling. I usually don't care about the particular exception being thrown. All I want to do is to make sure code either succeeds or fails gracefully and cleans up automatically.
I'd argue that the destructor of file should handle closing, but in case you're working with some legacy API, this could be useful.
0 comment threads
There are many things we label as bad when programming, for various reasons. Repeating code is slightly bad. But writing function like-macros is extremely bad.
You should not try to replace something slightly bad with something extremely bad. As it turns out, the programmer putting out fire with gasoline in their attempts to avoid code repetition is a very common pitfall when we look at broken C++ program designs. Don't go out of your ways to avoid code repetition! Less "DRY" and more "KISS", for those who like design buzzwords.
The reasons why we should avoid function-like macros are many: they are type unsafe, it is easy to write subtle precedence bugs when writing them, they are hard to read, they are very hard to debug and maintain.
But mostly we should avoid them because individuals inventing their own personal, secret macro language makes the code impossible to read. Other C++ programmers know C++. They don't know your secret macro language. Therefore you should strive to write the code in C++ instead.
In your specific case, you could simply have written a function instead. Perhaps something along the lines of:
bool exception_failed (const std::string& str);
This could likely be a private member function too. If the reason why you can't do this is because you've got lots of variables scattered outside a class, then probably you should have created a class.
Cleaning up file handles and mutexes is ideal for a little local class that you can create an object of at local scope, to get all the clean-up handled by RAII. This might reduce the need for exceptions too.
Regarding exceptions, my experience is that it is always best to stick to the standard ones unless you have very specialized needs, which is rarely the case. Most often you just wish to send a string around. The best option is to rewrite the API can't be rewritten to use std::runtime_error. If that's not possible, then it might be justified to use a wrapper to standardize the exception types.
If you have some function foo throwing rotten_tomatoes, then you could create a wrapper just to get rid of the custom, cumbersome type:
void foo_wrapper (int x)
{
try
{
foo();
}
catch(const rotten_tomatoes& ex)
{
throw( std::runtime_error(ex.msg()) );
}
}
Or better yet, don't throw an exception when there's no need for it and return an error class or enum. I started this post speaking of bad things we do when programming, and exception handling is definitely one of them. They should be used with caution and only for the most severe forms of errors, if at all.
1 comment thread