Optional Parameters should be used

[yngndrw]

In many places there are a number of overloads which could use optional parameters instead. An example would be the many ContinueWith methods within BackgroundJobClientExtensions.cs.

Is there a reason behind this or should they be converted ?

[yngndrw]

Accidentally closed this, re-opening

[odinserj]

I prefer to use optional parameters only for helper methods where it does make sense to use named arguments:

Html.Input(@class: "form-control", name: "login" /*, value: "odinserj", style: "margin:1px" */);

For other cases I prefer to use one method per one operations, despite of the fact it sometimes much harder to write 10+ overloads and this is a very boring stuff. For a quick reason to do so, consider the following method in Hangfire:

public static string ContinueWith<T>(
    this IBackgroundJobClient client,
    string parentId,
    Expression<Action<T>> methodCall,
    IState nextState)

For this method we can set the default value for nextState parameter. But what value to choose? We can't set it to new EnqueuedState, because the compiler tells us a default parameter value of a reference type other than string can only be initialized with null.

But if we set it to null, this does not make any sense, BackgroundJob.ContinueWith("some-id", () => Console.Write(), null) call may be very confusing as we don't know what state is being used, and we may treat that it is possible to create a continuation with a null state. For overload without nextState parameter we can write an xml doc that states that EnqueuedState will be used for a continuation.

And I see a lot of overloads in .NET BCL itself, they are doing the boring stuff of adding a lot of overloads instead of using optional parameters too.

[yngndrw]

I do see your point about nextState and that a value of null may be confusing, but I would argue that with an appropriate XML comment it would be self explanatory. It also prevents you from getting an argument null exception when passing in null, which I'd argue should just use an appropriate default anyway. (I see null as meaning either "don't use anything" or "I don't care what's used, pick a value for me" depending on the context - In this case I would see it as being the latter.)

The other way to look at it would be to say that some parameters should be optional, while others could use overloads where it is more appropriate. (Or where there isn't a meaningful default value)

For easy reference, here are the non-generic ContinueWith overloads:

        public static string ContinueWith(
            this IBackgroundJobClient client,
            string parentId,
            [InstantHandle] Expression<Action> methodCall)

        public static string ContinueWith(
            this IBackgroundJobClient client,
            string parentId,
            [InstantHandle] Expression<Action> methodCall,
            IState nextState)

        public static string ContinueWith(
            [NotNull] this IBackgroundJobClient client,
            string parentId,
            [InstantHandle] Expression<Action> methodCall,
            JobContinuationOptions options)

        public static string ContinueWith(
            [NotNull] this IBackgroundJobClient client,
            string parentId,
            [InstantHandle] Expression<Action> methodCall,
            IState nextState,
            JobContinuationOptions options)

If we were to use a mix we could combine these into these two, which I think is the best of both worlds as options has a clear default but specifying nextState is a special case and also quite rare:

        public static string ContinueWith(
            [NotNull] this IBackgroundJobClient client,
            string parentId,
            [InstantHandle] Expression<Action> methodCall,
            JobContinuationOptions options = JobContinuationOptions.OnlyOnSucceededState)

        public static string ContinueWith(
            [NotNull] this IBackgroundJobClient client,
            string parentId,
            [InstantHandle] Expression<Action> methodCall,
            IState nextState,
            JobContinuationOptions options = JobContinuationOptions.OnlyOnSucceededState)

[odinserj]

@yngndrw, sorry for the delay. I think that the changes should be introduced in 2.0.0, only if this would help to drastically decrease the number of overloads – there will be a confusion if half of overloads would use regular overload semantic, and another half using the optional parameters.