Feedback

PowerShell Help
1

I was required to find all the devices attached to an SCCM application and get the user affinity as well.
It works, and i’m quite proud of what i’ve done here but would really value feedback on improvements and areas to improve on.

Many thanks everyone,


Function Get-DevicesDeployedToApplication { #Find what devices are assigned to an application
        
        [CmdletBinding()]
       
       Param (

        [Parameter(Mandatory=$true, 
                   ValueFromPipeline=$true,
                   Position=1)]
        [string]$Application =" ",
        
        #set log file. If left default is set to "c:\temp\DevicesDeployedToApplication.csv"
        [string]$LogFile = "c:\temp\DevicesDeployedToApplication.csv"

            )


#set Get-Ciminstance values to be queried
$App = @{
Namespace = 'ROOT\SMS\site_AAC' 
ClassName = 'SMS_G_System_DEVICE_INSTALLEDAPPLICATIONS'
Filter = "Name like '$Application'"
}

#Set message while data is being collated
Write-Host -ForegroundColor Green "Collating how many devices are in $Application`nLog file location is $LogFile`nPlease Wait...."
 
#Running query to find application ResourceID to be passed to find devices paired to the ResourceID
$Output = (Get-CimInstance @App).ResourceID |  
    foreach { Get-CimInstance -Namespace 'ROOT\SMS\site_AAC' -Query "SELECT * FROM SMS_G_System_DEVICE_INFO WHERE ResourceID Like $_ " | 
              #Select the Device Model, IMEI number and the phone number (Only last 4 digits shown)
              select DeviceName, Model, @{Name="Device_Affinity_User";Expression={ (Get-CMUserDeviceAffinity -DeviceName $_.DeviceName).UniqueUserName }}
            }

    #Output data to CSV file
    $Output | Export-Csv -Path $LogFile -NoTypeInformation -Delimiter " "
    
    
        #Add count to $logfile
        Add-Content $LogFile "`nNumber of devices are $($Output.Count)"  
        
        #display the number of devices in Application
        write-host -ForegroundColor Green "`nThe number of devices in the application $application is $($output.count)"              

#done
 }
2

I’d say it’s rather high quality PowerShell. Your code would be even better when properly indented though, see GitHub - PoshCode/PowerShellPracticeAndStyle: The Unofficial PowerShell Best Practices and Style Guide for guidance.

One other small issue, assigning your parameter “Application” a default value of " " is not really useful, since this parameter is mandatory anyway. Just leave it without a value.

3

My chief complaint is that you’ve included the Export-Csv cmdlet inside your function. You’d be better to write your functions so that the object it produces, can be piped to Export-Csv if desired by your function’s user, such as: Get-DevicesDeployedToApplication | Export-Csv -Path c:\devices.csv -NoTypeInformation. That said, if you’re supplying this function to someone that isn’t aware of the pipeline and Export-Csv, then I can understand why you might wrap this cmdlet.

4

Thank you both for your comments, i really do appreciate the feedback as keeps me moving in the right direction.
Daniel, your comment made my day. The amount of effort i’ve put in the past year, reading books, articles and forums , that comment meant alot.
I think PowerShell is amazing and i simply can’t get enough of it.

Again, thanks Tommy and Daniel for taking the time to comment.

5

I’ve had a quick read and attempted to amend my formatting…

Function Get-DevicesDeployedToApplication { #Find what devices are assigned to an application
    [CmdletBinding()]  
    Param(
        [Parameter(Mandatory=$true, 
                   ValueFromPipeline=$true,
                   Position=1)]
       
        [string]$Application,
        
        #set log file. If left default is set to "c:\temp\DevicesDeployedToApplication.csv"
        [string]$LogFile = "c:\temp\DevicesDeployedToApplication.csv"
    )
    Process {

#set Get-Ciminstance values to be queried
    $App = @{
        Namespace = 'ROOT\SMS\site_AAC' 
        ClassName = 'SMS_G_System_DEVICE_INSTALLEDAPPLICATIONS'
        Filter = "Name like '$Application'"
    }

    #Set message while data is being collated
    Write-Host -ForegroundColor Green "Collating how many devices are in $Application`nLog file location is $LogFile`nPlease Wait...."
 
    #Running query to find application ResourceID to be passed to find devices paired to the ResourceID
    $Output = (Get-CimInstance @App).ResourceID |  
        foreach { Get-CimInstance -Namespace 'ROOT\SMS\site_AAC' -Query "SELECT * FROM SMS_G_System_DEVICE_INFO WHERE ResourceID Like $_ " | 
            #Select the DeviceName, model of phone and primary user of device
             select DeviceName, Model, @{Name="Device_Affinity_User";Expression={ (Get-CMUserDeviceAffinity -DeviceName $_.DeviceName).UniqueUserName }}
        }

   #Output data to CSV file
   $Output | Export-Csv -Path $LogFile -NoTypeInformation -Delimiter " "
    
    
   #Add count to $logfile
   Add-Content $LogFile "`nNumber of devices are $($Output.Count)"  
        
   #display the number of devices in Application
   write-host -ForegroundColor Green "`nThe number of devices in the application $application is $($output.count)"              
    }

#done

}

On the right lines ?

6

Way better! I’d have indented the code in the Process scriptblock as well though :slight_smile: