PoshCode / PowerShellPracticeAndStyle

The Unofficial PowerShell Best Practices and Style Guide
https://poshcode.gitbooks.io/powershell-practice-and-style
Other
2.22k stars 289 forks source link

Should Parameters Always Have Default Values? #67

Open Jaykul opened 8 years ago

Jaykul commented 8 years ago

I've already written about this, but it's probably worth having this conversation here.

Function parameters are always automatically defined in the local scope and initialized by PowerShell to their default value (the equivalent of null -- which comes out as an empty string or a zero for numerical values, or a default struct, etc).

This happens even when they are part of an unused parameter set.

There is, in my opinion, nothing to be gained in setting them yourself -- unless you actually need them to default to something other than $null.

If anything, this should be warned against for performance reasons, and because people frequently provide default values on mandatory parameters (which can be misleading and even invalid defaults, since they're not used).

As with C# and most other programming languages, initializing a variable to it's default value is just extra work that accomplishes nothing. It might even make your script (infinitesimally) slower, whilst changing nothing.

rkeithhill commented 8 years ago

Agreed. Although Tobias brings up a fair point about folks using non-mandatory without default values later in their script without ensuring definite assignment first - resulting in potential $null reference errors.

If there isn't already a PSSA rule for this, I'd love to see one added. But even without the rule, I would encourage null-checking on non-mandatory parameters over assigning $null as def value - which actually doesn't help with the $null reference problem at all except that perhaps the scripter will see that the default value is $null. But that doesn't change the need for null checking later as the scripter has no idea whether the function caller provided a value for that parameter or not.

TobiasPSP commented 8 years ago

There are two issues to this IMHO. Technically, it is not necessary to assign a default value to an optional parameter if you are fine with the default $null value, nor is it always possible (as with DateTime type as was pointed out). Assigning $null to an optional parameter as a default value does produce better manageable code, though. While truly experienced PowerShell writers know about the default assignments, mere mortals in the field often do not. When assessing production code, very often there are functions that declare optional parameters that need values other than null. They cannot use $null. If the author was asked to either assign $null as default, or mark the parameter as mandatory, the author would be forced to think about it and quickly identifies parameters that cannot use $null and should be mandatory. So the rule "optional parameters should have a default value" is not a technical requirement. It is a rule designed to pinpoint a potential issue.

TobiasPSP commented 8 years ago

Commenting on c#: When you write a method in c#, you need to define parameters. You can either provide an explict default value (in which case the parameters are optional), or provide no default value (in which case they are mandatory). What you cannot do is define a parameter without default value and then omit it. It is different with fields. They are initialized to null/0/false. However, "publicly" accessible structures such as methods (in c#) and functions (in PowerShell) should leave no doubt what their respective value should be. The fact that PS uses field initializer for optional parameters with no default value IMHO is just something to make PowerShell more robust, but not a best practice that should be recommended.

rkeithhill commented 8 years ago

When assessing production code, very often there are functions that declare optional parameters that need values other than null.

Absolutely and that is the case where you do want to use a default value for a parameter.

Assigning $null to an optional parameter as a default value does produce better manageable code

I don't agree on this because you still need to null check the parameter before using it. Assigning $null as a default value does not change this.

Jaykul commented 8 years ago

But @rkeithhill, that's not a valid point. If you need to set the default to something other than $null, then of course you should do that. But setting it to null doesn't accomplish anything (it already was null, that's why you hypothetically had that problem). You're merely invoking an assignment to no purpose.

@rkeithhill as I linked in the OP -- there used to be a rule. It was wrong. I convinced them to ditch it.

@TobiasPSP always assigning default values (and especially adding rules about it) is misleading your "mere mortals" -- you're making them think that they need to do things they do not. They need to learn that PowerShell parameters already have default values -- not learn that they have to be assigned (when they do not!).

In PowerShell, ALL PARAMETERS are initialized. EXACTLY THE SAME WAY that in C#, all variables are initialized.

TobiasPSP commented 8 years ago

Assigning $null to a default value is an active measure you took, so chances are you thought about it. Not assigning anything is often something that just slipped through.

TobiasPSP commented 8 years ago

@Jaykul that is not true. C# does not work the way you suggest. Compare method parameters, not field initializers.

Jaykul commented 8 years ago

It is absolutely incorrect that "mere mortals" think about what PSScriptAnalyzer tells them to do. It says assign a default value, they just do that.

rkeithhill commented 8 years ago

@Jaykul If you need to set the default to something other than $null, then of course you should do that.

That's what I thought I said. Sorry if it wasn't clear.

TobiasPSP commented 8 years ago

@Jaykul in the real world, we have uncovered tons of issues this way. People write functions and define parameters, then they call them always with their sets of arguments. Once they export this to module and make it public, people start to submit different arguments (or none), and all breaks. The rule helped people immediately find their issues.

Jaykul commented 8 years ago

@TobiasPSP: you can't compare method parameters, because in C# method parameters are mandatory. Assigning them a default value is the syntax for making them optional, it is not to make them have a value.

rkeithhill commented 8 years ago

Compare method parameters, not field initializers.

It's not about the specific ways each language handles parameters. It is a more general concept of - don't add extra script (more script, more bugs) that is unnecessary.

TobiasPSP commented 8 years ago

So why strongly type a variable? If you always write perfect code, you can get around a lot of things that otherwise can protect you.

TobiasPSP commented 8 years ago

I guess what I want to say is: there is a natural clash when it comes to best practice discussions. Everyone has his or her special background, and of course we all think we know what is best. That's why rules can be disabled. No hard feelings. My focus is on the field. I am at companies every single week, working with thousands of ITPro during a year, reviewing tons of code, most of it pretty ugly. I want rules to be practical, and to produce better and safer code. When they do that, I am happy.

rkeithhill commented 8 years ago

So why strongly type a variable

Because I'm giving PowerShell information it didn't have i.e. that I want this variable to not be type promiscuous as Bruce would say. :-)

Jaykul commented 8 years ago

The point is that you're suggesting people write a variable assignment as "documentation" But what you're documenting is "how PowerShell works" ... not "how my function works"

The only documentation you need of default values is [Parameter(Mandatory)] to indicate when they are not.

TobiasPSP commented 8 years ago

what kind of comment is a "thumbs down"?

TobiasPSP commented 8 years ago

So when a rule proves to identify significant numbers of real-world issues, then you are still opposed to it because people should have just written better code in the first place? Hm, maybe I did not get the idea of these rules then :-)

