2

I have some code to sort a vector of objects. If any of the objects is invalid I want to stop sorting immediately and report an error. In the error, I want to include the description of an invalid object (no matter which one if there are many).

This is my code (not complete, but I hope you can follow me):

int sortProc(const Bulk & b1, const Bulk & b2) {
    if (!b1.isValid()) throw b1;
    if (!b2.isValid()) throw b2;
    return b1.compareTo(b2);
}

vector<Bulk> * items = getItems();
try {
    sort(items->begin(), items->end(), sortProc);
} catch (const Bulk & item) {
    cout << "Cannot sort item: " << item.description();
}

Now, I'm a bit unsure of my code because I've heard that all exceptions should subclass the exception class and it's considered bad practice to throw objects that are not instances of exception, but I don't really understand why. My code above works, is anything wrong with it? This is a serious question, so if you see any problems I'd be glad to know. I'm not looking for moral concerns.

TyMarin
  • 63
  • 3
  • 4
    The problem is, how would *others* catch it? Especially if they link against the static library, and the `class Bulk` isn't defined in any header files. – Joker_vD Dec 09 '13 at 20:02
  • 1
    Consider that what you refer to as "moral concerns" exist for a reason. – John Dibling Dec 09 '13 at 20:04
  • 4
    Coding moral concerns are important. See and enjoy: http://thecodelesscode.com/case/1 – Timothy Shields Dec 09 '13 at 20:06
  • @JohnDibling: I'm not at all sure what either you or the OP mean by "moral concerns". – Keith Thompson Dec 09 '13 at 20:06
  • 1
    @KeithThompson: My assumption was that OP would disregard any reasons that were not hard technical requirements as being irrelevant "Holy War" type arguments. – John Dibling Dec 09 '13 at 20:09
  • @Keith: I'm pretty sure "moral concerns" means "do as you're told without knowing why", which IMO isn't good practice even where what you're told is correct :-) – Steve Jessop Dec 09 '13 at 20:09
  • Here is answer to your question: http://stackoverflow.com/a/1551014/1280316 – Chemik Dec 09 '13 at 20:09
  • 1
    Unfortunately there are plenty of "good practices" that are _incredibly_ hard to justify in purely practical terms. "But my code works!" is the worst thing to answer when it's true but misguided! – Lightness Races in Orbit Dec 09 '13 at 20:10
  • @JohnDibling well, not really disregard. But yes that's exactly my point with the technical requirements. – TyMarin Dec 09 '13 at 20:13
  • 1
    To add to Joker_vD's concerns: Not only can your exception type only be catched by someone who knows type `Bulk`, it is also rather useless if not catched directly at the call site of `sort`. No one but the caller can interpret the thrown `Bulk` as not being *sortable*, and at higher levels in the call stack you'd just see an exception that doesn't contain any explanation of what went wrong (no `.what()`), only *where* it went wrong. – dyp Dec 09 '13 at 20:14

6 Answers6

7

I'm not looking for moral concerns.

You can't ask a style question then ban all answers based on "moral concerns", if you expect to figure it out.

Some people think that throwing only objects of types deriving std::exception provides consistency of interface, since you can invoke .what() on all of them and catch them all together at the top level of your program. You can also guarantee that other translation units — those who have never heard of your class Bulk — will be able to catch the exception if they want to (if only as a std::exception).

  • Is your program wrong? No.

  • Does it work? Yes, of course it does.

But sometimes simply "working" is not considered enough and we like to be a little more tidy about things.

That's really it...

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • The only legit reason I can think of to throw non-`std::exception`-derived objects is to model tagged union type (a-la Haskell `Either`). But it's slow as hell, and if one wants algebraic datatypes, using the language with them would be a wiser idea. – Joker_vD Dec 09 '13 at 20:09
  • @Joker_vD I don't think even that is correct. Like I said in my "solution", anything that is not an exception is not an exception and thus shall not be thrown. This is more regarding the program logic. I am personally against using such "tricks" for almost any programming unless there's no way around. One example would be using an empty structure pointer at the end of a piece of declared memory in kernel. No way other than this trick can achieve this. – Xephon Dec 09 '13 at 20:17
3

Why is throwing a non exception considered bad design?

Because rarely do systems exist in a vacuum.

Exceptions serve a fundamental purpose: to generate a packet of information about an error or some other "exceptional" condition, and propagate that information across boundaries without regard to the locality of those boundaries.

Consider the last part carefully:

without regard to the locality of those boundaries.

You can think of an exceptional condition as one which must be attended to in some way. If there is a piece of code that can handle it, then that code should be given the opportunity. If no code exists which can attend to it, then the program must die.

In this context, the packet of information describing the exceptional condition must be free to flow through any part of the program -- even those that you did not personally write, or even thought might one day be written when your project was a mere glimmer in your eye. In order for this to work however, all exceptions must be written using a familiar protocol. That is to say, far-flung exception handlers must be able to understand at least the basic information contained in the packet. Everybody has to be speaking the same language.

The way this is generally accomplished in C++ is by deriving all exception objects from std::exception. This way, a handler in a far-flung part of the code -- perhaps even code you never even dreampt of writing -- can at the very least report on the exceptional occurrence before the program meets with it's demise. It might even be able to handle the situation and allow the program to live on, but this is often not possible.

