Do not use optional arguments on public APIs

[paulomorgado]

As part of the discussion around adding async document/element loading for XLINQ (#110) it was mentioned using optional arguments (commonly called optional parameters).

PLEASE, DON'T USE IT ON PUBLIC APIS!!!

Before there were optional arguments the problem of a large number of possible parameters for an API already existed. And since we are talking about XML, look at how XmlReader and XmlWritter solved it:

• public static XmlReader Create(TextReader input, XmlReaderSettings settings)
• public static XmlWriter Create(XmlWriter output, XmlWriterSettings settings)

Taking advantage of object initialization, calling it looks like optional arguments:

XmlReader.Create(reader, new XmlReaderSettings{ IgnoreWhitespace = true });

(I'm going to propose more type inference for cases like this: Do not require type specification for constructors when the type is known dotnet/runtime#13855)

It's extensible. Another setting can always be added with proper defaults and without breaking existing code.

And those settings also become something that can be passed around.

Imagine that the original API used optional arguments and there was only one Save method. How would you keep that and now add an CancellationToken parameter?

[AdamSpeight2008]

Parameters are what the method specified as being required.
Arguments are what you supply to method, to fulfil those requirements..

I have no problem with optional parameter on Public APIs, since VB.net has them for like forever :)

That section of XmlReader.Create could be rewritten using Optional Parameters like so,

#if !SILVERLIGHT


        // Creates an XmlWriter for writing into the provided file with the specified settings.
        [ResourceConsumption(ResourceScope.Machine)]
        [ResourceExposure(ResourceScope.Machine)]
        public static XmlWriter Create(string outputFileName, XmlWriterSettings settings = null)
        {
            return (setting ?? new XmlWriterSettings()).CreateWriter(outputFileName);
        }
#endif

        // Creates an XmlWriter for writing into the provided stream.
        public static XmlWriter Create(Stream output, XmlWriterSettings settings = null)
        {
            return (setting ?? new XmlWriterSettings()).CreateWriter(output);
        }

        // Creates an XmlWriter for writing into the provided TextWriter with the specified settings.
        public static XmlWriter Create(TextWriter output, XmlWriterSettings settings = null)
        {
            return (setting ?? new XmlWriterSettings()).CreateWriter(output);
        }

        // Creates an XmlWriter for writing into the provided StringBuilder with the specified settings.
        public static XmlWriter Create(StringBuilder output, XmlWriterSettings settings = null)
        {
            if (output == null) { throw new ArgumentNullException("output"); } // ** A Small perf improvement **
            return (setting ?? new XmlWriterSettings()).CreateWriter(new StringWriter(output, CultureInfo.InvariantCulture));
        }
        // Creates an XmlWriter wrapped around the provided XmlWriter with the specified settings.
        public static XmlWriter Create(XmlWriter output, XmlWriterSettings settings = null)
        {
            return (setting ?? new XmlWriterSettings()).CreateWriter(output);
        }

Note: I also reorder one of the guards so if the output == null then you don't create the XmlWriterSettings object then only to throw it away because of the thrown exception.

If the constructor of XmlWriterSetting also use optional parameters, you could utilise named parameter syntax.
XmlReader.Create(reader, new XmlReaderSettings( IgnoreWhitespace : true ));

Imagine that the original API used optional arguments and there was only one Save method. How would you keep that and now add an CancellationToken parameter?

It would be a new method SaveAsnc( ... blah .. ) since the return type would change, to Task
The existing one could can the new Async method and do a blocking wait.

[AArnott]

@paulomorgado I'd be curious to know your reasons for being so strongly against optional parameters on public methods. I know in the past .NET has had a similar policy, but I don't know if that's being carried into CoreCLR or not -- it seems like an old school restriction IMO.

Your proposal that object initializers can effectively emulate optional parameters is insufficient when the object you're constructing is immutable, which is becoming increasingly popular. But optional parameters work really well in these cases from what I've observed.

[davkean]

We've been playing around with a proposal from the Roslyn guys around how to version methods with optional parameters, this was the last thing that was preventing us from using them.

Imagine we shipped V1 with the following:

Task SaveAsync(Stream stream, SaveOptions options = SaveOptions.None)

V2 comes along and we want to add CancellationToken parameter so that the operation can be cancelled.

You first make the original overload non-optional (binaries built against it will continue to run):

Task SaveAsync(Stream stream, SaveOptions options);

Then you introduce a second overload is that optional:

Task SaveAsync(Stream stream, SaveOptions options = SaveOptions.None, CancellationToken token = null)

This covers the recompile case and dynamic binding cases. They pick up the new overload, but we would guarantee that the behavior was the same.

[paulomorgado]

@AdamSpeight2008, XmlReaderSettings/XmlWriterSettings can be extended without requiring new overloads.

@AdamSpeight2008, @AArnott, @davkean, I'm sure you know that the default argument will be hardcoded on the caller and, without recompilation, the callee cannot change it. Also, for all intents and purposes, all arguments must be passed.

Am I the only one that finds ridiculous that the solution to versioning not using overloads is using overloads? Why not stick with them in the first place?

If you really are thinking to follow this path, you must be dead sure about what you're doing and that the defaults will never change. Because it might not be possible to recompile all the code using those libraries.

And, also, only use optional arguments when it's default value is the default value for its type:

Task SaveAsync(Stream stream, SaveOptions options = default(SaveOptions))

Task SaveAsync(Stream stream, SaveOptions options = SaveOptions.None, CancellationToken token = default(CancellationToken))

By the way, CancellationToken is a struct and when you want to indicate that you haven't one you pass CancellationToken.None, which is the same as default(CancellationToken).

@AArnott, I could argue that using optional arguments is "old school" for those that lived too much time in languages that do not have overloads think that optional arguments is just the same thing done different.

@AArnott, I'm sorry but I don't understand what mutability or immutability has to do with this.

And what comes next? Publicly accessible field constants?

[davkean]

Paulo,

Yes, we would never change defaults for optional arguments. We already avoid changing defaults values for overloads - turns out people rely on them. :)

