anarchuser / mic_stream

Flutter plugin to get an infinite audio stream from the microphone
https://pub.dev/packages/mic_stream
GNU General Public License v3.0
100 stars 68 forks source link

Suggestion: default values #54

Closed ab36245 closed 1 year ago

ab36245 commented 1 year ago

I have a function which invokes MicStream.microphone. I want to be able to pass my function optional values for sample rate, audio format and so on and pass them on to microphone.

That works fine but because of the way the default values are specified in the microphone call itself it is difficult to provide a way for my function to default to simply use the microphone defaults.

For example:

Future<Stream<Uint8List>?> myMicFunc({
  int sampleRate = ???,
  ...
}) async {
  ...
  final stream = await MicStream.microphone(sampleRate: sampleRate);
  ...
}

The ??? in the code above is the problem. As it stands I can't simply use MicStream's default value (_DEFAULT_SAMPLE_RATE) because it is private. I could copy the value (16000) but that seems tacky and error prone.

One solution would be to make _DEFAULT_SAMPLE_RATE publicly accessible so my code would become:

Future<Stream<Uint8List>?> myMicFunc({
  int sampleRate = MicStream.DEFAULT_SAMPLE_RATE,
  ...
}) async {
  ...
  final stream = await MicStream.microphone(sampleRate: sampleRate);
  ...
}

However that's also pretty fiddly.

I think a better solution would be to make the optional parameters to microphone nullable. For example:

static Future<Stream<Uint8List>?> microphone({
  ...
  int? sampleRate,
  ...
}) async {
  sampleRate ??= _DEFAULT_SAMPLE_RATE;
  // code from here as before
  if (sampleRate < _MIN_SAMPLE_RATE || sampleRate > _MAX_SAMPLE_RATE) {
    throw (RangeError.range(sampleRate, _MIN_SAMPLE_RATE, _MAX_SAMPLE_RATE));
   ...

(I'm only showing sampleRate here but the principle applies to all defaulting arguments.)

This would mean the calling function could be simply defined:

Future<Stream<Uint8List>?> myMicFunc({
  int? sampleRate,
  ...
}) async {
  ...
  final stream = await MicStream.microphone(sampleRate: sampleRate);
  ...
}

I have found in general that using nullable values for default parameters and then applying the default where it is needed is better than using default values in the parameter lists of dart functions.

Is this a change you would consider?

anarchuser commented 1 year ago

Thank you for elaborating on this. I understand your issue. I need to think about this for some time, though.

What I can say upfront: I don't mind making the default values publicly accessible. They are currently hidden to reduce noise; i.e., as long as no one needs them, they don't need to be accessible. The deeper issue here is that my default values, as sensible as they may or may not be, are only there for convenience and ease of getting started. They are by no means a tried-and-tested "works for all" configuration. As such, I believe the cleanest way would be for you to actually define your own defaults. This makes your code independent, more self-contained (whoever reads your code can easily look your own defaults up), and you can always easily adapt the defaults to whatever suits your needs.

Changing the function definition to allow for nullable values would circumvent the whole issue, yes. Dart tries to get rid of exactly the problem of nullable function parameters, specifically by allowing named default parameters. Subverting this would reintroduce all the issues of using nullable values for optional function parameters. I recommend having a look at the whole debate around nullability; you find plenty of resources online.

I am going to have a closer look at possible other solutions for this pattern. And if you want I can make my defaults public if you wish to rely on me always having sensible values, though I recommend you to define your own ones. Furthermore, I will not make the function parameters nullable.

ab36245 commented 1 year ago

Thanks for the feedback.

I understand that you consider the current defaults as perhaps arbitrary at best and so I should consider defining my own. That's totally fine!

Also, exposing the default values used by MicStream is good, even if you don't fully "endorse" them! :-)

I guess we'll have to agree to disagree on null parameters. (What follows may look like a rant and/or imply criticism. It's neither, just an observation about what I consider to be valid use of an otherwise rightly frowned upon feature to provide a robust and scalable pattern for default values.)

I am completely familiar with Dart's move to sound null-safety and I totally endorse it. My code is much cleaner and more robust because of it. In general, we are much better off without nullability!

However, I think that nullable parameters are one very valid exception to this rule and do not in anyway undermine a push to remove nullablility in general. They allow implementation of a pattern that is otherwise very difficult or, in the current case, impossible to implement.

In general if function a() is a low-level function which can have the best knowledge of what default values should be and function b() is a wrapper around it, it makes sense to allow b to provide a mechanism to its caller to allow passing a specific value or to let the low-level a decide the defaults. Having b replicate the defaults of a is tedious at best (your reservations about a actually knowing the best defaults aside!). It gets worse if we want a higher-level c() which calls b.

Allowing nulls to specify to the low-level function to use its best guess at a default solves this very cleanly. It does not encourage the use of null values "in the wild", only across calling interfaces. Further Dart's ??= operator provides a very clean way for the low-level function to apply defaults as part of the argument sanity checking process.

An additional benefit is that this scales seamlessly for non-constant defaults, which Dart's default value mechanism does not. Sticking with the admittedly arbitrary example of sampleRate, suppose in future you wanted to default to one value for 8 bit encoding and a different value for 16 bit, or to different defaults for android vs ios. This is not do-able via Dart's const default mechanism. This is a pattern you see in the flutter libraries themselves. For example, AnimationController.repeat looks like this:

  TickerFuture repeat({ double? min, double? max, bool reverse = false, Duration? period }) {
    min ??= lowerBound;
    max ??= upperBound;
    period ??= duration;
    assert(() {
      if (period == null) {
        throw FlutterError(
          'AnimationController.repeat() called without an explicit period and with no default Duration.\n'
          'Either the "period" argument to the repeat() method should be provided, or the '
          '"duration" property should be set, either in the constructor or later, before '
          'calling the repeat() function.',
        );
      }
      return true;
    }());
    assert(max >= min);
    assert(max <= upperBound && min >= lowerBound);
    assert(reverse != null);
    stop();
    return _startSimulation(_RepeatingSimulation(_value, min, max, reverse, period!, _directionSetter));
  }

In my opinion using nullable values this way does not in any way suggest that this code is not null sound. It is simply making use of one of the very few but totally valid uses of null left!

Ok, that's my $0.02 worth!

And thanks very much for the package by the way - it's a very nice simple way to stream microphone input cross-platform and I am grateful to have it!

anarchuser commented 1 year ago

Thank you again for your detailed information. I did some research and - reluctantly - have to agree, there does not seem to be any way to have a wrapper switch arbitrarily between default and other values (besides hard-coding every possible case, ugh).

Seeing the Flutter team doing the same for the AnimationController [src, for reference], I agree that it's okay to use nullable values for this specific type of scenarios. Here's my suggestion: Make a Pull Request with how exactly you would like the interface to look, including making the defaults public, then I look over it and finally merge it into a new version.

anarchuser commented 1 year ago

published as 0.6.4 now