rkeithhill commented 8 years ago

I want rules to be practical, and to produce better and safer code. When they do that, I am happy.

I still think this could be accomplished with a PSSA rule that verifies the scripter has null checked a non-mandatory parameter in script before reference it for any purpose (other than assigning to it -which would be weird and probably another rule violation). @Jaykul are you sure about the rule you got them to remove? I thought that was just to ensure parameters have default values assigned which is different than what I'm proposing.

Jaykul commented 8 years ago

It took you only seven posts to switch from trying to convince me there's a good reason for this to claiming you know best based on your particular background. Not helpful. That's what a Thumbs Down is.

rkeithhill commented 8 years ago

what kind of comment is a "thumbs down"?

On GitHub you can "react" to someone's comment with a thumbs up/down, yay, etc. Check out the "+smiley" in the upper right of a comment's toolbar .

TobiasPSP commented 8 years ago

@rkeithhill null-checking would be equally valuable in uncovering the issues I talked about. Maybe it would also be a better approach. @Jaykul the purpose was a discussion I assumed. Now it is ideological and personal. I apologize if I did something wrong, was not my intention.

TobiasPSP commented 8 years ago

I especially did not want to appear as "I knew best", I just wanted to illustrate that there is more to this than just a highly technical approach.

Jaykul commented 8 years ago

@rkeithhill null checking is complicated in PowerShell because you can do it in several ways, however, you are right that a rule which actually checks the code would be very useful, and is not what they had before.

TobiasPSP commented 8 years ago

true, the challenge probably would be how you identify whether the PS code in fact did a null checking.

rkeithhill commented 8 years ago

null checking is complicated in PowerShell

I've been too spoiled by C# definite assignment checks. :-)

TobiasPSP commented 8 years ago