Dave

[s-aida]

VB has lacked overloads until VB.NET, yet had optional since VB4, but I don't think optional is considered as old school fashion among VB users. Even If so, I don't think many people on corefx prefer VB over C# or are VB-oriented so we can pretty much focus on what's good at the edge C#.

[AArnott]

@paulomorgado Object initializers only work when objects are mutable. If the object is immutable, an object initializer doesn't work because you have no settable properties or fields. Therefore, to initialize the object either the number of constructor/factory method overloads you have either grows exponentially with the number of fields, or you have fewer overloads than some will find helpful, or... you use optional parameters on just a very few overloads.

Publicly accessible field constants? We already have those, too. I was surprised just a few months ago by this as well, but evidently when you have a singleton pattern, it is sometimes acceptable to make a public static readonly field on a type rather than a private static readonly field with a public static property getter. One reason for this is the JIT actually can do better with fields than properties (I know that's against some of the claims, but it's sadly true). I don't mean to defend the policy, I'm just saying it's not next because it's already here. :-|

[paulomorgado]

@AArnott, the compiler doesn't handle const fields the same way as non const fields. Does it?

Take these sample classes:

class C1
{
    public const string c = "const";
}
class C2
{
    public static string f = "field";
}

This code:

var v1 = C1.c;
var v2 = C2.f;

will be compiled to:

ldstr       "const"
stloc.0     // v1
ldsfld      C2.f
stloc.1     // v2

If you later change C1.c and C2.f without recompiling the client code, v1 will still have the value of "const" but v2 will have whatever value C2.f now has.

That's the same with String.Empty which is declared as public static readonly string Empty = "" and not as public const string Empty = "".

Optional arguments behave the same way as const fields, not static readonly fields.

What on the framework is already being exposed as a public const field?

And I don't by into this immutability fundamentalism. Immutability is good when it's good for the good reasons. The settings objects that I'm talking about are just DTOs or messages. If you look at the reference implementations I mentioned, you'll see that the code, although using these objects, doesn't old on to them - and it could be better.

[paulomorgado]

@davkean,

I'm well aware of that. I try not to and sometimes I prefer to call the big overload with all the documented defaults, just in case you change them without my authorization. 😄

Let's get back to the example of SaveAsync. So, we start with:

Task SaveAsync(Stream stream, SaveOptions options = default(SaveOptions))
{
    ...
}

And later we decide to add a cancellation token. Using the mentioned pattern, we remove the default argument value of the old method and put it in the new:

public Task SaveAsync(Stream stream, SaveOptions options)
{
    ...
}

public Task SaveAsync(Stream stream, SaveOptions options = default(SaveOptions), CancellationToken token = default(CancellationToken))
{
    ...
}

Either using proper overloads or optional arguments, seems obvious that the behavior of first method is the same as the behavior of the second. So, we just call the second from the first with the default value:

public Task SaveAsync(Stream stream, SaveOptions options)
{
    return this.SaveAsync(stream, options, default(CancellationToken));
}

Plain old school traditional overloading techniques.

Except that we are now writing the default value in two places.

I'm not totally against optional arguments. I'm pretty fine with them when there's no way to change them without the callers also being recompiled. Like this:

public Task SaveAsync(Stream stream, SaveOptions options)
{
    return SaveAsyncImpl(stream, options);
}

public Task SaveAsync(Stream stream, SaveOptions options, CancellationToken token)
{
    return SaveAsyncImpl(stream, options, token);
}

private Task SaveAsyncImpl(Stream stream, SaveOptions options = default(SaveOptions), CancellationToken token = default(CancellationToken))
{
    ...
}

[AdamSpeight2008]

Are XmlReader and XmlWriter part of CoreFX?

[paulomorgado]

Found this on a quick search.

I haven't found the code for XmlReader, XmlReaderSettings, XmlWritter and XmlWritterSettings per se, but found its usage.

But, does it really matter, @AdamSpeight2008?

[AdamSpeight2008]

I spotted a couple of places where we could remove an unnecessary object initialisation. A small perf improvement. Planned to do a pull request but couldn't locate XmlReader.

[weshaggard]

XmlReader/XmlWriter are not published yet but they are on our list, no ETA yet however.

[stephentoub]

Can this issue be closed now? Is there an action associated with this at this point?

[paulomorgado]

I'm curious on what the final decision was.

Using optional arguments? Ho about versioning?

[Joe4evr]

Since this was bumped...

What on the framework is already being exposed as a public const field?

Math.PI and Math.E are two.

[krwq]

We do not recommend using optional args. Closing this thread as there is no actions expected.

[paulomorgado]

👍