1
\$\begingroup\$

Using Powershell, I've created a script to create a tree-view of a group/s' memberships in Active Directory. I originally had layers of nested loops, but I wanted a more algorithmic solution.

The following code does the following:

  1. Get user input for: input source, depth-level int, output location
  2. Recursively find all the AD Groups from the input and print out in tree-style formatting to a plaintext file.

I'm concerned the following code is too complicated and has reached "spaghetti-code" status. I've read multiple times code-readability is very important. What improvements could be made to improve readability and make the code more professional? Any additional comments and advice to keep me from going down the dark path of sloppy programming are appreciated.

###   -- PRIMARY FUNCTION --   ###
function getGroupMemberships (){
    ### DESCRIPTION: Main Function ###

    # -- SWITCH OPTIONS -- #
    write-host "options:`n1: Type a User/Group`n2: Type filepath of CSV file containing list of users/groups."
    $route = read-host ":"
    switch ($route)
    {
        1 {
            $group = read-host "type group or user"
            $primaryGroups = Get-ADPrincipalGroupMembership $group    
        }

        2 {
            $continue = $false
            while ($continue -eq $false) {
                $csv = read-host "CSV Filepath (no quotes!)"
                if ((test-path $csv) -eq $true) {
                    $continue = $true
                }
                elseif ((test-path $csv) -eq $false) {
                    write-host "Location $csv does not exist.  Please try again..."
                }
            }
           $primaryGroups = Import-Csv $csv
        }
    }

    # -- OTHER REQUIRED OPTIONS -- "
    $maxLevelInt = read-host "# depth of levels"
    $ouputFilePath = read-host "TXT Output Filepath: (no quotes!)"
    write-host "------------------------------------"
    $level = 0
    getNextGroupMembers $level $primaryGroups
}

### -- SECONDAY FUNCTIONS -- ###
    function getNextGroupMembers ($currentLevel, $groupIn) {
        # DESCRIPTION: main loop for gathering next groups' memberships and recursively finding

        $level++
        $groupIn = ( $groupIn | sort-object name ).name

        foreach ($group in $groupIn) {
            outputFormat $level $group
            $nextLevelMemberships = Get-ADPrincipalGroupMembership $group | select name
            if ( $level -eq $maxLevelInt ) { break } # if reached maxLevelInt
            getNextGroupMembers $level $nextLevelMemberships
        }

        $level-- # subtracts level counter once the loop completes
    }


        function outputFormat ([int]$level,$groupName) {
            # DESCRIPTION: creates string for formatting tree view; adds bars and spaces to make more readable

            $spaces="    "; $bar="|"
            if ($level -eq 1) { $out = $groupName }
            elseif ($level -eq 2) { $out = ( $spaces + $groupName ) }
            elseif ($level -gt 2) { $out = ( $spaces + $bar )*( $level - 2 ) + $spaces + $groupName }
            write-host $out; writeToFile $out
        }

        function writeToFile ($writeLine) {
            ### DESCRIPTION: writes string to file location ###

            $writeLine | out-file $ouputFilePath -append
        }

    function checkFilePath ($path) {
        $filepathTest = test-path $path
        return $filepathTest
    }

### RUN ###
getGroupMemberships
\$\endgroup\$
0

1 Answer 1

Reset to default
2
\$\begingroup\$

Separating Presentation From Logic

You're mixing interface code with code that retrieves the group membership information and provides the logic of the script.

Consider separating these. Ideally, you would have a function that accepts as a parameter a user or group object, and returns some kind of object that contains all of that user's memberships (even if it's just a nested [hashtable]).

The object returned from this would be passed to another function to format it for display (creating the tree view).

A separate function or set of functions is responsible for getting the information from the user (prompting, validating, etc.).

This separation allows you to take the object with the actual data, and make other functions that use it (to display it graphically, to write it as structured data to an XML file or to a database, to gather statistics on it, whatever), or to use it in quick one-liners to test some logic.

Similarly, you can change the way the interface operates without worrying about breaking the logic. You can change the way the object is displayed without introducing errors in the other parts.

The way it is now, these are all intertwined in such a way that you can't easily change one thing without changing another.

Actual Code Stuff

Option 2

$continue = $false
while ($continue -eq $false) {
    $csv = read-host "CSV Filepath (no quotes!)"
    if ((test-path $csv) -eq $true) {
        $continue = $true
    }
    elseif ((test-path $csv) -eq $false) {
        write-host "Location $csv does not exist.  Please try again..."
    }
}

There are several things to note here:

if ((test-path $csv) -eq $true) {}

You don't need -eq $true:

if (test-path $csv) {}

Similarly with the elseif:

elseif ((test-path $csv) -eq $false) {}

vs.

elseif (!(test-path $csv)) {}

or:

elseif (-not (test-path $csv)) {}

But actually, you don't need elseif at all here. You're testing for the existence of this file twice. You already checked for it in the if so, just else would do:

if (test-path $csv) {
    break
} else {
    Write-Host "Location $csv does not exist.  Please try again..."
}

By the way, it's good that you're checking user input and figuring out if the file exists. But the user could just press enter (on purpose or by accident) with an empty string. If that happens, Test-Path will throw an exception, so you need to check for this case too:

