Peer Review: Advanced Function

Welcome Forums General PowerShell Q&A Peer Review: Advanced Function

Viewing 9 reply threads
  • Author
    Posts
    • #42262
      Participant
      Topics: 7
      Replies: 14
      Points: 22
      Rank: Member

      Hi All,

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

      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.

    • #42268
      Participant
      Topics: 18
      Replies: 573
      Points: 32
      Rank: Member

      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
        Participant
        Topics: 7
        Replies: 14
        Points: 22
        Rank: Member

        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
      Participant
      Topics: 0
      Replies: 669
      Points: 0
      Rank: Member

      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
      Participant
      Topics: 21
      Replies: 55
      Points: 3
      Rank: Member

      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
      Participant
      Topics: 7
      Replies: 14
      Points: 22
      Rank: Member

      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
      Participant
      Topics: 18
      Replies: 573
      Points: 32
      Rank: Member

      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
      Participant
      Topics: 2
      Replies: 376
      Points: 0
      Rank: Member

      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

    • #42501
      Participant
      Topics: 6
      Replies: 658
      Points: 47
      Rank: Member

      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
      Participant
      Topics: 2
      Replies: 376
      Points: 0
      Rank: Member

      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
      Participant
      Topics: 2
      Replies: 376
      Points: 0
      Rank: Member

      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 ...
        ...
      }
      
Viewing 9 reply threads
  • The topic ‘Peer Review: Advanced Function’ is closed to new replies.