John Dibling
  • 99,718
  • 31
  • 186
  • 324
1

It's not about moral. It's not about functionality. It's not about correctness either. It's about the logic behind your code.

Remember code reflects how you think. A clear minded developer would never throw an "exception" which is indeed not an exception cause that just confuses what the logic is.

Joker_vD is also right regarding the productivity of the code but I don't think you are there yet.

Xephon
  • 393
  • 1
  • 12
1

I see a (maybe serious) problem having Bulk transporting input and failure data. Having a BulkException derived form std::exception is way cleaner!

While having a state indicating a failure is fine, I think using the class to transport the failure is no good.

A BulkException could gather additional information (like a stack trace) which is useless for normal operation.

0

Here is code very similar to yours, except it doesn't have random-access flow control:

std::unordered_set<std::string> errors;
auto sortProc = [&errors](const Bulk & b1, const Bulk & b2)->int {
  if (!b1.isValid()) errors.insert(b1.description());
  if (!b2.isValid()) errors.insert(b2.description());
  if (!b1.isValid()||!b2.isValid())
    return b1.isValid()<b2.isValid();
  return b1.compareTo(b2);
}

vector<Bulk> * items = getItems();
sort(items->begin(), items->end(), sortProc);
if (!errors.empty()) {
  std::cout << "Unsortable items found while sorting:\n";
  for (auto const& e : errors) {
    std::cout << e << "\n";
  }
}

I prefer avoiding random access flow control except in exceptional circumstances. The sort function could continue oblivious to the failure of an element to be isValid, so it should by default rather than have interface decoupled exception based flow control.

While it is tempting to use exceptions as a way to return a "second return value", the result is that your code's interface becomes obscured and coupled with your implementation details of both your code, and the code that uses your code.

Depending on the program, truly exceptional circumstances might vary from "failure to allocate memory" through to "hard disk failure", where there is no way to recover or have your code act error-oblivious.

Note that you can even make the above code less coupled by noting that invalid elements are sorted first or last. Then after the sort, examine your list, and check if there are invalid elements that have been collected there, rather than relying on a log of cumulative errors.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • So by "random access flow control" you mean "early exit on failure"? – Steve Jessop Dec 09 '13 at 20:35
  • 1
    @SteveJessop nope, I mean "dressed up goto", where you go from one location in the code to another, without much of any indication in the structure of the intermediate code that this is going to happen. Early exit on failure is great, you should design your functions to have clear code paths that allow for that when it is worth it! Using exceptions for non-exceptional failure cases is...less good to me. If the `sort` is *expected* to fail rather often, you should not use exceptions. If it is not expected to fail often, then failing to early exit isn't a large cost now? – Yakk - Adam Nevraumont Dec 09 '13 at 20:42
  • 1
    I recognise that it's widely held, but I don't share the belief the the appropriateness of exceptions depends on the frequency of occurrence of the event (and that "exceptional" means something along the lines of "rare and bad"). Neither do I think that an exception is any more or less like a goto according to how rare the event is or how bad it is. It's a case that must be considered in analyzing the control flow of the code (or if it's not then don't write any code to handle it). In this example, if some of the items are "unsortable", then "sort" is impossible, and should not return. – Steve Jessop Dec 09 '13 at 20:45
  • 1
    ... what you've done is made "unsortable" items sortable after all, then reported them as "unsortable" in your output. That may or may not be any less of a muddle than catching an exception, but comparing your code with the questioner's I'm inclined to say yours is more of a muddle at the moment. That said, it is potentially more *useful* to report all errors rather than just the first encountered, so the muddle may be worth it for that reason. – Steve Jessop Dec 09 '13 at 20:49
  • @SteveJessop It isn't all that unsortable, because... I sorted it above. I'm ok with random-access gotos when the problem is truly exceptional. I'm against them when what you really want is a second return value (just return a `variant` or `optional`). I'm even more against them when your goal using them is to inject a second return value into an intermediate function you didn't write in order to communicate with your callback you passed into it and bypass its code flow. If there is a *strong* need for such a random access goto, fine: but the above example does not seem to be a strong need. – Yakk - Adam Nevraumont Dec 09 '13 at 20:54
  • it's unsortable because you printed out to the user that it's unsortable (the questioner's phrasing is essentially the same, `cannot sort`). Or your output is lies ;-) When people say that it's too complicated to reason about "gotos" in the case where an object isn't valid for comparison, but fine to reason about "gotos" in the case where a "truly exceptional" event occurs (say running out of memory, or in Java opening a file that doesn't exist), what that tells me is that 90% chance they aren't reasoning about the "truly exceptional" cases at all, they're ignoring them. – Steve Jessop Dec 09 '13 at 20:57
0

Good design is about quality. Is your code provably correct? Does it minimize dependencies on external objects? is it reusable? Is it understandable? Does it eliminate redundancies? Can you test it? Can other people read it and know what it does? That last question is really hard, because you are not other people - you're the author.

John Deters
  • 4,295
  • 25
  • 41