Feedback

This topic contains 5 replies, has 3 voices, and was last updated by Profile photo of Daniël Daniël 1 year ago.

  • Author
    Posts
  • #33269
    Profile photo of Graham Beer
    Graham Beer
    Participant

    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
     }
    
    
  • #33276
    Profile photo of Daniël
    Daniël
    Participant

    I'd say it's rather high quality PowerShell. Your code would be even better when properly indented though, see https://github.com/PoshCode/PowerShellPracticeAndStyle 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.

  • #33278
    Profile photo of tommymaynard
    tommymaynard
    Member

    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.

  • #33279
    Profile photo of Graham Beer
    Graham Beer
    Participant

    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.

  • #33281
    Profile photo of Graham Beer
    Graham Beer
    Participant

    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 ?

  • #33309
    Profile photo of Daniël
    Daniël
    Participant

    Way better! I'd have indented the code in the Process scriptblock as well though 🙂

You must be logged in to reply to this topic.