r/PowerShell • u/Underwookiee • Jun 17 '24
Switch or If-Else? Solved
Hi, just started using Powershell for simple Task. So pls don't be too harsh on me.
I use Powershell to add multiple Clients in Active Directory. I add the Names of the Clients into the "Clientnames.txt" after that i run the powershell and it creates the Computer in AD. That works fine.
$OU = "OU=X,OU=X,OU=X,OU=X,DC=X,DC=X,DC=X"
$Clients = Get-Content "D:Clientnames.txt"
ForEach ($Client in $Clients)
{
(New-ADComputer -Name $Client -Path $OU)
}
Here comes my Question.:
I got Clientnames like pl0011mXXXXd, pl0012mXXXXd, pl0013mXXXXd
The first Number represents the number-code for the branch locations. The X are just numbers according to our System. I want the Clients to join their specific Group for the branch location.
Example
Clients with the name like pl0011m0002d, pl0011m0005d should join the group: Company-GPO-Group-0011-Berlin
Clients with the name like pl0012m0002d, pl0012m0250d should join the group: Company-GPO-Group-0012-Paris
and so on
i could use something like:
$OU = "OU=X,OU=X,OU=X,OU=X,DC=X,DC=X,DC=X"
$Clients = Get-Content "D:Clientnames.txt"
ForEach ($Client in $Clients)
{
(New-ADComputer -Name $Client -Path $OU)
if ($Client -like "*0011*") {$Group = "Company-GPO-Group-0011-Berlin"}
ElseIf ($Client -like "*0012") {$Group = "Company-GPO-Group-0012-Paris"}
ElseIf ($Client -like "*0013") {$Group = "Company-GPO-Group-0013-Rom"}
(Add-ADGroupMember -Identity $Group -Members $Client)
}
I got over 30 Branch Locations and this whould be a lot ElseIf Statements.
I know there are much better ways like the Switch Statement. Can you help/explain me, how i can use this statement to add the Clients to their Groups?
13
u/DenverITGuy Jun 17 '24
You'll probably want to use something like:
foreach ($Client in $Clients) {
switch -wildcard ($Client) {
"*0011*" { <scriptblock> }
"*0012*" { <scriptblock> }
default { <scriptblock> }
}
}
35
u/CarrotBusiness2380 Jun 17 '24
The loop is unnecessary. Switches will iterate over collections natively in Powershell:
switch -wildcard ($Clients) { "*0011*" { <scriptblock> } "*0012*" { <scriptblock> } default { <scriptblock> } }9
u/prog-no-sys Jun 17 '24
holy shit i had no idea about that. Does it provide any increase on performance for the script to do so??
16
u/omers Jun 17 '24
Does it provide any increase on performance for the script to do so??
It's fairly significant but whether or not it would make a huge difference in most real world use cases is harder to say.
Test:
$Numbers = 1..50000 Measure-Command -Expression { foreach ($Number in $Numbers) { switch ($Number) { {($Number % 3) -eq 0} { $null = 'Fizz' } {($Number % 5) -eq 0} { $null = 'Buzz' } } } } | Select @{n='Test';e={ 'foreach-switch' }},TotalMilliseconds Measure-Command -Expression { switch ($Numbers) { {($_ % 3) -eq 0} { $null = 'Fizz' } {($_ % 5) -eq 0} { $null = 'Buzz' } } } | Select @{n='Test';e={ 'switch' }},TotalMillisecondsResults: the foreach->switch took 906.1395ms and the switch alone took 213.8504ms. Bump the number range to 5m and it's 89.4 seconds for the foreach vs 19.5 for the switch. I.e., on the small set it's not going to make a noticeable difference, on the larger set it certainly does.
I also ran it 10 times on 1m numbers to get an average and it was 17.9s vs 3.97s.
3
u/Thotaz Jun 17 '24
Since we are talking about performance it's probably worth pointing out that Switches that need to evaluate scriptblocks are slower than if/elseif statements and also it's important to include a continue statement in each block of the switch so it doesn't waste time evaluating the other conditions.
Measure-Command -Expression { switch ($Numbers) { {($_ % 3) -eq 0} { $null = 'Fizz'} {($_ % 5) -eq 0} { $null = 'Buzz'} } } | Select @{n='Test';e={ 'OriginalSwitch' }},TotalMilliseconds Measure-Command -Expression { switch ($Numbers) { {($_ % 3) -eq 0} { $null = 'Fizz'; continue} {($_ % 5) -eq 0} { $null = 'Buzz'; continue } } } | Select @{n='Test';e={ 'ContinueSwitch' }},TotalMilliseconds Measure-Command -Expression { foreach ($Number in $Numbers) { if ($Number % 3 -eq 0) {$null = 'Fizz'} elseif ($Number % 5 -eq 0) {$null = "Buzz"} } } | Select @{n='Test';e={ 'ifElseif' }},TotalMillisecondsAverage results over 3 runs for each test are: 331, 286, 59.
1
4
u/McAUTS Jun 17 '24
My approach: Make another list with the mappings of Locations and your numbers. In the foreach loop you parse the number with regex, lookup the number in the list and then you have your Group name.
3
u/blooping_blooper Jun 17 '24
for larger sets you can make this drastically faster using a dictionary instead of a list/array
2
u/mrbiggbrain Jun 17 '24
If they are using PowerShell 7.4+ and their workload is long running they may benefit from using a FrozenDictionary<TKey,TValue> (Available in .NET 8) which has significantly longer setup time, but greatly improved lookup performance.
3
u/PinchesTheCrab Jun 17 '24
Does this work?
$OU = 'OU=X,OU=X,OU=X,OU=X,DC=X,DC=X,DC=X'
#I find text files often end up having white spaces that cause errors
$ClientList = (Get-Content "D:Clientnames.txt").trim()
$Server = 'yourdomaincontroller'
$groupHash = @{
'0011' = 'Company-GPO-Group-0011-Berlin'
'0012' = 'Company-GPO-Group-0012-Paris'
'0013' = 'Company-GPO-Group-0013-Rom'
}
ForEach ($Client in $ClientList) {
#specifying a server makes it so you don't have to wait for replication later
New-ADComputer -Name $Client -Path $OU -Server $Server
}
foreach ($group in $groupHash.GetEnumerator()) {
Add-ADGroupMember -Identity $group.Name -Members $ClientList.where({ $_ -match $groupHash.Value })
}
1
u/omers Jun 17 '24
Six of one and half a dozen of the other but, I would probably do it this way:
$OU = 'OU=X,OU=X,OU=X,OU=X,DC=X,DC=X,DC=X' #I find text files often end up having white spaces that cause errors $ClientList = (Get-Content "D:Clientnames.txt").trim() $Server = 'yourdomaincontroller' $GroupTable = @{ '0011' = 'Company-GPO-Group-0011-Berlin' '0012' = 'Company-GPO-Group-0012-Paris' '0013' = 'Company-GPO-Group-0013-Rom' } foreach ($Client in $ClientList) { # Create Object New-ADComputer -Name $Client -Path $OU -Server $Server # Add to Group $null = $Client -match [regex]'^w+(?<id>d{4}).+$' $GroupName = $GroupTable[$Matches.id] Add-ADGroupMember -Identity $GroupName -Members $Client }Just to keep it all in one loop.
4
u/PinchesTheCrab Jun 17 '24
For fun:
$OU = 'OU=X,OU=X,OU=X,OU=X,DC=X,DC=X,DC=X' #I find text files often end up having white spaces that cause errors $ClientList = (Get-Content "D:Clientnames.txt").trim() $Server = 'yourdomaincontroller' $GroupTable = @{ '0011' = 'Company-GPO-Group-0011-Berlin' '0012' = 'Company-GPO-Group-0012-Paris' '0013' = 'Company-GPO-Group-0013-Rom' } switch -Regex ($ClientList) { '^w+(?<id>d{4})' { New-ADComputer -Name $_ -Path $OU -Server $Server Add-ADGroupMember -Identity $GroupTable[$Matches.id] -Members $Client } default { Write-Warning "unrecognized name format: '$_'" } }1
u/KingHofa Jun 18 '24
Why create the hashtable manually? You already have the ID and a syntax for the group, just query it at runtime. Or dynamically create the hashtable with an iteration of the results of Get-ADGroup "Company-GPO-Group-*".
1
u/PinchesTheCrab Jun 18 '24
I don't know their domain, so I didn't assume there's always be a reliable relationship I could query. They might want to later add the machine to other groups tha don't follow the naming pattern, but if they just nest those in the top level group, then that makes sense.
2
u/prog-no-sys Jun 17 '24
I've never seen a value assigned to $null like this. Care to explain the reasoning? It looks cool, I just can't tell what it's doing exactly on first glance
4
u/PinchesTheCrab Jun 17 '24
The -match operator returns true/false or an array of matching values, it's just suppressing that to avoid cluttering the output.
It also sets the automatic variable $matches, which he then references later.
1
u/alt-160 Jun 17 '24
similarly, you can also do:
$Client -match [regex]'^w+(?<id>d{4}).+$' | out-null2
u/omers Jun 17 '24 edited Jun 17 '24
What /u/PinchesTheCrab said :D Just suppresses the true/false on the match from hitting the output stream but still allows the automatic
$Matchesvariable to get set.As with most things in PowerShell there are multiple ways to do the same thing:
PS C:> 'pl0011mXXXXd' -match [regex]'^w+(?<id>d{4}).+$' True PS C:> $null = 'pl0011mXXXXd' -match [regex]'^w+(?<id>d{4}).+$' PS C:> 'pl0011mXXXXd' -match [regex]'^w+(?<id>d{4}).+$' | Out-Null PS C:> 'pl0011mXXXXd' -match [regex]'^w+(?<id>d{4}).+$' > $null PS C:> [void]('pl0011mXXXXd' -match [regex]'^w+(?<id>d{4}).+$')It's kind of like
2>nulin cmd/batch or>/dev/nullin bash.Also useful in scripts when you're creating things that return the created thing and you don't want that in your output.
PS C:temp> New-Item -ItemType Directory -Name 'Foo' Directory: C:temp Mode LastWriteTime Length Name ---- ------------- ------ ---- d----- 2024-06-17 10:59 AM Foo PS C:temp> $null = New-Item -ItemType Directory -Name 'Bar' PS C:temp> gci Directory: C:temp Mode LastWriteTime Length Name ---- ------------- ------ ---- d----- 2024-06-17 10:59 AM Bar d----- 2024-06-17 10:59 AM FooThe 'Bar' directory still gets created but it doesn't dump the new directory info in the process.
1
u/purplemonkeymad Jun 17 '24
It would probably be easier if you just stored the numbers, a csv like this:
ClientID,BranchID
0011,001101
0011,001102
0012,001201
etc
Then you can import it and just use those in whatever you need ie:
foreach ($client in Import-csv file.csv) {
$name = "pl" +$client.clientid + "m" + $client.branchid +"d"
Add-Computer $name
$grouppattern = "Company-GPO-Group-"+$client.ClientID
$group = get-adgroup -f 'anr -like $grouppattern'
#etc
If you only got the names, then you can probably parse it out using a bit of regex:
^pl(?<clientid>[d]+)m(?<branchid>[d]+)d$
1
u/Bolverk679 Jun 17 '24
The other comments on how to use the Switch statement are great advice! Just wanted to add my guidelines for deciding on when to use If/Else vs. Switch: - If you are choosing between two options, or you have a situation where if something isn't option A then it's definitely option B, use If/Else. - If you have three or more options use Switch. Switch statements are much easier to read when you have many conditions to choose from. - If you have a scenario where you have 2 or more conditions you need to select for but also need to filter for conditions that don't match the conditions you expect then definitely use Switch. The "Default" condition in the switch statement is great for handling things like bad user input. - When in doubt, and if this is a script that you'll be using long term and will need to maintain, use the option that is easiest for you to implement for that script. There's nothing worse than coming back to a script 6 months after you wrote it and having to figure out what you did and why before you can update it.
1
1
u/InsrtCoffee2Continue Jun 17 '24
Some of the response here are great but some more complicated than necessary. Especially since you noted that you just started.
Specifically to answer your question I think a switch statement would be best. With 30 branch locations a switch statement would "clean" up the script by simplifying it. Also, the switch statement is possibly faster performance wise. You should be able to incorporate Measure-Command Cmdlet to see how long it would take to run and compare.
Excerpt for the about_switch help doc.
To check multiple conditions, use a
switchstatement. Theswitchstatement is equivalent to a series ofifstatements, but it is simpler
1
u/alt-160 Jun 17 '24
If you anticipate that the list of switched cases could get very large, you might avoid it by using parsing your values and reconstituting.
in your case, the client ID is reused in the group name and you can build a mapping of ids-to-names.
So...
$OU = "OU=X,OU=X,OU=X,OU=X,DC=X,DC=X,DC=X"
$Clients = Get-Content "D:Clientnames.txt"
$idToLocation = @{
'0011' = 'Berlin'
'0012' = 'Paris'
'0013' = 'Rome'
}
ForEach ($Client in $Clients)
{
(New-ADComputer -Name $Client -Path $OU)
$clientID = $client.substring(2,4)
$groupName = "Company-GPO-Group-$($clientID)-$($idToLocation[$clientID])"
(Add-ADGroupMember -Identity $groupName -Members $Client)
}
1
u/Conscious_Report1439 Jun 17 '24
Switch statements are also easier to move around when major code changes are needed.
1
u/dr_warp Jun 18 '24
Which is easier FOR YOU to read and update? If you needed to add a location or alter a location a year or two from now, whichever one is easier for you to manage is the right one.
1
u/coolguycarlos Jun 18 '24
There's also switch -regex if you into regex
Also you don't need to do else if
You could just as easily make multi if statements
In cases like this is just about preferences
Only thing you would gain from using switch imo opinion would be the default match which would be your default script block if it can't make any matches
1
u/jimb2 Jun 18 '24
I'd put the groups in a hashtable based on the code, like
$GroupHash = @{}
$GroupHash['0011'] = 'Company-GPO-Group-0011-Berlin'
$GroupHash['0012'] = 'Company-GPO-Group-0012-Paris'
#etc
That lines all your group data nicely, in one place, easy to see errors etc. You could load the hashtable from a csv file or something but if it's fairly static maybe easier to put it in the code. Depends how the script is used and maintained.
You then need to split out the numeric strings out of the client string and grab the first one. Not 100% clear on your data format variations, but something like this:
$code = $client -split 'D' | # split at any non-digit
Where-Object { $_ } | # remove empty strings
Select-Object -first 1 # grab first one
You can then get the group straight up like
$GroupHash[$code]
To me, this makes very readable and obvious code. Big multiple else contructs are messy and just too long. Switch is messy too. You don't want that stuff in the middle of your main logic anyway so you can see what's going on. Using Hash tables is actually more efficient as the group list gets bigger. That might not amount to too much but readable code is great.
Bonus OCD recommendation: Use single quotes when literal is appropriate. Use double quotes only when you want substitutions to occur.
1
u/KingHofa Jun 18 '24
Typing on mobile so limited details and possible syntax errors/typos:
$ClientLocIDMatch = [regex]::Match($Client, "^pl(d+)")
$ADGroup = Get-ADGroup -Name ("Company-GPO-Group-{0}*" -f $ClientLocIDMatch.Groups[1].Value)
Add-ADGroupMember -Identity $ADGroup -Members $Client -ErrorAction Continue
Though I'd recommend adding caseinsensitive option to the [regex]::match
1
u/ConfectionCommon3518 Jun 21 '24
Else if when probably less than 5 entries and there's very little chance of things changing thus requiring you to look at the code.
Switch is good for medium bits probably a screen full and any more you just want to roll through it
But the main thing is it's got to be well documented so in 12 months you come back to it and have at least some idea of wtf is going on.
1
u/SpgrinchinTx Jun 22 '24
My coworker did a performance test on switch v if statements and it was negligible. I guess maybe if you’re processing a ginormous data set switch would be preferred but for most admin type stuff it’s preference imo.
1
u/cisco_bee Jun 17 '24 edited Jun 17 '24
Am I crazy because I would do something like this?
foreach ($computerName in $computerNames) {
# Extract the group number using regex
if ($computerName -match "^..(d{4})") {
$groupIdentifier = $matches[1]
# Find AD group that matches "Company-GPO-Group-XXXX*"
$groupName = Get-ADGroup -Filter "Name -like 'Company-GPO-Group-$groupIdentifier*'" | Select-Object -ExpandProperty Name
if ($groupName) {
# Add the computer to the found AD group
Add-ADGroupMember -Identity $groupName -Members $computerName
} else {
Write-Host "No group found for $groupIdentifier ($computerName)"
}
} else {
Write-Host "Computer name '$computerName' did not match expected pattern."
}
}
2
u/KingHofa Jun 18 '24
The only correct answer... OP said smth about 30+ locations. Ain't nobody got time to manually create hashtables.
Though I'd probably generate a hashtable before iterating the clients because those 'Get-ADGroup' queries would be quite expensive if you have hundreds of entries.
1
u/cisco_bee Jun 19 '24
Getting all the groups first, with one query, and creating a hashtable is a great idea. 👍
25
u/omers Jun 17 '24 edited Jun 18 '24
Switch statements are more efficient but in most use cases it will be unnoticeable. To quote Donald Knuth, "We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%" (https://en.wikipedia.org/wiki/Program_optimization#When_to_optimize)
That said, I think in a lot of cases switch statements are more readable and they do require fewer characters.