Code Structuring, Functions and Layout

This topic contains 3 replies, has 3 voices, and was last updated by Profile photo of Adnan Rashid Adnan Rashid 2 years, 5 months ago.

  • Author
    Posts
  • #16779
    Profile photo of Adnan Rashid
    Adnan Rashid
    Participant

    Hello!

    So firstly apologises, this is going to be abit of a long thread to read as i'm going to try be as detailed as possible.

    I have been doing Powershell properly for around 6 months I would say and i can pretty much accomplish what i need to do. At the beginning this was fine and enough. I would write my script and it would work so that's brilliant.

    As time has gone by, I've seen myself having to revisit scripts, improving them adding in error handling etc... and that is fine. However i have also noticed that i just do not know how to structure my code properly. I also don't know how to pull out areas of my script and make them into a function and how to call them from other parts of the script.

    I have attached the code below – however its not actually that big, instead its been repeated almost 4 times! Why? Because i dont know how to segregate this and pull out the function and call the function in the necessary order.

    This is why i would love some advice – be brutal that's fine – I really want to become a better scripter so all the advice i can get here is great.

    The core of what i would like to know is the following:

    1. How would you pull out the repetitive code below and call it?
    2. What would you improve in this code?
    3. What is done very badly in this code.

    This is a first draft also – so i've literally just got it to work – no error handling, no parameters etc.. done yet.

    [b]What exactly does the code below do? [/b]

    Essentially it is checking the permissions for each folder and then ensuring they have particular keywords such as 'Everyone' – if they have all the necessary keywords, that is compliant. If they do not, they are not in compliance and something has gone wrong with the permissions.

    The child folder should not have 'Everyone' and if it does, then that is also out of compliance.

    1. First we are reading from a CSV file a list of project names
    2. We find out which VNX share we are searching 1 or 2 and then call the function (which is a terrible function because its doing all sorts of crap)
    3. If we head over to the function now it contains two switches representing VNX01 and VNX02

    4. The first thing to note is switch 1 is identical to switch 2 – the difference is VNX01 or VNX02 thats it – so i have repeated out code already once here

    5. Next we have two for each statements – one for the parent, one for the child – again they are identical, the child is just looking at the child folder instead
    6. The first foreach statement first checks every folder and outputs the result to a text file. It then checks to see if its matching and if it does it continues and if it doesn't then its flagged up.
    7. The same concept for child.

    The reason there is two foreach statements is because, something weird was going on with $project variable when i moved from parent to child and i wasn't looping through the list properly so i had to call the foreach statement again to ensure nothing is going weird.

    – that is another problem, when code is so big and repetitive its just bad... its bad for maintenance, troubleshooting and just general usage.

    [b]Finally – For anyone that takes the time to read this and help me, a massive thank you and very highly appreciated. [/b]

    Thank You

  • #16782
    Profile photo of Don Jones
    Don Jones
    Keymaster

    Consider reading "Learn PowerShell Toolmaking in a Month of Lunches."

    Essentially, I prefer to make as much as possible into functions, and to store those in a script module. A function – more generically, a command – should do one thing, and one thing only. Just like PowerShell commands. So, one command to "Import-ProjectFromCSV," one command to "Get-VNXShare," etc.

    Those tools always accept input ONLY via parameters, and ONLY produce output as pipeline objects (they may also write Verbose messages and stuff, but that's not "output").

    Taken individually, the tools don't do anything useful. So you also write a "controller script" that brings them together for a specific task. That's my approach to modularization, and it closely follows the shell's native commands. Tools do one thing, and one thing only, and a controller script is often very simple and just brings the tools together, perhaps applying some context-specific logic to the tools that are run.

    In your case, I think a lot of the functionality could be broken out into tools. For example, it's easier to repeatedly call a function to do something than to chunk a bunch of code inside nested ForEach loops. At least, easier in terms of troubleshooting and maintenance, because a function can be tested independently if needed.

  • #16784
    Profile photo of Mathieu Buisson
    Mathieu Buisson
    Participant

    An easy way to get rid of the 2 redundant code blocks of in the switch statement ( and to get rid of the Switch statement all together) is to make your VNX share number a parameter.

    Below, the parameter $VNXArrayNumber (which I guess should be mandatory) will represent your share name.
    Then, the variable $ProjectNamesFromCSV is derived from $VNXArrayNumber and is replacing your variables $VNX01 and $VNX02.
    Now, we have a single variable instead of 2, which allows us to suppress the redundancy in the Switch statement.

    function testSharePath {
    
    [cmdletbinding()]
    
    Param(
    [Parameter(mandatory=$true)]
    [string]$VNXArrayNumber
    )
    
    $ProjectNamesFromCSV = Import-Csv -Path (Join-Path -Path "C:\VNX" -ChildPath "$VNXArrayNumber.csv") -Header Project
    
    Foreach ($project in $ProjectNamesFromCSV) {
        do this
        do that
        do these
    
        } # End of Foreach
    

    Also, I would generally prefer populating a variable with a parameter rather than prompting the user with a Read-Host, unless the script is destined to be used by "non-command-line-savvy" users.
    I added [cmdletbinding()] , although not necessary, because I think it is a good habit and it gives you nice things.
    It allows you to easily leverage Powershell Common parameters , like -Verbose or -Debug by using Write-Verbose or Write-Debug in your function.

    Also, if you are sure that your share names will always ever be VNX01 or VNX02, you may want to make sure that the value of the parameter $VNXArrayNumber is either "VNX01" or "VNX02".
    You can do that by adding the ValidateSet attribute to the $VNXArrayNumber parameter, like so :

    function testSharePath {
    
    [cmdletbinding()]
    
    Param(
    [Parameter(mandatory=$true)]
    [ValidateSet("VNX01", "VNX02")]
    [string]
    $VNXArrayNumber
    )
    
    $ProjectNamesFromCSV = Import-Csv -Path (Join-Path -Path "C:\VNX" -ChildPath "$VNXArrayNumber.csv") -Header Project
    
    Foreach ($project in $ProjectNamesFromCSV) {
        do this
        do that
        do these
    
        } # End of Foreach
    

    By the way, the "something weird" with your $project variable may have something to do with the "-Header Project" at the end of your Import-Csv command.
    Because of that , you may be looping through all the rows in the CSV file [b]AND[/b] the extra "Project" header added by Import-CSV -Header.
    Do you really need to add a header ?
    According to what I see in your script, the answer is no (but I might be missing something).
    Unless your CSV file has one single column, I don't see why you add a single header, instead of a comma-separated list of headers for each column in your CSV file.

    If you really need to add a header , I would suggest you read the section explaining the -Header parameter in the help for Import-CSV.

  • #16820
    Profile photo of Adnan Rashid
    Adnan Rashid
    Participant

    Don,

    Thanks for the advice and I got a copy of toolmaking so I am going through that now.

    Mathieu, thanks for your advice and efforts also. I will give those a try and let you know. The reason a header is used is because it is just a single column.

    I think im going to go through the book and then rewrite the code, try and break down into its necessary functions as mentioned above, then rebuild the code calling those functions so hopefully its abit more dynamic script and also not so repetitive.

    Thanks,

    Adnan

You must be logged in to reply to this topic.