10
\$\begingroup\$

Previously in the Settings JAVA_HOME with batch post, I had created a script in bash to change my JAVA_HOME env variable. Since then, I have been trying to use more and more PowerShell so I recreated the script with PowerShell.

The script has several options that provide different functionalities:

  • e or echo : output the current value of JAVA_HOME.
  • s or set : option to change the current value of JAVA_HOME. You must provide the key you want to change to.
  • a or add : add a key value pair to the store file that you can later on changed to.
  • l or list : list the current value in the file.
  • r or remove : remove the key-value pair from the store file.

This is still a script that I use for my personal use so there are still things that are not robust. You can add a key with an empty value or an invalid one and the script will not stop you. I've tested every option and all the "happy paths" are working.

This is my first script in PowerShell, so there were a lot of googling and patching all around the script to make it work.


<# Name: javaenv Date: 12 September 2016 Author: Marc-Andre Girard Purpose: Modify the JAVA_HOME env variable based on Java installations on the computer. #> Param( [Parameter(Mandatory=$True)] [string]$action, [string]$key, [string]$value ) <# Show the JAVA_HOME variable to the screen #> function Output() { Get-ChildItem Env:JAVA_HOME } <# Set the JAVA_HOME variable in the Process scope to only influence the current process. #> function SetJavaHome([string]$value_to_set) { [Environment]::SetEnvironmentVariable("JAVA_HOME", $value_to_set, "Process") } <# Get a value from the version file, based on a key provided. #> function GetValueFromFile([string]$key_in_file) { $javaVersions = GetVersionsFromFile if($javaVersions.ContainsKey($key_in_file)) { $javaVersions.$key_in_file } else { Write-Host "The key does not exit in this file" exit } } <# Add a version to the version file, based on a key-value provided. #> function AddVersionToFile([string]$key_in_file, [string]$value_to_add) { $javaVersions = GetVersionsFromFile if (!$javaVersions.ContainsKey($key_in_file)) { Add-Content $dir/java_versions.store "$key_in_file=$value_to_add" } else { Write-Host "Key already exist in the file" exit } Write-Host "$key_in_file=$value_to_add" } function GetVersionsFromFile() { ConvertFrom-StringData (Get-Content $dir/java_versions.store | Out-String) } function RemoveVersionFromFile([string]$key_to_remove) { Write-Host $key_to_remove $javaVersions = GetVersionsFromFile $javaVersions.remove($key_to_remove) $content = HashConvertToString $javaVersions $content | Out-File $dir/java_versions.store } function HashConvertToString($ht) { $content_file = "" foreach($pair in $ht.GetEnumerator()) { $content_file += "{0}={1}`n" -f $($pair.key),$($pair.Value) } $content_file } Set-StrictMode -Version Latest $scriptpath = $MyInvocation.MyCommand.Path $dir = Split-Path $scriptpath if($action -eq "e" -or $action -eq "echo") { Output } if($action -eq "s" -or $action -eq "set") { if(![string]::IsNullOrEmpty($key)) { $value_from_file = GetValueFromFile($key) SetJavaHome($value_from_file) Output } else { Write-Host "You must provide the key to set with -key or provide the second argument" } } if($action -eq "a" -or $action -eq "add") { AddVersionToFile $key $value } if($action -eq "l" -or $action -eq "list") { GetVersionsFromFile } if($action -eq "r" -or $action -eq "remove") { RemoveVersionFromFile $key } 

This is an example of what a store file should look like :