Can I ask a final technical question? What (other than more code than needed) are the downsides of assigning an explicit default value to optional parameters? After all, most of the time this is what is done anyway (most of the time, the default value is non-null). Trying to understand the pros and cons.

Jaykul commented 8 years ago

@TobiasPSP: I spend most of my day on Slack and IRC in the PowerShell user group channel. I spend hours each day teaching people PowerShell there. I know that there are many different levels of skill, and many different types of people with different backgrounds learning PowerShell. However, my exposure to how people think and code isn't the point. The point is whether what we should be teaching them.

I am suggesting that what we should be teaching them is that:

  1. Variables (and especially parameters) have default values.
  2. What those values are depends on the variable type (of course you can assign a default to a DateTime, just not $null)
  3. If you want the user to choose the value, you need to use [Parameter(Mandatory)]
  4. If you know a better and acceptable reasonable default value, you may set that
  5. Validation attributes don't run on parameter default values ❗❗
  6. But type coercion and ArgumentTransformation attributes do
  7. $PSBoundParameterValues is your friend, it's the only way you can tell if a user passed the same value as the default value.
TobiasPSP commented 8 years ago

@Jaykul completely agree. Here is what I am struggling with: We don't have the chance to always teach people these things. How can we support them through pure code analysis?

As you and @rkeithhill suggested, analyzing PS code to find out whether there is appropriate null checking would be the technically best way but is probably prohibitively complex. In addition, when you think about it, we don't just need $null checking, we then need "validation" validation, thus need to check whether any default value is suitable, not just $null. That's out of scope I suppose.

When you look at the source of most problems, it starts IMHO here: many novices just define the parameters for the arguments their code needs. They don't consider the case where arguments are not submitted, nor do they see that this can create unexpected $null values.

I hope you see that I did not want to brute force my view. I am struggling to find a manageable way to solve this, just like you. The perfect rule might be: "always assign an appropriate default value to optional parameters when the default value should not be $null".

But since you cannot detect whether $null is or is not appropriate, the second best rule would be "always assign a default value to optional parameters", considering that $null seldom is the default value that people come up with in the end, and paying the price that there might be some $null assignments that are not really needed.

So what is better?

I agree that technically, assigning a $null value is not a good recommendation. When you do, though, you explicitly "declare" that you are ok with it, and enable rules to find all the other cases. That's why I thought it might still be a good idea.

Jaykul commented 8 years ago

Let me preface this by saying that I heartily endorse the use of reasonable default values for non-mandatory parameters -- the more you can do by default, the better. In particular, I love the fact that I can have a default value for a parameter that uses values from other (mandatory?) parameters. e.g. this is wild

@TobiasPSP the main downsides, as I see them:

First, it's unnecessary code - you're (re)doing something that's already done. The less you write, the less you have to maintain.

This does not just mean extra work on your part (to type it). It also means extra work on PowerShell's part every time it's run, including type coercion (e.g. $null to int = 0).

It also means people who come after you have to read what you wrote, and figure out: Why did he assign it the value it already had? They have to worry about whether they're misunderstanding something, did they misread the type? Is there an argument transformation attribute that this is triggering?

Second, it's possible to initialize parameters to values that are not allowed by the Validation attributes. This can cause all sorts of trouble, and is more likely when people are just a checklist that says they must to assign some default value.

Third, it's skipping over teaching people how PowerShell *actually works. It misleads people into thinking they really do have to assign a default value. It makes them believe that this is like Visual Basic (or whatever other language they came from) where they have to initialize everything ...

Additionally, there's the case that struct parameters are always null by default, but cannot be assigned null.

