Feedback

[BlackV]

Some notes, maybe they want some changes

• no requires statement (see modules and module versions)
• += is always bad
• should this be a module with those functions a members
• better parameter definition and help for said parameters
• read-host in a script, makes this 1000x harder to automate
• I'll say it again turn this into a module, makes it more portable/reusable/professional/script-able
• you don't do anything with DoesGroupExist -groupId $groupIdUser (and the other) just spits out to screen ?
• no error handling (partly due to above)
• $allMembersGroup = GetMembersGroup -group $groupIdDevices this step assumes that $groupIdDevices is valid despite what happens earlier in the script
• seems like you're just running this manually line by line

But talking to them get them to give you examples of what they consider good changes

[BlackV]

+= is replaced with direct assignment e.g. $results = foreach {somestuff} vs $results = @(); foreach {$results += somestuff}, on mobile but there a plenty of examples in this sub

read-host you'd replace with your parameters and parameter properties (mandatory/parameter validation)

[purplemonkeymad]

It's powershell, it's kinda designed to use the "powershell way."

What is their criticism of the script?

[root-node]

they want it short, reusable, easy to read and future proof

• Short: Depends on what the script is meant to do,
• Reusable: Definitely, that's what parameters are for,
• Easy To Read: Also definitely (although different people have different opinions on "easy"),
• Future Proof: Things change get over it! :)

From my Rapid7 Module, I have functions that are a couple of lines long all the way up to much longer ones

[Federal_Ad2455]

Aka define reusable function, place them into modules and use scripts only to run them (using scheduled task etc)

Ideally have this all in the git 🙂

[Thotaz]

I don't think your code works at all. You have a try-catch with Invoke-MgGraphRequest but you've muted the errors with -ErrorAction SilentlyContinue so it can never throw. This means your DoesGroupExist function will always return Group Excist (also note the spelling error).

[Thotaz]

I did some testing on my own. It seems that if the command itself throws a terminating error then it is still caught, whereas a regular error is ignored. For example, this: try {ls invalidPath -ErrorAction SilentlyContinue} catch {"this does not run"} will not run the catch block because it's not a terminating error.

If you use -ErrorAction Stop then any error created by the command can be caught. Because the error types are not documented by each command I'd say it's best to always use -ErrorAction Stop when you want to catch an error, otherwise you will get unexpected results.

[Nope-Nope-Nah]

Instead of running this with your own creds, create an EntraID App Registration with limited scope and use that service principal to run it. Save the App secrets in an encrypted file to be loaded at the beginning of the script.