Redundancy of Code in Advanced Function

This topic contains 4 replies, has 2 voices, and was last updated by Profile photo of Mike F Robbins Mike F Robbins 3 years ago.

  • Author
    Posts
  • #16238
    Profile photo of Mike F Robbins
    Mike F Robbins
    Participant

    This function is something I wrote a while back and now that I'm going back through it, the following line appears to be redundant since the script wouldn't have made it to this point if $VM didn't already contain the value it's being set to again:

    $VM = $VMs | Where-Object Name -eq $VM
    

    The goal of this function is to prevent a Hyper-V VM from being started that's connected to an external network since the domain controller in my test lab environment is running the DHCP server role and accidentally connecting it to an external network would result in chaos to say the least.

    #Requires -Version 3.0
    #Requires -Modules Hyper-V
    function Start-MrVM {
    
    
    
        [CmdletBinding()]
        param (
            
            [Parameter(Mandatory,
                       ValueFromPipeline,
                       ValueFromPipelineByPropertyName)]
            [Alias('Name')]
            [string[]]$VMName
    
        )
    
        BEGIN {
            try {
                $VMs = Get-VM
                $VMSwitches = Get-VMSwitch -SwitchType Internal, Private | Select-Object -ExpandProperty Name
            }
            catch {
                Write-Warning -Message "An error has occured.  Error details: $_.Exception.Message"
            }
        }
    
        PROCESS {
            foreach ($VM in $VMName) {
    
                if ($VMs.Name -contains $VM) {
                    
                    $VM = $VMs | Where-Object Name -eq $VM
    
                    if ($VM.State -ne 'Running') {
    
                        $VMSwitch = $VM | Get-VMNetworkAdapter | Select-Object -ExpandProperty SwitchName
    
                        if ($VMSwitches -contains $VMSwitch -or (-not($VMSwitch))) {
                
                            Write-Verbose -Message "Starting VM: $($VM.Name)"
                    
                            try {
                                Start-VM -Name $VM.VMName
                            }
                            catch {
                                Write-Warning -Message "An error has occured.  Error details: $_.Exception.Message"
                            }
                    
                        }
                        else {
                            Write-Warning -Message "VM: $($VM.Name) was not started because it's network is set to External!"
                        }
                    
                    }
                    else {
                        Write-Warning -Message "VM: $($VM.Name) is Already Running!"
                    }
                }
                else {                
                    Write-Warning -Message "VM: $VM is invalid or was not found!"
                }
            }
        }
    }
    

    I also wanted an excuse to test a longer sample of code with the new code formatter on this forum.

  • #16239
    Profile photo of Mike F Robbins
    Mike F Robbins
    Participant

    One thing I noticed and I'm not sure if this is specific to the new code formatter, but it appears that anything in less than and greater than signs is stripped out when attempting to post it in the forums. I posted comment based help with the function (approximately 34 lines of it) and it was stripped out being replaced by a couple of blank lines after hitting submit.

  • #16241
    Profile photo of Mike F Robbins
    Mike F Robbins
    Participant

    By the way, that line of code that I previously mentioned isn't redundant. Do you know and understand why?

  • #16242
    Profile photo of Dave Wyatt
    Dave Wyatt
    Moderator

    Before that line, $VM is a string. Afterward, it's one of the objects returned from Get-VM.

    Generally speaking, it's confusing to reuse a variable in that fashion, and I try to avoid it. I would prefer something like "$vmObject = $VMs | Where-Object Name -eq $VM", where it's more clear that $VM and $vmObject are different thingies. (A very technical term, I know.)

  • #16245
    Profile photo of Mike F Robbins
    Mike F Robbins
    Participant

    I definitely consider that to be great feedback since I wrote the code myself and I was confused after not looking at it for weeks or possibly months so I can agree that using a different variable name would indeed make it easier to understand. Thanks for taking the time to look at this.

You must be logged in to reply to this topic.