Peer Review: Advanced Function

Hi All,

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

http://tatux.co.uk/2016/06/07/check-ntupstatus/

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.

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)

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)

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

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.

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?

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

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
GitHub - proxb/PoshRSJob: Provides an alternative to PSjobs with greater performance and less overhead to run commands in the background, freeing up the console and allowing throttling on the jobs. ?

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

  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.

  1. 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
		}
  1. 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)
  1. 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.

May be I’m repeating, but all job related problems Curtis mentioned already solved with PoshRSJob :slight_smile:
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

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 ...
  ...
}