Usefulness of nullable return types

[clragon]

First off, wonderful package, thank you for making it.

I know that this package tries to closely mirror the Android SDK API, and therefore follows its documentation.
This is useful because it means we do not need to write documentation again, and people already familiar with it can use it easily.

However, I have noticed that the Android SDK API tends to go a "non-disrupting" route of error-handling:
public abstract @Nullable DocumentFile createDirectory(@NonNull String displayName)
where the return type is described as:
@Nullable DocumentFile file : representing newly created directory, or null if failed.

This is odd to me for two reasons;

• Normally, all Java code is quick to throw Exceptions, especially given that Java has clear annotations of what exceptions can be thrown but no enforcement of null or not null.
• Secondly, this gives absolutely no indication of why something went wrong.

(I am however not talking about functions like findFile where null is returned if nothing is found. that seems natural to me.)

It also leads to an incredibly annoying coding style, where after every operation one has to check whether said operation succeeded explicitly, instead of simply assuming it succeeded.

final file = dir.createFile(bytes: someBytes, mimeType: someMime);
// have I actually created a file?
if (file == null) {
  // I failed to create the file. what now?
} else {
  // I have successfully created the file. onwards with my operations!
}

In my own code, I have written a short method to counteract this problem:

  Future<T> _throwOnNull<T>(FutureOr<T?> future, String message) async {
    T? result = await future;
    if (result == null) {
      throw SomeException(message);
    }
    return result;
  }

That I have simply used on all my usages of SAF:

await _throwOnNull(
  dir.createFile(
    mimeType: downloadMime,
    displayName: _downloadName(),
    bytes: downloadBytes,
  ),
  'Could not create SAF file!',
);

This solution is not nice, but it is, in my opinion, better than have to write copious amounts of if-statements. Frankly, I am unsure what I would even do inside such an if-statement. Probably throw an exception nonetheless.
I could of course also simply use ! to always assert that my operations succeed. But this would rob me of the information on why they failed, again.

I am not proposing the package should be changed to throw exceptions instead of returning null and I doubt that such a change would be implemented upstream in the Android SDK library, but I felt like talking about this because it just seems odd to me.

Perhaps I am just doing this the wrong way.

[alexrintt]

First off, wonderful package, thank you for making it.

Nah, you're welcome man, thanks for taking time to open this discussion.

I know that this package tries to closely mirror the Android SDK API, and therefore follows its documentation.

You're right, it was a unwise choise I did when I started this package implementation, I'll explain later on.

This is useful because it means we do not need to write documentation again, and people already familiar with it can use it easily.

I agree with you but, though it seems pretty comfortable at the beginning, things start to collapse when implementation starts to get complex.

Normally, all Java code is quick to throw Exceptions, especially given that Java has clear annotations of what exceptions can be thrown but no enforcement of null or not null.
Secondly, this gives absolutely no indication of why something went wrong.

In the case of this package, this is even more helpful, where we can catch the exception and see why does it failed, and sincerely, this is the best approach, but for some reason we do not have it "as default" inside the Android SDK.

It also leads to an incredibly annoying coding style, where after every operation one has to check whether said operation succeeded explicitly, instead of simply assuming it succeeded.

Comically I've write a question on SO about this annoying null fact when coding in Dart.

And though your solution seems to work, as you said, you have not access to the internal implementation to get the exception metadata.

This solution is not nice, but it is, in my opinion, better than have to write copious amounts of if-statements.

But I think even with exceptions being thrown, we have if + catch statements to handle the error. But this is the advantage here: we have information about the error to handle.

// So instead of:
final file = dir.createFile(...);
if (file != null) ...

// We have:
try {
  final file = dir.createFile();
} on SecurityException catch(e) {
  if (e.restriction == SecurityRestriction.operatingSystem) ...
  if (e.restriction == SecurityRestriction.noPermission) ...
} on IOException {
  ...
}

I could of course also simply use ! to always assert that my operations succeed. But this would rob me of the information on why they failed, again.

Yep. Just don't. We do not wanna NullPointerExceptions inside our Play Store console.

This should be used just when you know that the type can't be T? but for some reason the analyzer doesn't recognize.

I am not proposing the package should be changed to throw exceptions instead of returning null and I doubt that such a change would be implemented upstream in the Android SDK library, but I felt like talking about this because it just seems odd to me.

To be true, this package is already fated to remove the "Mirror APIs" style and starting throwing exceptions as described here github.com/alexrintt/shared-storage/issues/56.

For some reasons:

• Kotlin is not Dart and Dart is not Kotlin, as the package gets complex, the API doesn't follow the Android SDK anymore due to language constraints (from part of Kotlin and Dart, they are different languages after all!).
• null is not enough to indicate exceptions is most cases.

And from what you described seems we're going to the right solution, because these errors (null checking everywhere and missing exception info) would not exists if I did follow this the Flutter recommended article recommendations for creating plugins: https://medium.com/flutter/writing-a-good-flutter-plugin-1a561b986c9c.

Let me know if it helps and makes sense. You're welcome to add any missing information and propose other issues and solutions.

Thanks.

[clragon]

So the package is already going this way? thats brilliant, love to hear it.

The article also mentions how top level functions are undesirable.
Personally I really dislike using packages with the import ... as ... syntax,
because it means my imports have to be special in every file I import them.
So if we were to get static functions on namespace-like Objects that cant be instantiated as well, I would be very happy.

[alexrintt]

Yep! This is also being implemented, I've already removed the specific file import statements:

import 'package:shared_storage/saf.dart'; // no more
import 'package:shared_storage/media_store.dart'; // no more
import 'package:shared_storage/environment.dart'; // no more

import 'package:shared_storage/shared_storage.dart'; // single import for all above