[owncloud-devel] Defensive programming

Robin McCorkell rmccorkell at karoshi.org.uk
Wed Feb 4 12:36:57 GMT 2015


I was mainly referring to the general situation of exceptions vs return
null, perhaps for getUser it does make sense to return null. But there are
plenty of other functions that return null instead of throwing
On 4 Feb 2015 12:30, "Vincent Petry" <pvince81 at owncloud.com> wrote:

> @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
> >
>
>
>
> _______________________________________________
> 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/278512bb/attachment.html>


More information about the Devel mailing list