Code review comment for lp:~dandrader/frame/smarter_backend

On 08/30/2012 11:01 AM, Daniel d'Andrada wrote:
>> * Any functions that may fail need to return a UFStatus and provide the return
>> result through a referenced parameter. All of the new functions here should do
>> this. I forgot about this when I approved the previous merge proposal as well.
>> We should go back and fix these up.
>
> Isn't that extra status variable redundant in those cases? What's the benefit?

Let's take a function like:

UFBackendTouch frame_backend_frame_get_touch_by_id(
     UFBackendFrame frame, UFTouchId id)

Without knowing exactly how this is implemented, there are two possible
recoverable error conditions:

1. The touch ID is invalid (UFStatusErrorInvalidTouch)
2. The allocation and initialization of UFBackendTouch could fail
(UFStatusErrorResources or UFStatusErrorGeneric)

The current implementation doesn't actually catch allocation and
initialization exceptions, but it could if we found that to be a problem.

The client could use this information to determine how to proceed. It's
almost impossible to recover from allocation errors, so the client could
abort. However, if the touch ID is invalid, it could stop processing
gestures for that frame and continue on. At the very least, it could log
a more meaningful error message, which has been helpful in debugging
issues in the past.

The other main reason is that it maintains the cohesiveness of the API.
The entire grail and frame API behaves like this. It makes sense to
continue using the same mechanism.

I'm not a huge fan of the API, tbh. I much prefer an exception based
approach. However, this is C, and there's only so much you can do with a
C API. There is one benefit to the API, however, and it becomes when you
actively check the return value.

UFBackendTouch be_touch;
assert(frame_backend_frame_get_touch_by_id(frame, id, &be_touch) ==
        UFStatusSuccess);

As opposed to:

UFBackendTouch be_touch;
assert((be_touch = frame_backend_frame_get_touch_by_id(frame, id)) !=
        nullptr);

My personal opinion is that the second approach is messy because you are
both assigning a value and using C's () operator to check the assigned
value is correct, all in one statement of code. You could split it up,
but then you're using more lines of code and it isn't any more
comprehensible versus the original approach, imo.