if ($csv -and (Test-Path $csv)) { break }

It's also a bit strange that you're looping while a variable named $continue is $false but breaking once it's $true (seems backwards). But more than that, you could just use the break keyword instead:

while ($true) {
    $csv = read-host "CSV Filepath (no quotes!)"
    if (test-path $csv) {
        break
    }
}

But going further, you don't need to use an infinite loop and then check a conditional to figure out when to break. It seems that way because you're using a while loop which checks the conditional at the beginning, but you need to execute the loop body at least once before checking the conditional.

That's where do {} while() or do {} until() comes in. Given that, your entire loop can be reduced to:

do {
    $csv = read-host "CSV Filepath (no quotes!)"
} until ($c -and (Test-Path $c))

This is missing the message to the user about the CSV not existing, but frankly I think they'll get it if it keeps prompting. If you really want to do that part, then you can still shorten it:

while ($true) {
    $csv = read-host "CSV Filepath (no quotes!)"
    if ($csv -and (test-path $csv)) {
        break
    } else {
        Write-Host "Location $csv does not exist.  Please try again..."
    }
}

One last thing, if the users are constantly wrapping the file name in quotes, you might consider just stripping them off instead of trying to get them to enter it another way:

$csv = $csv.TrimStart('"').TrimEnd('"')

Or replace all the quotes:

$csv = $csv -replace '"',''

getNextGroupmembers:

There isn't too much to say here other than the big changes that would be made by refactoring everything like I described at the beginning.

But a simple step you could take toward that goal is to remove the call to Get-ADPrincipalGroupMembership from the menu, and make this function do the lookups from the beginning. In other words, you give it the user or group you want the memberships for, not the first set of groups. It's already a recursive function; it can figure out how to get the initial memberships.

Important note about $maxLevelInt: you're reading it one function, then referring to it in another, without passing it along.

Frankly, I don't think that even works. I hesitated to even say this, but you could make that work by explicitly referring to it as global in both places:

$Global:maxLevelInt = read-host "# depth of levels"

and

if ( $level -eq $Global:maxLevelInt ) { break }

Please don't do this.

If you want to use this value, then make it a parameter of this function:

function getNextGroupMembers ($currentLevel, $groupIn, $maxLevel) {
        if ( $level -eq $maxLevel ) { break }
}

You just need to make sure to include it whenever you call the function (including the recursive calls to itself).

I'm assuming this check is to protect against circular nested groups. This will prevent your code from looping infinitely, but you might not get the output you need.

You might consider passing a list of all the groups checked so far so that you can skip a recursive call for those, and never get caught in the infinite loop, while still being able to enumerate the rest of the groups. If that were implemented, you could safely ignore a level limit.

The code for that is a bit outside the scope of a code review, but it's something you could work on.

outputFormat:

Fairly simple function here. I would say you could replace your if () elseif () elseif() with switch () as it seems to be a good use case for it.

Along the lines of separation, ideally this would return the formatted data which the caller can then decide whether to write it to the screen, or to a file, or multiple files, or whatever, but meh.

If you really wanted to go further with this, and you've gotten your functions refactored to a point where you can get a single object that contains all the nested membership data (as objects, not formatted), then you could start looking at using Format-Custom along with a PowerShell formatting file. This is a very advanced option that no one has ever used. Ever. But it would be pretty cool.

Required reading for this:

writeToFile:

This function accepts a line, and then writes to a pre-determined file. You're again using a global variable here (don't do this please), and I don't know how it's working (in my tests I can't get it to work).

You should pass the file name into it and reference it that way.

But going further, I'm not sure why you need this function at all. Why not just use Out-File directly?

checkFilePath:

There's no need to assign the result of Test-Path to a variable just to return it from the function. You could just return Test-Path $path or omit return:

Test-Path $path

But then of course I have to ask (rhetorically), why would you use this function at all?

And then I see that in fact you aren't using it at all. So I recommend deleting it!

Parameterizing the script itself

You can make the script itself take parameters the way a function would. At the very top, put a param() block:

param($UserOrCsv)

Why do this? Because you could give the user the option of skipping the interactive menu altogether and just running the process. Then the script could be called on a schedule, or by some other automated process.

Of course the above param block is rather annoying, because you would need to determine whether the user intended to pass a user name or a CSV file.

A better way is to use advanced function semantics so that you use advanced parameter sets. So at the top you could do something like this:

[CmdletBinding(DefaultParameterSetName='ByUser')]
param(
    [Parameter(
        ParameterSetName='ByCSV',
        Mandatory=$true
    )]
    [String]
    $CSV , 

    [Parameter(
        ParameterSetName='ByUser',
        Mandatory=$true
    )]
    [String[]]
    $User
)

Why do this? Because now they can call:

.\MyScript.ps1 -CSV C:\whatever\stuff.csv
.\MyScript.ps1 -User ThisGuy
.\MyScript.ps1 -User ThisGuy,ThatOne,SomeUser

You can do a lot more than that with advanced function syntax, like validating the existence of the CSV in the parameter declaration, accepting users via the pipeline, etc.

I won't get too much further into it because you may not even want it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.