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