27

Consider the following method:

public List<Guid> ReturnEmployeeIds(bool includeManagement = false)
{

}

And the following call:

var ids = ReturnEmployeeIds(true);

For a developer new to the system, it'd be pretty difficult guessing what true did. First thing you'd do is hover over the method name or go to the definition (neither of which are big tasks in the slightest). But, for readability sake, does it make sense to write:

var ids = ReturnEmployeeIds(includeManagement: true);

Is there anywhere that, formally, discusses whether or not to explicitly specify optional parameters when the compiler doesn't need you to?

The following article discusses certain coding conventions: https://msdn.microsoft.com/en-gb/library/ff926074.aspx

Something similar to the article above would be great.

JᴀʏMᴇᴇ
  • 2,361
  • 4
  • 22
  • 25
  • 2
    These are two rather unrelated questions: (1) Should you write out optional arguments for clarity? (2) Should one use`boolean` method arguments, and how? – Kilian Foth Jan 19 '16 at 10:47
  • What do you mean by "formally discusses"? – MetaFight Jan 19 '16 at 10:53
  • I'd call the likes of the following article 'formal' enough for what I'm after: https://msdn.microsoft.com/en-gb/library/ff926074.aspx – JᴀʏMᴇᴇ Jan 19 '16 at 10:54
  • 5
    This all boils down to personal preference/style/opinion. Personally, I like parameter names when the value passed in is a literal (a variable might already convey enough information through its name). But, that's my personal opinion. – MetaFight Jan 19 '16 at 10:54
  • 1
    maybe include that link as an example source in your question? – MetaFight Jan 19 '16 at 10:56
  • @MetaFight - good idea. – JᴀʏMᴇᴇ Jan 19 '16 at 10:57
  • @JᴀʏMᴇᴇ, the link you provide may be "formal", even "official", but it's pure opinion. Also, "Is there anywhere that, formally, discusses..." is a request for third party resources, which is outside the scope of questions here on Programmers. – David Arno Jan 19 '16 at 11:22
  • I don't mind opinion, I asked for 'formal'. MSDN may be primarily opinion based, but it's formal enough for me. – JᴀʏMᴇᴇ Jan 19 '16 at 11:26
  • 4
    If it adds anything, I ran it thru StyleCop & ReSharper - neither had any clear view on it. ReSharper simply says the named parameter *could* be removed and then goes on to say a named parameter *could* be added. Advice is free for a reason I guess... :-/ – Robbie Dee Jan 19 '16 at 12:00
  • 4
    Possible duplicate of [Named arguments (parameters) as a readability aid](http://programmers.stackexchange.com/questions/52873/named-arguments-parameters-as-a-readability-aid) – Doc Brown Jan 19 '16 at 13:37
  • 1
    See also [this SO post](http://stackoverflow.com/questions/7041752/any-reason-not-to-always-use-keyword-arguments) for Python. – Doc Brown Jan 19 '16 at 13:40
  • i think fxcop actually advises against optional parameters at all. – Andy Sep 21 '17 at 00:03

7 Answers7

28

I'd say that in the C# world, an enum would be one of the best options here.

With that you'd be forced to spell out what you're doing and any user would see whats happening. It's also safe for future extensions;

public enum WhatToInclude
{
    IncludeAll,
    ExcludeManagement
}

var ids = ReturnEmployeeIds(WhatToInclude.ExcludeManagement);

So, in my opinion here:

enum > optional enum > optional bool

Edit: Due to discussion with LeopardSkinPillBoxHat below regarding a [Flags] enum which in this specific case might be a good fit (since we specifically are talking about including/excluding things), I instead propose using ISet<WhatToInclude> as a parameter.

It's a more "modern" concept with several advantages, mostly stemming from the fact that it fits in the LINQ family of collections, but also that [Flags] has a maximum of 32 groups. The main downside of using ISet<WhatToInclude> is how badly sets are supported in C# syntax:

var ids = ReturnEmployeeIds(
    new HashSet<WhatToInclude>(new []{ 
        WhatToInclude.Cleaners, 
        WhatToInclude.Managers});

Some of it might be mitigated by helper functions, both for generating the set and for creating "shortcut sets" such as

var ids = ReturnEmployeeIds(WhatToIncludeHelper.IncludeAll)
NiklasJ
  • 675
  • 4
  • 6
  • I tend to agree; and have generally taken to using enums in my new code if there's any vaguely chance that the conditions will be expanded in the future. Replacing a bool with an enum when something becomes a tri-state ends up creating really noisy diffs and making blaming code a lot more tedious; when you end up having to retrofit your code again a year down the road for a 3rd option. – Dan Is Fiddling By Firelight Jan 19 '16 at 19:33
  • 3
    I like the `enum` approach, but I'm not a huge fan of mixing `Include` and `Exclude` in the same `enum` as it's more difficult to grasp what's going on. If you want this to be truly extensible, you can make it a `[Flags]` `enum`, make them all `IncludeXxx`, and provide a `IncludeAll` which is set to `Int32.MaxValue`. Then you can provide 2 `ReturnEmployeeIds` overloads - one which returns *all* employees, and one which takes a `WhatToInclude` filter parameter which can be a combination of enum flags. – LeopardSkinPillBoxHat Jan 20 '16 at 04:32
  • @LeopardSkinPillBoxHat Ok, I agree on the exclude/include. So this is a bit of contrived example, but I could have called it IncludeAllButManagement. On your other point, I don't agree - I think `[Flags]` is a relic and only should be used for legacy or performance reasons. I think a `ISet` is a vastly better concept (unfortunately C# is lacking some syntactic sugar to make it nice to use). – NiklasJ Jan 20 '16 at 09:23
  • @NiklasJ - I like the `Flags` approach because it allows the caller to pass `WhatToInclude.Managers | WhatToInclude.Cleaners` and the bitwise or expresses exactly how the caller will process it (i.e. managers or cleaners). Intuitive syntactic sugar :-) – LeopardSkinPillBoxHat Jan 20 '16 at 09:35
  • @LeopardSkinPillBoxHat Exactly, but that's the only upside of that method. Downsides include for example limited number of choices and not being compatible with other linq-like concepts. – NiklasJ Jan 20 '16 at 09:50
  • @NiklasJ Here is how to add sugar to IEnumerable :) http://codereview.stackexchange.com/questions/117845/flags-on-steroids – Dmitry Nogin Jan 25 '16 at 12:05
  • Sets are actually supported slightly better, you can write `new HashSet { WhatToInclude.Cleaners, WhatToInclude.Managers }`. – svick Mar 07 '16 at 00:31
  • [Flags] has a maximum of *64* unique values: [System.Flags] public enum ErrorCodeType :* long* {...} – user1454085 Jun 24 '20 at 12:59
20

does it make sense to write:

 var ids=ReturnEmployeeIds(includeManagement: true);

It is debatable if this is "good style", but IMHO this is as least not bad style, and useful, as long as the code line does not become "too long". Without named parameters, it is also a common style to introduce an explaining variable:

 bool includeManagement=true;
 var ids=ReturnEmployeeIds(includeManagement);

That style is probably more common among C# programmers than the named parameter variant, since named parameters were not part of the language before Version 4.0. The effect on readability is almost the same, it just needs an additional line of code.

So my recommendation: if you think using an enum or two functions with different names is "overkill", or you don't want to or cannot change the signature for a different reason, go ahead and use the named parameter.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • Might want to note that you are using additional memory on the second example – Alvaro Jan 19 '16 at 19:41
  • 10
    @Alvaro That's highly unlikely to be true in such a trivial case (where the variable has a constant value). Even if it was, it's hardly worth mentioning. Most of us aren't running on 4KB of RAM these days. – Chris Hayes Jan 19 '16 at 20:40
  • 3
    Because this is C#, you could mark `includeManagement` as a local `const` and avoid using any additional memory at all. – Jacob Krall Jan 19 '16 at 21:08
  • 8
    Guys, I am not going to add any stuff to my answer which might only distract from what I think is important here. – Doc Brown Jan 19 '16 at 21:21
17

If you encounter code like:

public List<Guid> ReturnEmployeeIds(bool includeManagement = false)
{

}

you can pretty much guarantee what's inside the {} will be something like:

public List<Guid> ReturnEmployeeIds(bool includeManagement = false)
{
    if (includeManagement)
        return something
    else
        return something else
}

In other words, the boolean is used to choose between two courses of action: the method has two responsibilities. This is pretty much guaranteed a code smell. We should always strive to ensure that methods only do one thing; they only have one responsibility. Therefore, I'd argue both of your solutions are the wrong approach. Using optional parameters is a sign that the method is doing more that one thing. Boolean parameters are also a sign of this. Having both should definitely set alarm bells ringing. What you should do therefore, is refactor the two different sets of functionality out into two separate methods. Give the methods names that make clear what they do, eg:

public List<Guid> GetNonManagementEmployeeIds()
{

}

public List<Guid> GetAllEmployeeIds()
{

}

This both improves the readability of the code and ensures you are better following SOLID principles.

EDIT As has been pointed out in the comments, the case here that these two methods will likely have shared functionality, and that shared functionality will likely best be wrapped in a private function that will need a boolean parameter to control some aspect of what it does.

In this case, should the parameter name be supplied, even though the compiler doesn't need it? I'd suggest there's no simple answer to that and that judgement is needed on a case by case basis:

  • The argument for specifying the name, is that other developers (including yourself in six months time) should be the primary audience of your code. The compiler is definitely a secondary audience. So always write code to make it easier for other people to read; rather than just supplying what the compiler needs. An example of how we use this rule every day, is that we do not fill our code with variables _1, _2 etc, we use meaningful names (which mean nothing to the compiler).
  • The counter argument is that private methods are implementation details. One could reasonably expect anyone looking at a method such as below, would trace through the code to understand the purpose of the boolean parameter as they are inside the internals of the code:
public List<Guid> GetNonManagementEmployeeIds()
{
    return ReturnEmployeeIds(true);
}

Is the source file, and the methods, small and easily understood? If yes, then the named parameter probably amounts to noise. If no, it may well be very helpful. So apply the rule according to circumstances.

David Arno
  • 38,972
  • 9
  • 88
  • 121
  • 12
    I hear what you're saying, but you've sort of missed the point of what I'm asking. Assuming a scenario whereby it *is* most appropriate to use optional parameters (these scenarios do exist), is it ok to name the parameters even though it's unnecessary? – JᴀʏMᴇᴇ Jan 19 '16 at 11:26
  • 4
    All true, but the question was about a *call* to such a method. Also a flag parameter doesn't guarantee a branch within the method at all. It might simply be passed on to another layer. – Robbie Dee Jan 19 '16 at 11:30
  • This is not wrong, but oversimplifying. In real code, you will find `public List ReturnEmployeeIds(bool includeManagement) { calculateSomethingHereForBothBranches; if (includeManagement) return something else return something else }`, so splitting that simply into two methods will probably mean those two methods will need the result of the first calculation as a parameter. – Doc Brown Jan 19 '16 at 11:58
  • 4
    I think what we're all saying is the the *public* face of your class should probably expose two methods (each with one responsibility) but, internally, each of those methods can reasonably forward the request to a **single** *private* method with a branching bool parameter. – MetaFight Jan 19 '16 at 12:25
  • @MetaFight, absolutely. – David Arno Jan 19 '16 at 12:42
  • @JᴀʏMᴇᴇ, I've updated the answer to address that. – David Arno Jan 19 '16 at 12:53
  • @DocBrown, you make a good point that there likely will be a private method with a bool parameter being these methods. I've updated the answer to reflect that. – David Arno Jan 19 '16 at 12:54
  • The annoying thing about two separate methods is that now any code that controls whether to include management by use of a boolean now has to have that if/else logic. If that happens in many places, that means a lot of boilerplate. – Reinstate Monica Jan 19 '16 at 18:08
  • 1
    I can imagine 3 cases for the behavior that differs between the two functions. 1. There is pre-processing that is different. Have the public functions pre-process arguments into the private function. 2. There is post-processing that's different. Have the public functions post-process the result from the private function. 3. There is intermediate behavior that differs. This can be passed in as a lambda to the private function, assuming your language supports it. Often times, this leads to a new reusable abstraction that didn't really occur to you before. – jpmc26 Jan 19 '16 at 18:08
  • @Solomonoff'sSecret The normal route that a function with an optional boolean parameter comes into existence is because it's used throughput the code in one form (eg returns all IDs) and then a new requirement comes along (eg for non-management IDs). Rather than create a new function, the developer applies a "quick fix" of a boolean option parameter and it never gets properly fixed later. You are arguing from the opposite position that the method with the optional boolean comes first. That's highly unlikely. – David Arno Jan 19 '16 at 18:54
  • Whoa!! It's certainly POSSIBLE that a Boolean parameter is used to decide which of two totally separate code blocks to execute. And I heartily agree that that would be a bad thing: yes, you should just have made two functions. But I can imagine a hundred other scenarios. Maybe there are many lines of code in the function and the Boolean controls one tiny distinction. Maybe the Boolean is just passed as part of a WHERE clause that controls which records are retrieved, e.g. WHERE includeManagement = 1 OR level!='M'. Etc. "Boolean parameters are evil" is way too simplistic. – Jay Jan 20 '16 at 04:02
  • @Jay, "Maybe there are many lines of code in the function..." Then rewrite the function as having many lines of code is a sign of bad design. "Maybe the Boolean ... controls which records are retrieved..." Then use a enum to make the purpose of the parameter clear. Boolean parameters aren't evil, any more than goto, switch, tightly coupled code, no unit tests etc etc are. They are simply signs of poor design. "Don't use boolean parameters" is a principle that should be applied pragmatically. If an edge-case demands one, then use one; but only do so if not doing so would make the design worse. – David Arno Jan 20 '16 at 08:33
  • @DavidArno I don't accept that "many lines of code is a sign of bad design". A function that does many different things is bad design. But a function that copies record A to record B might have many assignment statements to do all the moves, but could be fundamentally very simple. Breaking it in half because of some arbitrary rule that a function shouldn't be more than 50 lines or whatever would be terrible design. And "many lines of code" in this context could be 10. If there were ten lines of tightly-coupled code, and line 6 said IF fooFlag THEN bar+=foo, then breaking into two functions ... – Jay Jan 20 '16 at 14:47
  • ... with the first five lines and the second five lines pushed into subfunctions, and with a bunch of data having to be passed between them, would be painful. Duplicating the code and having two versions, one with the conditional line and one without, would be worse. – Jay Jan 20 '16 at 14:49
  • @DavidArno When I've written in languages that did not have named parameters, yes, sometimes I have created enums with values of true and false to make the code more readable. But when the language provides named parameters, the code is more clear if you DON'T do this. If I write doFoo(includeManagement) and doFoo(dontIncludeManagement), I make clear what the parameter means but I obscure the fact that it's really a Boolean. But doFoo(includeManagement:=true) has all the advantages of the enum while also making the parameter type clear. And eliminating the enum eliminates clutter. – Jay Jan 20 '16 at 14:52
1

For a developer new to the system, it'd be pretty difficult guessing what true did. First thing you'd do is hover over the method name or go to the definition (neither of which are big tasks in the slightest)

Yes, true. Self documenting code is a beautiful thing. As other answers have pointed out, there are ways to remove the ambiguity of the call to ReturnEmployeeIds by using an enum, different method names, etc. However sometimes you can't really avoid the case but you don't want to go off and use them everywhere (unless you like the verbosity of visual basic).

For instance, it may help clarify one call but not necessarily another.

A named argument may be helpful here:

var ids = ReturnEmployeeIds(includeManagement: true);

Not add much additional clarity (I'm going to guess that this is parsing an enum):

Enum.Parse(typeof(StringComparison), "Ordinal", ignoreCase: true);

It may actually reduce clarity (if a person doesn't understand what capacity on a list is):

var employeeIds = new List<int>(capacity: 24);

Or where it doesn't matter because you're using good variable names:

bool includeManagement = true;
var ids = ReturnEmployeeIds(includeManagement);

Is there anywhere that, formally, discusses whether or not to explicitly specify optional parameters when the compiler doesn't need you to?

Not AFAIK. Let's address both parts though: the only time you need to explicitly use named parameters is because the compiler requires you to do so (generally it's to remove statement ambiguity). So other than that you can use them whenever you'd like. This article from MS has some suggestions on when you may want to use them but it's not particularly definitive https://msdn.microsoft.com/en-us/library/dd264739.aspx.

Personally, I find I use them most often when I'm creating custom attributes or stubbing out a new method where my parameters may change or be reordered (b/c named attributes can be listed in any order). Additionally I only use them when I'm providing a static value inline, otherwise if I'm passing a variable I try to use good variable names.

TL;DR

Ultimately it comes down to personal preference. Of course there are a few use cases when you have to use named parameters but after that you need to make a judgement call. IMO - Use it anywhere that you believe it will help document your code, reduce ambiguity, or allow you to protect method signatures.

0

If the parameter is obvious from the (fully qualified) name of the method and it's existence is natural to what the method is supposed to do, you should not use the named parameter syntax. For example, in ReturnEmployeeName(57) it's pretty damn obvious that 57 is the employee's ID, so it's redundant to annotate it with the named parameter syntax.

With ReturnEmployeeIds(true), like you said, unless you look at the function's declaration/documentation it's impossible to figure what true means - so you should use the named parameter syntax.

Now, consider this third case:

void PrintEmployeeNames(bool includeManagement) {
    foreach (id; ReturnEmployeeIds(includeManagement)) {
        Console.WriteLine(ReturnEmployeeName(id));
    }
}

The named parameter syntax is not used here, but since we are passing a variable to ReturnEmployeeIds, and that variable has a name, and that name happens to mean the same thing as the parameter we are passing it to(this is often the case - but not always!) - we don't need the named parameter syntax to understand it's meaning from the code.

So, the rule is simple - if you can easily understand from just the method call what the parameter is supposed to mean, don't use the named parameter. If you can't - that's a deficiency in the code's readability, and you probably want to fix those(not at any price, of course, but you usually want the code to be readable). The fix doesn't have to be named parameters - you can also put the value in a variable first, or use the methods suggested in the other answers - as long as the end result makes it easy to understand what the parameter does.

Idan Arye
  • 12,032
  • 31
  • 40
  • 7
    I disagree that `ReturnEmployeeName(57)` makes it immediately obvious that `57` is the ID. `GetEmployeeById(57)` on the other hand... – Hugo Jan 19 '16 at 13:52
  • @HugoZink I initially wanted to use something like that for the example, but I intentionally changed it to `ReturnEmployeeName` to demonstrate cases where even without explicitly mentioning the parameter in the method's name, it's fairly reasonable to expect people to figure what it is. At any rate, it's noteworthy that unlike `ReturnEmployeeName`, you can't reasonably rename `ReturnEmployeeIds` to something that indicates the meaning behind the parameters. – Idan Arye Jan 19 '16 at 14:56
  • 1
    In any case, it's pretty unlikely that we would write ReturnEmployeeName(57) in a real program. It's far more likely that we would write ReturnEmployeeName(employeeId). Why would we hardcode an employee ID? Unless it's the programmer's ID and there's some sort of fraud going on, like, add a little to this employee's paycheck. :-) – Jay Jan 20 '16 at 04:06
0

Yes, I think it's good style.

This makes most sense for Boolean and integer constants.

If you're passing in a variable, the variable name should make the meaning clear. If it doesn't, you should give the variable a better name.

If you're passing in a string, the meaning is often clear. Like if I see a call to GetFooData("Oregon"), I think a reader can pretty much guess that the parameter is a state name.

Not all strings are obvious, of course. If I see GetFooData("M") then unless I know the function I have no idea what "M" means. If it's some kind of code, like M="management-level employees", then we probably should have an enum rather than a literal, and the enum name should be clear.

And I suppose a string could be misleading. Maybe "Oregon" in my example above refers, not to the state, but to a product or a client or whatever.

But GetFooData(true) or GetFooData(42) ... that could mean anything. Named parameters are a wonderful way to make it self-documenting. I've used them for exactly that purpose on a number of occasions.

Jay
  • 2,657
  • 1
  • 14
  • 11
0

There aren't any super clear reasons as why to use this type of syntax in the first place.

In general, try to avoid having having boolean arguments that at a distance may look arbitrary. includeManagement will (most likely) affect the result greatly. But the argument looks like it carries "little weight".

The use of an enum has been discussed, not only will it look like the argument "carries more weight", but it will also provide scalability to the method. This might however not be the best solution in all cases, since your ReturnEmployeeIds-method will have to scale alongside the WhatToInclude-enum (see NiklasJ's answer). This might give you a headache later.

Consider this: If you scale the WhatToInclude-enum, but not the ReturnEmployeeIds-method. It might then throw a ArgumentOutOfRangeException (best case) or return something completely unwanted (null or an empty List<Guid>). Which in some cases might confuse the programmer, especially if your ReturnEmployeeIds is in a class-library, where the source-code is not readily available.

One will assume that WhatToInclude.Trainees will work if WhatToInclude.All does, seeing that trainees is a "subset" of all.

This does (of course), depend on how ReturnEmployeeIds is implemented.


In cases where a boolean argument could be passed in, I try to instead break it up into two methods (or more, if needed), in your case; one might extract ReturnAllEmployeeIds, ReturnManagementIds and ReturnRegularEmployeeIds. This covers all bases and is absolutely self-explanatory to whoever implements it. This will also not have the "scaling"-issue, mentioned above.

Since there are only ever two outcomes for something that has a boolean argument. Implementing two methods, takes very little extra effort.

Less code, is seldom better.


With this said, there are some cases where explicitly declaring the argument improves readability. Consider for instance. GetLatestNews(max: 10). GetLatestNews(10) is still pretty self-explanatory, the explicit declaration of max will help clear up any confusion.

And if you absolutely must have a boolean argument, which usage cannot be inferred by just reading true or false. Then I would say that:

var ids = ReturnEmployeeIds(includeManagement: true);

.. is absolutely better, and more readable than:

var ids = ReturnEmployeeIds(true);

Since in the second example, the true might mean absolutely anything. But i would try to avoid this argument.

mausworks
  • 768
  • 5
  • 10