5
\$\begingroup\$

This is pretty much the first Powershell script I've ever done and I am here looking for a quick review of it. Is there something I could have done better? Is the format okay? I don't know much about expected standards, and this was cobbled together as a result of me wanting to learn.

$Company = Read-Host 'Enter Company Name:'
$Credentials = Get-Credential

Write-Host 'Thank you! Creating client FTP folder on ServerA.' -ForegroundColor Magenta -BackgroundColor White 

Invoke-Command ServerA {mkdir ("e:\ftp\users\" + $args[0] + "\INCOMING")} -ArgumentList $Company -Credential $Credentials
Invoke-Command ServerA {mkdir ("e:\ftp\users\" + $args[0] + "\OUTGOING")} -ArgumentList $Company -Credential $Credentials

Write-Host 'Done. Now creating duplicate folder on ServerB.' -ForegroundColor Magenta -BackgroundColor White 

Invoke-Command ServerB {mkdir ("e:\ftp\users\" + $args[0] + "\INCOMING")} -ArgumentList $Company -Credential $Credentials
Invoke-Command ServerB {mkdir ("e:\ftp\users\" + $args[0] + "\OUTGOING")} -ArgumentList $Company -Credential $Credentials

Write-Host 'Done.' -ForegroundColor Magenta -BackgroundColor White  

Whenever we first set up a user for FTP, we have to set up a folder for them. The folder needs to be named after the company, and include "Incoming" and "Outgoing" subfolders. This needs to be created on ServerA, and the exact same thing created on ServerB. This script finds out what the company name is, checks credentials, and then creates the folders in the appropriate places.

Thoughts? Things I could research to improve it?

\$\endgroup\$
4
\$\begingroup\$

You're using this string literal in four different places.

"e:\ftp\users\"

I would suggest storing it in a variable. That way, if the path ever needs to change, one change would cascade through all of the places it needs to without any risk of making a mistake in just one of them.

The same could be said of the "\INCOMING" and "\OUTGOING" literals as well.

\$\endgroup\$
4
\$\begingroup\$

Congratulations, you managed to get a PS script to run, that's already something!

Jokes aside, you should look into parameterizing the script, so you could invoke it with command-line arguments instead of prompting for them, if that's possible. This would also reduce the maintenance burden on the script, because if the "server A" and "server B" aren't hard-coded in the script, there's nothing to change but how the script is invoked when either server needs to be changed.

Something like this at the start of the script:

param (
    $Company = "Contoso Inc."
    $ServerA = "PathA"
    $ServerB = "PathB"
)

There's no need for Write-Host, if you want to output a string, just output a string:

"Some output"

Will output the string... Some output. Yup. That easy.

\$\endgroup\$
2
\$\begingroup\$

In addition to the already great answers it may also be worth your time to modularize the code file so that it's easy to invoke and easy to distribute. that way, instead of having to invoke it as

. ./createcompanyfolders.ps1

you can add it to your modules directory and just type (assuming you named it this way)

New-CompanyFolders

By creating the script as a function in a module, it also makes it easy to add native PowerShell help to your script that describes usage, expectations, etc. All of this is very easy to do without any significant extra work. Remember, documenting your code will help when in 18 months someone has to come along and modify the code, knowing nothing about it. Doubly so because that person is likely to be you!

Here's a great post by Ed Wilson that describes how to modularize an already written PowerShell script and it shows how to include native help as a comment. A little old, but still very relevant.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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