Peer Review: Advanced Function

Tagged: 

This topic contains 10 replies, has 6 voices, and was last updated by Profile photo of Max Kozlov Max Kozlov 6 months ago.

  • Author
    Posts
  • #42262
    Profile photo of Nigel Tatschner
    Nigel Tatschner
    Participant

    Hi All,

    I was hoping any of you could take a look at my advanced function, I was hoping for some good criticism.

    the function checks in the system in pingable, is listening for RDP connections and and gets the up time.

    While these things are easy to gather I took a while to get this to happen as background jobs. I figured it out but was wondering if you had any thoughts on on how it could be done better.

  • #42268
    Profile photo of Dan Potter
    Dan Potter
    Participant

    get rid of the redundant code. Put your custom object in a function and supply computername as an argument.

    Your datetime conversion doesn't have to be that complicated.

    $Boot = Get-WmiObject Win32_OperatingSystem
    $uptime = $Boot.ConvertToDateTime($Boot.LastBootupTime)

    • #42298
      Profile photo of Nigel Tatschner
      Nigel Tatschner
      Participant

      Good shout on the code cleanup, i'll do that.
      And the date time would definitely be easier, I was over thinking it and making it easier to read for the end user in our scenario (Patching)

  • #42309
    Profile photo of Richard Siddaway
    Richard Siddaway
    Moderator

    use $env:COMPUTERNAME rather than localhost for default computer name. I've seen issues with local host in the past

    Your parameter block would be easier to read with a blank line between parameters

    Help messages on parameters only work if the parameter is mandatory

    Need an -ErrorAction Stop on the Import-Module for the try – catch to work

    $computername parameter isn't defined as an array so will only have a count of 1. If you want it to be an array need to define the parameter as
    [String[]]$ComputerName

    Use full parameter names -ErrorAction not -ea

    If you use Get-CimInstance rather than Get-WmiObject the dates will be in a usable format without any need to convert

    Create properties for your object in a hashtable and then use New-object – lots less typing than multiple Add-Members

    If you create the object before doing the WMI query you can reduce code – in fact create it near the top of the code than fill in the values – much less coding

    If you make the computername parameter an array you can cut your code in half as the singleton case is dealt with as part of the array processing

    You can reduce the code to a single foreach loop in the PROCESS block if you pass the values in computername or from the file to a $computers variable

    Overall you can cut this code down by a large percentage if you consolidate the code blocks – it'll then be much easier to update and expand

  • #42311
    Profile photo of Tony Pagliaro
    Tony Pagliaro
    Participant

    ComputerName param:
    Why not use "Name" instead, and add ComputerName and __SERVER as aliases? This will allow it to more easily pipe with wmio and Adcomp gets.

  • #42428
    Profile photo of Nigel Tatschner
    Nigel Tatschner
    Participant

    Thanks everyone.

    I've made the changes as suggested.
    Both old and new functions are still on the page.
    I'm having issues with the order of the output object.
    I made the hashtable [Ordered] but I think as soon as I changed the object it seems to loose the order.

    Any ideas?

  • #42436
    Profile photo of Dan Potter
    Dan Potter
    Participant

    I think you could get this down to about 10 lines..You are only doing two things. I have a full gui that does this in more in less than 200 lines.

     photo Untitled.png

  • #42438
    Profile photo of Max Kozlov
    Max Kozlov
    Participant

    If you #requires -version 3 why you work with hashtables and not use just

    $ObjectProps =[PSCustomObject] [Ordered]@{
    					"ComputerName" = "";
    					"LastBootUpTime" = "Could Not Query";
    					"Ping" = "No Responce";
    					"RDP" = "Not Listening";
    					"ObjectCount" = ""
    				}
    

    ?
    and now you create job even for one computer...
    it's a great waste of time/resources
    may be it's time to try
    https://github.com/proxb/PoshRSJob ?

    btw, I don't know why you need ObjectCount, but if you use it, yiu can generate it outside of a job in one place or while creating your obj and not copypasting it in every code branch

    • This reply was modified 6 months ago by Profile photo of Max Kozlov Max Kozlov.
  • #42501
    Profile photo of Curtis Smith
    Curtis Smith
    Participant

    1) This may be picky, but your parameter set naming does not really represent what is happening there. Singular represents commandline input, and Multiple represents file input. In both cases, however, multiple servers to process against are accepted.

    Singular accepts an array of strings which can be many server names.
    Multiple accepts a text file input that can be many server names.

    2) In your Importing needed modules section. It may be worth while to add the actual error message that generated the stop action. It will be helpful in determining which module had the issue.

    		Catch
    		{
    			$ErrorMessage = $_.Exception.Message
    			Write-Warning "Module NetTCPIP or CimCmdlets not installed: $ErrorMessage"
    			Break
    		}
    

    3) Again this one is a little picky, but this loop runs logically opposite from the verbiage used. When $AddJobs is true, it loops and does not proceed to add any jobs, When $AddJobs is false, it breaks the loop and proceeds with adding a job.

    			do
    			{
    				$LimitJobCount = @(Get-Job | Where-Object State -eq 'Running')
    				if ($LimitJobCount.Count -gt $ConcurrentJobs)
    				{
    					$AddJobs = $true
    				}
    				else
    				{
    					$AddJobs = $false
    				}
    			}
    			while ($AddJobs)
    

    Using until and less than makes the verbiage match the logic

    			do
    			{
    				$LimitJobCount = @(Get-Job | Where-Object State -eq 'Running')
    				if ($LimitJobCount.Count -lt $ConcurrentJobs)
    				{
    					$AddJobs = $true
    				}
    				else
    				{
    					$AddJobs = $false
    				}
    			}
    			until ($AddJobs)
    

    4) I may also be wise to a some down time to the above Do loop using start-sleep so that the processor is not constantly churning while waiting to start another job.

  • #42522
    Profile photo of Max Kozlov
    Max Kozlov
    Participant

    May be I'm repeating, but all job related problems Curtis mentioned already solved with PoshRSJob 🙂
    look at usage example and note -Trottle parameter

    1..25 | Start-RSJob -Batch pingtest -ScriptBlock { "{0}: {1}" -f $_, [DateTime]::Now; Start-Sleep -Seconds 5 } -Throttle 5
    
    Get-RSJob -Batch pingtest | Wait-RSJob | Receive-RSJob
    
    Get-RSJob -Batch pingtest | Remove-RSJob
    

    Inside of scriptblock there can be your code and instead of 1..25 can be your prepared input objects

    code schema:

    #requires -Version 3 -Module modulename1, modulename2 #your modules here, not need module checking
    # but can't remember is v3.0 support it or not
    begin {
    # some checking, test function/scriptblock declaration can be here
    }
    process{
      if (parameterset -eq 'file') {
        $computername = gc $file # there you can do txt/csv conversions/parsing
      }
      foreach ($comp in $computername) {
        $null = # not need to save Start-RSJob output if use -batch name
          [PSCustomObject]@{
             ComputerName = $comp
             Object = 'Here'
             Another = '...'
         } | Start-RSJob -batch #  complete code here ....
      }
    }
    end {
      Get-RSJob -batch | Wait-RSJob | Receive-RSJob
      Get-RSJob -batch | Remove-RSJob
    }
    

    I think its much shorter and cleaner

  • #42524
    Profile photo of Max Kozlov
    Max Kozlov
    Participant

    Should say:
    My code schema above have one flaw: -Throttle parameter for Start-RSJob work only for current pipeline input
    i.e 1..25 | Start-RSJob -throttle 5 can work as expected
    and
    1..25 | foreach { $_ | Start-RSJob -throttle 5 } is not
    thus for full throttling support code must make its own queue checking like yours or collect all input objects and send it to start-rsjob later #more memory pressure!
    something like that

    begin {
       $all = {}.Invoke() # use collection instead of array
    }
    process{
      if ( parameterset -eq 'file') {
        gc $file | foreach { $all.Add($_)  }
      }
      else {
        $computername | foreach { $all.Add($_) }
      }
    }
    end{
      $null =  $all | foreach { [pscustomobject]@{ }  } | start-rsjob
      get-rsjob ...
      ...
    }
    
    • This reply was modified 6 months ago by Profile photo of Max Kozlov Max Kozlov.

You must be logged in to reply to this topic.