Function with multiple parameters

PowerShell Help
1

I have a function that works if I choose a single parameter. If I choose multiple params like -Expired and -Nologon it just runs one of them. Is there a better way to write this to support multiple params rather than a bunch of if statements?

function Get-InactiveUsers {

[CmdletBinding()]
param(
# [Parameter(Mandatory=$true)]
[String]$Inactive = 90,
[String]$Disable = 'false',
[String]$Move = 'false',
[String]$Expired = 'false',
[String]$NoLogon = 'false',
[String]$DisabledAccounts = 'false'

)

#Check value of $disable and run if statement
elseif ($Disable -eq 'true') {

Write-Host "Disable was detected"

}

#Check if $move is set to true
elseif ($Move -eq 'true') {

write-host "Move OU was detected"

}

#Check if expired is set to true
elseif ($Expired -eq 'true') {

write-host "Expired accounts was selected"

}

#Check if NoLogon is set to true
elseif ($NoLogon -eq 'true') {

write-host "NoLogon was selected"

}

#Check if DisabledAccounts is set to trye
elseif ($DisabledAccounts -eq 'true') {

write-host "Finding all disabled accounts"

}

else {

write-host "Runing default actions"

}

}

 

2

First off, it appears you have the ‘if’ statement beginning with ‘else if’…

Here are my suggestions:

  • Parameters should reflect a little more intention
  • Use more appropriate data types
  • Function name should be singular
  • Depending on what you're trying to accomplish, you may want to consider using ParameterSets
(Switch parameters default to false) 
function Get-InactiveUser {
    [CmdletBinding()]
    param(
        [Parameter()]
        [uint32]$Inactive = 90,

    [Parameter()]
    [switch]$Disable,

    [Parameter()]
    [switch]$Move

    [Parameter()]
    [switch]$Expired

    [Parameter()]
    [switch]$NoLogon

    [Parameter()]
    [switch]$DisabledAccounts
)

if ($Disable.IsPresent) {
    Write-Host "Disable was detected"
}
elseif ($Move.IsPresent) {
    Write-Host "Move OU was detected"
}
elseif ($Expired.IsPresent) {
    Write-Host "Expired accounts was selected"
}
elseif ($NoLogon.IsPresent) {
    Write-Host "NoLogon was selected"
}
elseif ($DisabledAccounts.IsPresent) {
    Write-Host "Finding all disabled accounts"
}
else {
    Write-Host "Running default actions"
}



}

Example to get all expired users:

Get-InactiveUser -Expired
3

That still only allows one parameter to run then it stops.

Get-InactiveUser -Disable -Move

The above just runs the disables parament and it stops. I need it to run each paramount that is set.

 

 

4

Don’t use elseif, just have separate if stacks. That’ll solve the immediate problem.

But in my mind I’d think this is much more easily accomplished by rolling all the actions into a single parameter and using a switch() statement instead. For example:

That said… I would highly recommend not taking any action with a Get- function. By definition, a Get- verb implies that no environment-impacting actions are taken. Breaking that convention means that anyone else who comes into contact with your code won’t know what to expect, and that can get a bit risky, especially as you’re not implementing ShouldProcess support here.

5

There are several syntax error in your post, in that param block. There are not any commas after

    param
    (
        [Parameter()]
        [uint32]$Inactive = 90,

        [Parameter()]
        [switch]$Disable,

        [Parameter()]
        [switch]$Move

        [Parameter()]
        [switch]$Expired

        [Parameter()]
        [switch]$NoLogon

        [Parameter()]
        [switch]$DisabledAccounts
    )

Should be this…

    param
    (
        [Parameter()]
        [uint32]$Inactive = 90,

        [Parameter()]
        [switch]$Disable,

        [Parameter()]
        [switch]$Move,

        [Parameter()]
        [switch]$Expired,

        [Parameter()]
        [switch]$NoLogon,

        [Parameter()]
        [switch]$DisabledAccounts
    )

… and the use case you are after does not fit this.

Consider this…

1 - First change the function name. Since you are setting states on the object not just getting. Suggestion:

# using one of the approved verbs

Get-Verb | Sort-Object -Property Verb

Verb        Group         
----        -----         
Add         Common        
Resume      Lifecycle     
Restart     Lifecycle     
Request     Lifecycle  
…

Request-InactiveUser

2 - Second is you are passing multiple switches, then this if/elseif is not a thing as the first true response wins, by design, that is how conditional statements work.

3 - Some of the combinations, you appear to want to be able to use, is really a logic error as well.

Request-InactiveUser
{
    [CmdletBinding()]
    [Alias('siu')]

    param
    (

    # Just by the name,, this is filter condition
        [Parameter()]
        [uint32]$Inactive = 90,

    # Just by the name, this is an action
    
        [Parameter()]
        [switch]$Disable,

    # Just by the name, this is an action

        [Parameter()]
        [switch]$Move,


    ### Which means maybe, it should be one or the other, not both.


    # Just by the name, this is an state
        [Parameter()]
        [switch]$Expired,

    # Just by the name, this is a state

        [Parameter()]
        [switch]$NoLogon,

    # Just by the name, this is filter condition
        [Parameter()]
        [switch]$DisabledAccounts
    )
}

You need to think this out more. Meaning, what condition(s)/filter(s) to you want to check, and what combinations are really valid based on actions or end state to be determined. So, before write any real code, Pseudo this out based on use case you imagine., then construct your code to meet each use case. For example:

Use case 1

Check for inactive user within a given set of days

Use case 2

Check for inactive user within a given set of days, that are also disabled accounts - which is really a misnomer, if they are disabled then of course they are inactive.

Use case 3

Check for inactive user account disabled in a target date range.

Use case 4

Check for users who have not logged on in a target date range. Nologon

Use case 5

Check for users who have not logged on in a target date range, disable and potentially move the account

… etc.

So, all-in-all, when you do this evaluation, this may be should not be all in one function.

6

+1 postanote, and thanks for pointing out my oversights (one of those nights). And yes, the combination of switches can be a problem if they’re not in line with parametersets of cmdlets that will do the work, hence the importance of fully knowing the requirements before coding.