[owncloud-devel] Defensive programming

Robin McCorkell rmccorkell at karoshi.org.uk
Wed Feb 4 12:21:07 GMT 2015


Hi,

I feel as though we are in limbo with regards to exceptions. Some code uses
exceptions to report an error, while others return null or false. I suspect
this is one of the reasons why these situations have occurred.

It is my opinion that a function should only return values that make sense.
If a function is named getUser, it should only return the user, and if it
can't that is clearly an error situation where an exception should be
thrown. On the other hand, a function like getListOfThings could viably
return false if there is nothing to get (unless those things are expected
to be there!).

A change to a fully exception based system won't happen overnight, but it'd
be nice to see it promoted more in the code base.

Just my thoughts,
Robin McCorkell
On 4 Feb 2015 12:14, "Vincent Petry" <pvince81 at owncloud.com> wrote:

> Hello,
>
> After debugging several bugs, I recently found that many pieces of the
> core code is assuming that a function call will always return the
> expected value.
>
> There were many scenarios where a given bug was causing \OCP::getUser()
> to return null but the code would carry on working with that null value,
> and cause side-effects:
>
> https://github.com/owncloud/core/blob/stable7/apps/files_encryption/hooks/hooks.php#L438
>
> I suggest that from now on we try and code more defensively.
> This means that we want to check for correct values more often.
> For example when the current user is needed, check for "null" afterwards
> and throw an exception / log a warning instead of letting the code
> continue.
>
> Here is another example: https://github.com/owncloud/core/issues/13862
> And here when `getRelativePath()` fails, it is not a normal situation,
> we need to throw an exception instead of continuing:
>
> https://github.com/owncloud/core/blob/master/lib/private/files/view.php#L792
>
> What do you think ?
>
> I have raised https://github.com/owncloud/core/issues/13885 with a list
> of cases that can be fixed. If you know of other places in the code that
> can be improved that way, please add them in the checklist (and/or
> submit a PR to fix them)
>
> Thanks,
>
> Vincent
>
>
> _______________________________________________
> Devel mailing list
> Devel at owncloud.org
> http://mailman.owncloud.org/mailman/listinfo/devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.owncloud.org/pipermail/devel/attachments/20150204/af342d7a/attachment.html>


More information about the Devel mailing list