Need Some Feedback on My Script Please

This topic contains 6 replies, has 2 voices, and was last updated by Profile photo of Ron Ron 3 years ago.

  • Author
    Posts
  • #14199
    Profile photo of Ron
    Ron
    Participant

    The attached script (Check-ADGroupsExist.ps1.txt) I just finished and it would be great to get some feedback on it.

    Also, I was told by someone at work that if I use the Mandatory=$true, then there is no need to use the [ValidateNotNullorEmpty()]. I always thought that Mandatory just makes the parameter mandatory. Just because a parameter is mandatory does not mean that it has to contain a value, right? This is why the need to use the [ValidateNotNullorEmpty()], correct, or am I off my rocker?

    Does the use of Mandatory=$true tell PowerShell that the parameter MUST be used and that the parameter also MUST have a value?

    If this is the case, then why do I keep seeing both used all the time as in the example below:

    Param (
    [Parameter(Position=0,Mandatory=$True,HelpMessage="Enter a process name like lsass")]
    [ValidateNotNullorEmpty()]
    [string]$Name,
    [Parameter(Position=1)]
    [ValidateNotNullorEmpty()]
    [string]$Computername=$env:computername

    Thanks everyone

    -Ron

  • #14208
    Profile photo of Ron
    Ron
    Participant

    Great info Dave!!!

    Thanks again brother.

    You're pretty good at this PowerShell stuff – LOL.

    I guess you're right, that extra function does seem superfluous now that I am looking at it closer. I will go ahead and toss everything out and just keep the one input GUI function then.

    Dave, one more thing, is there a way for me to NOT use the text files that you see in the script to output groups to that do not exist in each domain, but instead use a variable or something to speed up the script process?

    What I am doing is telling the script to output to a file, the group that does not exist in a domain, and if the total number of that group equals the totoal number of domains in the forest, then that group does not exist at all in the Active Directory environment. It works great for me, but just wanted to know if I can somehow just do away with the files altogether.

    Thanks again for your help Dave. You and Don are teaching me quite a bit.

    I swear, I am going to sit down some time and actually read all the Manning PowerShell books I bought off of Amazon. I just have to try and find a good, quiet hiding place from my 3 year old to do that.

  • #14209
    Profile photo of Dave Wyatt
    Dave Wyatt
    Moderator

    Hmm, I hadn't looked that closely at what the Check-ADGroupsExist function was actually doing. It looks like the only thing you were using the text file for was to determine whether the group exists in any of your domains, and that can be accomplish with a simple boolean value. For example (code shortened for clarity):

    $groupExists = $false
    
    foreach ($domain in $domains)
    {
        $group = Get-ADGroup -Filter "Name -eq '$groupName'" -Server $domain
    
        if ($group)
        {
            $groupExists = $true
            
            # Output some information about $group in $domain
        }
    }
    
    if (-not $groupExists)
    {
        # Output some information about how the group didn't exist in any domain.
    }
    

    If you actually need to keep track of which domains have a group with the matching name and which ones do not, you can also do that but with some sort of collection. For example:

    $groupFound = @()
    $groupNotFound = @()
    
    foreach ($domain in $domains)
    {
        $group = Get-ADGroup -Filter "Name -eq '$groupName'" -Server $domain
    
        if ($group)
        {
            $groupFound += $domain
        }
        else
        {
            $groupNotFound += $domain
        }
    }
    
    # Do stuff with the contents of $groupFound and $groupNotFound
    

    Either way, there's no need to save data to a file on disk, unless you intended to review it later or something.

  • #14215
    Profile photo of Dave Wyatt
    Dave Wyatt
    Moderator

    if (-not $groupExists) is the same thing as if ($groupExists -eq $false), assuming that $groupExists is a boolean variable (which it is, in this case). I set it to $false before the loop started, and only set it to $true if at least one matching group was found in the domains. The "if" statement after the loop is a check on that flag to see if it's still set to $false, which indicates that the group was not found in any domain.

  • #14217
    Profile photo of Ron
    Ron
    Participant

    Interesting. Cool thanks again, Dave.

  • #14201
    Profile photo of Dave Wyatt
    Dave Wyatt
    Moderator

    [quote]Also, I was told by someone at work that if I use the Mandatory=$true, then there is no need to use the [ValidateNotNullorEmpty()].
    [/quote]

    Your coworker is correct. The Mandatory property automatically includes the same behavior as [ValidateNotNullOrEmpty()], by default. However, you can adjust this with the AllowNull, AllowEmptyString, and AllowEmptyCollection attributes (if you want to allow someone to pass those values to a mandatory parameter.)

    This is easy to test. All three of the example function calls will generate a ParameterBindingValidationException:

    function Test-Function
    {
        [CmdletBinding()]
        param (
            [Parameter(Mandatory = $true)]
            [string[]]
            $Parameter
        )
    
        $Parameter
    }
    
    Test-Function -Parameter $null
    Test-Function -Parameter ''
    Test-Function -Parameter @()
    

    As far as the attached script goes, here are my observations as I read it:

    • You appear to be using tab characters for indents in your code, which is unusual these days. Most code editors use spaces for indentation (with 4 spaces being the generally accepted indent size, though some people prefer 2 spaces or other values as well.) The advantage to using spaces is that the code will look the same no matter what editor is used to view it, even NotePad. Some of your indentation looks odd right now because of a mix of spaces and tabs (mainly around the comment-based help and param blocks of the Check-ADGroupsExist function.)
    • I'd move the two #requires statements to the beginning of the script file. Even though you've defined them inside a function, they still apply to the entire script or module in which they're found. You can't have, for example, one function with #requires -Version 2.0 and another with #requires -Version 3.0; you'll get errors when PowerShell gets to the second one.
    • This line appears to be missing an opening parenthesis:
      $ADDomains = Get-ADForest).Domains
    • You've defined the Check-ADGroupsExists function to take a single parameter (and a mandatory one, at that), but you're calling the function with no parameters, with no way for the person calling your script to specify that input. From what I can infer of your intentions by reading the rest of the script, it looks like you are missing a param block at the script level (which will be identical to the param block of the Check-ADGroupsExist function), at which point you could pass the user's value of $ADGroupName on to the function. Alternatively, since the entire script does nothing but call that one function, you could just move all of the code from Check-ADGroupsExists out into the script's scope, if you prefer. Below is a brief example of both approaches:
    # Script with parameters, code in the script scope
    
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        [string]
        $ParameterName
    )
    
    "Doing something with '$ParameterName'"
    
    # Script with parameters, passing the parameter on to a function that does the work
    
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        [string]
        $ParameterName
    )
    
    function Do-Something
    {
        [CmdletBinding()]
        param (
            [Parameter(Mandatory = $true)]
            [string]
            $ParameterName
        )
    
        "Doing something with '$ParameterName'"
    }
    
    # Passing the value of the $ParameterName parameter from the script's scope on to the function
    
    Do-Something -ParameterName $ParameterName
    
  • #14214
    Profile photo of Ron
    Ron
    Participant

    I'm confused here. Can you explain the following:

    if (-not $groupExists)
    {
    # Output some information about how the group didn't exist in any domain.
    }

    Isn't that saying "If the group is NOT false then do something"? But I want to do something when the groups IS false, or are my noob eyes not reading this correctly?

    Shouldn't this be the following:

    If ($groupExists)
    {
    # Output some information about how the group didn't exist in any domain.
    }

    Also, what does the "+-" mean in PowerShell here:

    $groupFound += $domain

    Thanks Dave

You must be logged in to reply to this topic.