[owncloud-devel] Defensive programming

Vincent Petry pvince81 at owncloud.com
Wed Feb 4 12:29:49 GMT 2015


@RobinX, having getUser() return null is expected/legal when there is no
logged in user (ex: public page)

However in that specific situation, there's a bug causing the user to
not be defined but no exception is thrown.

So having a check there would be nice.

Cheers,

Vincent

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.owncloud.org/pipermail/devel/attachments/20150204/125c05d2/attachment.sig>


More information about the Devel mailing list