JAVA6=C:/Program Files/Java/jdk1.6.0_43 JAVA7=C:/Program Files/Java/jdk1.7.0_79 JAVA8=C:/Program Files/Java/jdk1.8.0_73 
\$\endgroup\$

    1 Answer 1

    5
    +100
    \$\begingroup\$

    For a beginner script there are some There might be more to cover but there are a few things here that could improve the script.

    Variable and function naming

    In general functions and cmdlets should follow the Verb-Noun naming convention. You have the verb and noun part down but the hyphens are not there. Not a must but I would expect you to see those on other scripts.

    Variables generally follow camelCase (with first work in lower) or PascalCase. I use camelCase myself. In either case PowerShell were prefer if you use one of those over the joined by underscore method.

    Parameter in functions / cmdlets you will always see use PascalCase.

    PowerShell script help

    You have a multiline command block that contains some information about the script and its creation. I used the same sort of thing for my old VBS scripts. PowerShell has a robust help system you could(should?) be using for this information.. Stealing the simple example from TechNet

    <# .SYNOPSIS The synopsis goes here. This can be one line, or many. .DESCRIPTION The description is usually a longer, more detailed explanation of what the script or function does. Take as many lines as you need. .PARAMETER computername Here, the dotted keyword is followed by a single parameter name. Don't precede that with a hyphen. The following lines describe the purpose of the parameter: .PARAMETER filePath Provide a PARAMETER section for each parameter that your script or function accepts. .EXAMPLE There's no need to number your examples. .EXAMPLE PowerShell will number them for you when it displays your help text to a user. #> 

    This way you can give more explanation to what your script is doing to the users that might be running it. (I realize this is a personal script). Also is an easy way to explain how each parameter works and give some examples.

    The power this gives you is using Get-Help in the shell against your script.

    Single line comments

    # You can also just use the number sign for single line comments instead of <##> 

    Too many ifs

    If you find yourself with several if blocks there is probably a better way. You should be using a switch for this.

    switch ($action){ {$_ -eq "e" -or $_ -eq "echo"}{ "Do Something." } {"s","set" -contains $_}{ "Do Something Else." } default { "You typed '$_' and it is has no associated action" } } 

    The above covers a couple of ways that you can work with multiple values of $action that result in the same code being executed. There is more you can do with switches so I encourage to read up on them. You can use regex as well for your cases but I opted not to include it here for brevity.

    Remember that

    If the value matches multiple conditions, the action for each condition is executed. To change this behavior, use the Break or Continue keywords.

    So it would not be uncommon to see break being used in examples on the web.

    Advanced function parameters

    This is another large topic that is covered in about_functions_advanced_parameters but one example to call out would be your $action parameter. You only support 10 values and the script will do nothing if the one of those values was not entered.

    Param ( [parameter(Mandatory=$true)] [ValidateSet("e", "echo","s","set","a","add","l","list","r","remove")] [String]$Action # Continue with other params ) 

    So now if you don't provide one of those values for $action PowerShell will throw an error stating just that.

    Cannot validate argument on parameter 'Action'. The argument "bagel" does not belong to the set "e,echo,s,set,a,add,l,list,r,remove" specified by the ValidateSet attribute. Supply an argument that is in the set and then try the command again.

    Parameter Sets

    A continuation of advanced functions... Your $action is looking one of a set of mutually exclusive choices. Why not convert those actions into parameter switches? Then, using parameter sets, you can define their use with the $key$value parameters. Ensuring that nothing will be called or used if the combination was not meant to be. Consider the following (ignore the name. I had to call it something) function:

    function Set-JavaHome{ [CmdletBinding(DefaultParametersetName="List")] Param ( [Parameter(ParameterSetName="Echo")] [switch]$Echo, [Parameter(ParameterSetName="Set")] [switch]$Set, [Parameter(ParameterSetName="Add")] [switch]$Add, [Parameter(ParameterSetName="List")] [switch]$List, [Parameter(ParameterSetName="Remove")] [switch]$Remove, [Parameter(ParameterSetName="Add",Mandatory=$true)] [Parameter(ParameterSetName="Remove",Mandatory=$true)] [Parameter(ParameterSetName="Set",Mandatory=$true)] [string]$Key, [Parameter(ParameterSetName="Add",Mandatory=$true)] [string]$Value ) switch($pscmdlet.ParameterSetName){ "Echo" {"Echo echo echo...."} "List" {"Show me the list!"} "Remove" {"Removing $key"} "Set"{"Setting the value to $key"} "Add"{"The new key:-$key- will be set to -$value-"} } } 

    This gives better control over what is used when calling the function (or script). Running Get-Command against this function will help explain.

    PS C:\Windows\system32> Get-Command Set-JavaHome -Syntax Set-JavaHome [-List] [<CommonParameters>] Set-JavaHome [-Echo] [<CommonParameters>] Set-JavaHome -Key <string> [-Set] [<CommonParameters>] Set-JavaHome -Key <string> -Value <string> [-Add] [<CommonParameters>] Set-JavaHome -Key <string> [-Remove] [<CommonParameters>] 

    This shows the acceptable ways to call this function. There is no need to check, for example, if $key has been populated with calling -Add since the cmdlet would throw an error or prompt the user otherwise. $key is now mandatory for the parameter sets it is assigned to. PowerShell now pro

    PS C:\Windows\system32> Set-JavaHome -Add -Value "C:\bagel" cmdlet Set-JavaHome at command pipeline position 1 Supply values for the following parameters: Key: 

    This is not a feature of parameter sets but since we have each "action" as its own parameter we also can take advantage of calling the parameters by their shortest unambiguous string. In your care that would accept the first letter.

    Set-JavaHome -l 

    Depending on your function this could collide with common cmdlet parameters () e.g. verbose, whatif. In this case I do not think there is any worry.

    Test-Path

    You are adding paths to your environment. It might be worth testing them before the get used. A simple code block might help prevent hair pulling down the line

    If (Test-Path $value){ "Valid Path" } else { "Not Valid Path" } 

    Hopefully that will give you a little something to work with.

    \$\endgroup\$
    5
    • \$\begingroup\$Thanks for the review, I've made the change, I just need to rename the functions and argument and I should be good. The only thing is now on my work computer it adds Chinese character, but I'll fix this later.\$\endgroup\$CommentedJan 20, 2017 at 18:30
    • 1
      \$\begingroup\$@Marc-Andre Can't believe I forgot to add a section about parameter sets. It would be perfect here. Should be able to add that tomorrow.\$\endgroup\$
      – Matt
      CommentedJan 23, 2017 at 4:02
    • \$\begingroup\$If it pairs parameters together, I'm sure it will be a fine addition to this answer!\$\endgroup\$CommentedJan 23, 2017 at 19:42
    • 1
      \$\begingroup\$@Marc-Andre Check out some parameter set example. Might be overkill for your script but it works very well here I think.\$\endgroup\$
      – Matt
      CommentedJan 24, 2017 at 2:09
    • \$\begingroup\$I think it will be perfect for the script, it will make it's use the way I intended to\$\endgroup\$CommentedJan 24, 2017 at 7:08

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.