Skip to main content
1 of 2
Matt
  • 2.6k
  • 15
  • 30

Your code and overall logic look good. In general the areas of improvement I see are

  • There are some things you are inconsistent about, e.g., function parameters
  • You are sending notification information down the pipeline.
  • Some of the parameter declaration could be improved.
  • I see that your are trying to make the code fit within a certain amount of characters on each line. While that is perfectly fine the are some features of PowerShell I could show that would improve code functionality and still give that same visual effect.

Param Block

Mandatory Parameters

In your first function you have the param $serviceName throw an error if it is not represent.

[string]$serviceName = $(throw "serviceName is required")

In another function you get the same thing but leveraging advanced parameters.

[Parameter(Mandatory=$true,ValueFromPipeline=$True)]
[string]$source,

So as you can see there is no reason for throw. The Mandatory flag set to true accomplishes the almost exact same thing. Calling the function without passing the parameter will make PowerShell ask for it.

cmdlet Get-Service at command pipeline position 1
Supply values for the following parameters:
serviceName: _ <-- Pretend that underscore is blinking

Perhaps this has occurred to you and you prefer the messages from throw. Setting the parameter will mitigate errors. If nothing else just take it as an FYI. Remeber that you would need to use a

Watch out for scope

The function Install-Service has a parameter defined $password with a default of $pass. While that code will still function is can be misleading as $pass is a variable defined in a parent scope. Consider making that one mandatory as well and just use $pass when you are calling the function.

Error Checking

You make several calls to WMI but there is not guarantee they are going to work. Consider using a try/catch block or -SilentlyContinue and validating the result. Examples of both are below. These are not production examples but samples to show functionality.

# Try/Catch Example
try{
    $result = Get-WmiObject -Class Win32_Volume -ComputerName doesnotexist
} catch {
    "There was and error: $($_.Exception)"
}

# SilentlyContinue Example
$result = Get-WmiObject -Class Win32_Volume -ComputerName doesnotexist -ErrorAction SilentlyContinue
if(!$result){"Something happened"}

Code Clarity

I refereed to this in a bullet above about mak[ing] the code fit within a certain amount of characters on each line. While playing with the backticks works it can be annoying when you have to make changes and forget to add them. Once place in Install-Service where you use them and it is not even required. PowerShell is forgiving for this:

$params = 
    $serviceName, 
    $displayName, 
    $physicalPath, 
    $serviceType, 
    $serviceErrorControl, 
    $startMode, 
    $interactWithDesktop, 
    $userName, 
    $password,
    $loadOrderGroup,
    $loadOrderGroupDepend,
    $dependencies

In other places you cannot get away with that. When you call Install-Service you use backticks again to get each parameter on its own line. That in itself is fine but I wanted to show you splatting. You pass a hashtable of parameter and value pairs and splat them to the cmdlet. Each one is on its own line and can be edited easily in place.

$params = @{
    ServiceName = $serviceName
    TargetServer = $targetServer
    DisplayName = "APP Test Syncronization Service"
    PhysicalPath = "C:\APP\APP.SyncService.exe"
    Username = $user
    Description = "Description"  
}

Install-Service @params

Function names

Get-Service and Start-Service are already cmdlet names. What you are doing is changing the command order precedence. Yours will get called first but you are using them for the same functionality the builtin cmdlets.

# Using the builtin
Get-Service -ComputerName $target -Name Spooler

I would opt for just using the built in ones. They return service ob

Be careful with your function output

Again, this is really wrong but something you need to be aware of. You are sending lots of status information down the pipeline. Basic example being

"Pausing to avoid potential temporary access denied"

That line is sent down the output stream. If you are going capture output from function, or use the pipeline like you configured on some of your parameters ValueFromPipeline=$True, you might get unexpected behavior. If this is truly information only then consider Write-host or a separate logging function.

Test-ServiceResult

The way you convert the error code to its friendly text could be shortened. If you have at least PowerShell v5 you can use the enum keyword. If not you could always add a type definition as well. To keep it simple I am just going to improve your error code array.

You can also use the -is operator to test the type of a variable.

if ($result -is [uint32]) {$retVal = $result} else {$retVal = $result.ReturnValue}

if ($retVal -eq 0) {return}

$errorCodes = "Success","Not Supported","Access Denied","Dependent Services Running",
    "Invalid Service Control","Service Cannot Accept Control","Service Not Active","Service Request Timeout",
    "Unknown Failure","Path Not Found","Service Already Running","Service Database Locked","Service Dependency Deleted",
    "Service Dependency Failure","Service Disabled","Service Logon Failure","Service Marked for Deletion","Service No Thread",
    "Status Circular Dependency","Status Duplicate Name","Status Invalid Name","Status Invalid Parameter","Status Invalid Service Account",
    "Status Service Exists","Service Already Paused"

$msg = ("{0} failed with code {1}:{2}" -f $operation, $retVal, $errorCodes[$retVal]) 
Matt
  • 2.6k
  • 15
  • 30