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" <pvinc...@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@owncloud.org > http://mailman.owncloud.org/mailman/listinfo/devel > >
_______________________________________________ Devel mailing list Devel@owncloud.org http://mailman.owncloud.org/mailman/listinfo/devel