As soon as you get a DateTime or a Rectangle or any other struct (in general, non-numeric ValueType descendants), you cannot assign it null, but if you just leave it alone it will be null. What would your "always assign a default value to optional parameters" rule do here? It will force people to assign something silly and then write a silly test for it. (i.e. testing DateTime values are greater than January 1, 1970, instead of just checking that they're not $null).

cdhunt commented 8 years ago

Are you talking about Functions or Advanced Functions with a param block?

Jaykul commented 8 years ago

@TobiasPSP: but that's the thing, isn't it:

having no option to point novices to potentially undefined optional parameters through automated rules at all

They are never "potentially undefined" -- this is EXACTLY my point. When you teach that you must assign a default, you are (implicitly, at least) teaching them that the variable might otherwise be undefined, but that's incorrect.

[int]$parameter - all the number types will be zero -- even if you assign $null [string]$parameter - will be an empty string -- even if you assign $null [DateTime]$parameter - will be null ❓❗

In fact, any other reference type or struct will be $null unless you assign it a default value.

@cdhunt: parameter variables within a param block. Does it matter if it's advanced or not? (elsewhere we recommend always writing advanced functions).

TobiasPSP commented 8 years ago

Thank you for sharing your thoughts.

On a side note, I tend to wonder whether the null default value mechanism is just one of the plenty mitigation techniques found in PowerShell to get away with lazy code. I really don't know. I personally feel that explicitly assigning a default value, just like in c# with optional parameters, appears to be a much cleaner and self-documenting approach. Can't the null default value mechanism also conflict with validation attributes, just like manual assignments?

@Jaykul I am now much better seeing your point. You are technically absolutely correct.

I am just arguing that it requires a lot of technical/programming/implicit knowledge to produce solid code: You need to know about types, and strongly type parameters, and you need to know what the default values for the used types are.

Explicitly setting a default value - just like it is done with c# optional parameters in a method - IMHO would be so much easier because it only requires the knowledge level for assigning a value to a variable, and is declarative (you see the value).

I really dunno what's best.

cdhunt commented 8 years ago

@Jaykul, no, I agree with you for both scenario. I was thinking there might be a valid reason with basic functions, but I haven't actually been able to think of one.

cdhunt commented 8 years ago

Also, am I missing something where $PSBoundParameters isn't a valid solution for handling optional parameters?

rkeithhill commented 8 years ago

[DateTime]$parameter - will be null ❓❗

I'm guessing they wrap the param value in a psobject and that gets set to null??

rkeithhill commented 8 years ago

Also, am I missing something where $PSBoundParameters isn't a valid solution for handling optional parameters?

That's yet another way to test if the user supplied a value for the parameter which is handy if you happen to need to know that the user supplied a value even if the value is the default value (implicitly or explicitly set).

Most of the time I don't need to know if the user supplied a value that happened to be the default, so I just check to see if the parameter is not equal to $null before using it. It's probably a cheesy excuse but it's less to type :-)

if ($null eq $Name) {
    #... use $Name
}
Jaykul commented 8 years ago

@TobiasPSP Yes, it's possible that $null is an invalid value -- but it's the expected and easy to test for invalid value. Not like param([DateTime]$When=0) ....

I understand what you mean about getting people to think about their default parameter values, but telling them they must do something, when they don't actually have to do it makes no sense to me: it doesn't improve correctness, and the preponderance of "validation" errors which insist on things which are not true leads to people not trusting the validation.

It's certainly harder to write a tool to verify correctness -- especially since PowerShell will allow me to reference properties of an unset $When without throwing an error, unless I Set-StrictMode, so I can test for null on properties, or just pass through the (possibly null) value to another command which will handle it properly.

Forcing people to initialize does nothing to guarantee correctness

Just because you force them to explicitly set a default value does not mean they will test for it.

I would rather see a "INFO" level warning on the first use of the variable, if it's not: if($parameter) or if($null -ne $parameter) or if($PSBoundParameters.Contains('parameter') ... something like:

The parameter $When is not mandatory, and may be null. You should consider testing the value.

@cdhunt actually, there is one weird case where testing $PSBoundParameters is not a valid solution:

function Test-Pipe {
[CmdletBinding()]
param(
   [Parameter(ValueFromPipelineByPropertyName)]
   $First,

   [Parameter(ValueFromPipelineByPropertyName,Mandatory)]
   $Last
)
process {
    if($PSBoundParameters.ContainsKey('First')) {
        Write-Host "$Last, $First"
    } else {
        Write-Host "$Last"
    }
}
}

$Both = [PSCustomObject]@{ First = "Joel"; Last = "Bennett" }
$One  = [PSCustomObject]@{ Last = "Hunt" }

# Here, both process blocks ContainsKey("First") = $true
$Both, $One | Test-Pipe
# Here, only the second one does:
$One, $Both | Test-Pipe

... but that's probably a bug.