On Mon, Nov 3, 2014 at 3:18 PM, Jef Driesen jef@libdivecomputer.org wrote:
And other then being a bit more verbose with one line extra, how's that different compared to yours:
One extra line? It's more than that. It's the difference between
if (something_failed) return -1;
with no messages at all, vs
if (something_failed) return error("Describe the failure"):
that looks fairly similar but is now _descriptive_ which is generally a bonus (maybe it even means that you can avoid a comment), and finally
if (something_failed) { ERROR("Describe the failure"); return -1; }
which I actually think is *worse* than even the first case, because now error handling actually ends up being bigger and more visually distracting than the actual real code.
See the difference?
So it's not just two lines more, and "more verbose". It actually ends up visually noticeably more distracting from the important part of the code itself, which is not the error handling, but the *normal* case code.
No, it's not a big deal in individual cases. If you have one of these in your project, who cares? Nobody. But you don't have one of them.
Look at that "receive_data()" function, for example. My "error_report()" function makes that small loop much more readable, exactly because the error handling doesn't end up blowing up all that code, and the whole loop easily fits in one screenful, even if you have a traditional 80x25 terminal.
THAT is important. Having error handling that doesn't detract (too much) from the actual real code.
If I'd have your "ERROR()" function that returns the completely useless SUCCESS enum, I'd literally have to remove all the messages entirely, and just go back to the simple "return -1" to get readable code. Or I'd have to make more than half of that simple loop be just error handling noise, and not fit on a screen any more.
As it is, now that that code "just works", removing the error messages entirely is probably fine. Of course, then if Suunto changes some protocol thing that I assumed, or there's some odd USB issue, you'd now be screwed.
So really. Look at that one "receive_data()" function. It's 30 lines total, of which two are that debugging comment for when you want to do per-packet dumps (yeah, hopefully never, but I ended up enabling that quite a lot while doing early development). And it shows exactly why I want that "report_error()" interface, because it has four very different error conditions in that inner loop. The actual "work" of the inner loop (ignoring errors) is already just seven lines (five lines of "work", and two lines for the exit condition).
Those *seven* lines are what is important. The rest is annoying error handling that right now is already 8 lines. But at least it is *only* eight lines. It's not sixteen. The error handling as it is is already bigger than the "real work", but hey, at least they are minimally bigger, and you can certainly argue that the error handling adds value.
Sixteen lines of error handling, for seven lines of actual working code? That's not a good tradeoff. I understand that some CS people think that "the more error handling, the better", but that's just insane.
Code is better if you can make the error handling *less* intrusive, not more. Yes, you want to handle error cases, but you want to handle them without swamping the real work and making it harder to read.
That one simple function also shows *why* that "-1" is special, in ways that an enum is not. Because a positive return value is actually a meaningful return value. It's not just 'success vs failure'. It's 'return data vs failure'. The enum simply cannot do that.
Side note: receive_data() is not the only example. Look at "send_receive()". Exactly the same issue. Do you want to fill your screen with real code, or with useless noise just because your code interfaces were bad? It has six different things it validates, and like receive_data(), it actually returns real information on success, so again an enum of failure reasons wouldn't work.
And yes, I realize that the whole "return negative on error" is somewhat idiomatic traditional C and UNIX interface. If you come from another background, it isn't *nearly* as natural. But there's a reason why idiomatic C and UNIX has been a very successful model. It's a *good* model. It's not just me, it's not just libdivecomputer, it's been a very successful model in a ton of big projects. There's a reason DOS and VMS died. They were horrible horrible models.
It is *not* a random accident that you have UNIX system calls return negative error codes, and have 0/positive be success. It's actually fundamentally good design. Those original UNIX people knew what they were doing.
(Of course, I actually think that the Linux model of not just returning "-1", but returning "-errno" is even more superior, because unlike "errno" it is also trivially thread safe and denser from an information standpoint. So a "negative enum for error cases" is actually a perfectly fine model, and imnsho actively superior. That freaks some people out just because they are used to the whole "-1 means error" model, though).
Linus