[owncloud-devel] Defensive programming

Bernhard Posselt dev at bernhard-posselt.com
Wed Feb 4 12:23:40 GMT 2015


Also try to stick with one return type or null. Some methods Ive seen
return booleans, objects and null which is just insane and would never
work with a statically typed language

On 02/04/2015 01:21 PM, Robin McCorkell wrote:
> 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
>>
>>
>
>
> _______________________________________________
> Devel mailing list
> Devel at owncloud.org
> http://mailman.owncloud.org/mailman/listinfo/devel



More information about the Devel mailing list