Should Parameters Always Have Default Values? about powershellpracticeandstyle HOT 38 OPEN

@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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

@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?

• having some rare cases of $null default assignments where they are not really needed
• having no option to point novices to potentially undefined optional parameters through automated rules at all

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.

from powershellpracticeandstyle.

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

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

@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.

from powershellpracticeandstyle.

@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.

from powershellpracticeandstyle.

@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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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. :-)

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

what kind of comment is a "thumbs down"?

from powershellpracticeandstyle.

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 :-)

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

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 .

from powershellpracticeandstyle.

@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.

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

@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.

from powershellpracticeandstyle.

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

from powershellpracticeandstyle.

null checking is complicated in PowerShell

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

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

@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.

from powershellpracticeandstyle.

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).

from powershellpracticeandstyle.

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

from powershellpracticeandstyle.

@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).

from powershellpracticeandstyle.

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.

from powershellpracticeandstyle.

@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.

from powershellpracticeandstyle.

[DateTime]$parameter - will be null ❓❗

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

from powershellpracticeandstyle.

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
}

from powershellpracticeandstyle.