locked
Powershell: What is the best practice for this script? RRS feed

Answers

  • You're messing up the script. Opening brackets should be on the same line. Indenting is wrong. It's hard to read. You don't have to specify the $userName = $user.username, stuff. It was just a minor error that can easily be fixed:

    Import-Csv C:\scripts\scratch\CreateBulkUserAdmins.csv | ForEach-Object {
        if (Get-ADUser -Filter "SamAccountName -eq $($_.username)") {
            Write-Warning "$($_.username) already exists in AD"
        } else {
            # Fill a HashTable with properties from CSV
            $newADUser = @{
                SamAccountName        = $_.username
                UserPrincipalName     = "$($_.username)@testenvironment.local"
                Name                  = "$($_.firstname) $($_.lastname)"
                GivenName             = $_.firstname
                SurName               = $_.lastname
                Enabled               = $true
                DisplayName           = "$($_.firstname) $($_.lastname)"
                Path                  = $_.ou
                EmailAddress          = $_.email
                AccountPassword       = (ConvertTo-SecureString $_.password -AsPlainText -Force)
                ChangePasswordAtLogon = $true
            }
    
            # Create the user by splatting the HashTable
            New-ADUser @newADUser
        }
    
        # Now add user to group here
        # e.g. Add-ADGroupMember ...
    }

    Please take some time to read previous answers. It seems like you aren't reading any of them, you just start over to create another mess. jrv already explained how to use the filter (with double quotes instead of curcly braces).

    • Proposed as answer by jrv Wednesday, August 22, 2018 7:06 PM
    • Marked as answer by Richard MuellerMVP Wednesday, August 29, 2018 1:14 PM
    Tuesday, August 21, 2018 10:03 AM

All replies

  • Her are best practices for PowerShell scripts:

    1. Learn PowerShell  
    2. PowerShell Documentation
    3. PowerShell Style Guidelines

    Pay close attention to #3 above. What you don't understand 1 and 2 will help you.


    \_(ツ)_/

    Friday, August 17, 2018 7:35 PM
  • Please edit your original post and post the code with the code posting tool provided on the edit toolbar.

    \_(ツ)_/

    Friday, August 17, 2018 7:38 PM
  • deleted
    Friday, August 17, 2018 7:42 PM
  • deleted

    Friday, August 17, 2018 7:43 PM
  • It is not marked as spam.  Just use the edit button above the post.

    Now that you have shown us the script in its original condition read the BP/Style article and apply what you have learned to the script. First thing is proper indentation.  Then variable naming conventions and CmdLet presentation.

    Go through each of the documents sections until you have altered your code to comply.


    \_(ツ)_/

    Friday, August 17, 2018 7:50 PM
  • deleted

    Friday, August 17, 2018 8:22 PM
  • Sorry about that.  The site seems to be having issues in the last few days.

    \_(ツ)_/

    Friday, August 17, 2018 8:42 PM
  • Hi,

    Please take some time to read through the PowerShell Style Guidelines, it really approves the quality of your script and helps with its readability. 

    I'll help you get started with creating the user. Since you are already in a foreach loop while creating the users, don't end it and start equal loops again for adding to groups. Use that same loop, less code and it will run a lot faster.

    Import-Csv C:\scripts\scratch\CreateBulkUserAdmins.csv | ForEeach-Object {
        if (Get-ADUser -Filter { SamAccountName -eq $_.username }) {
            Write-Warning "$($_.username) already exists in AD"
        } else {
            # Fill a HashTable with properties from CSV
            $newADUser = @{
                SamAccountName        = $_.username
                UserPrincipalName     = "$($_.username)@testenvironment.local"
                Name                  = "$($_.firstname) $($_.lastname)"
                GivenName             = $_.firstname
                SurName               = $_.lastname
                Enabled               = $true
                DisplayName           = "$($_.firstname) $($_.lastname)"
                Path                  = $_.ou
                EmailAddress          = $_.email
                AccountPassword       = (ConvertTo-SecureString $_.password -AsPlainText -Force)
                ChangePasswordAtLogon = $true
            }
    
            # Create the user by splatting the HashTable
            New-ADUser @newADUser
        }
    
        # Now add user to group here
        # e.g. Add-ADGroupMember ...
    }

    Now pay close attention to the indenting and the use of a hashtable to avoid backticks.

    Go ahead and finalize the script. We would be happy to review your code.




    Friday, August 17, 2018 9:02 PM
  • deleted

    Friday, August 17, 2018 9:23 PM
  • deleted

    Monday, August 20, 2018 6:51 AM
  • deleted

    Monday, August 20, 2018 6:53 AM
  • deleted

    Monday, August 20, 2018 7:17 AM
  • Make sure the delimiter is correct in the CSV file. If your CSV has the ';' of tab delimiter, and it is importing it with a comma ',' delimiter, it is not be able to read the headers.

    Monday, August 20, 2018 10:21 AM
  • deleted

    Monday, August 20, 2018 12:08 PM
  • deleted

    Monday, August 20, 2018 12:33 PM
  • deleted

    Monday, August 20, 2018 12:38 PM
  • You Csv has issues.  You shoe an example using commas then try to load it with ";".  You cannot do both.


    \_(ツ)_/

    Monday, August 20, 2018 5:04 PM
  • deleted

    Monday, August 20, 2018 5:11 PM
  • The line:

    if (Get-ADUser -Filter ( -eq $_.username){

    Is wrong.  You are missing the name.


    \_(ツ)_/

    Monday, August 20, 2018 5:14 PM
  • deleted

    Monday, August 20, 2018 5:29 PM
  • This is saying the variable is either typed wrong or is null.


    \_(ツ)_/

    Monday, August 20, 2018 5:43 PM
  • deleted

    Monday, August 20, 2018 5:49 PM
  • It is very hard to understand your posts as the code samples, errors abd question do not match.

    To use a property in PowerShell in a string you must do this:

    $($_.username)

    if (Get-ADUser -Filter "samAccountName -eq '$($_.username)'")

    And you must use double quotes and not {}.  Also the single quotes are needed.


    \_(ツ)_/

    Monday, August 20, 2018 5:53 PM
  • deleted
    Monday, August 20, 2018 5:55 PM
  • Wow I gues that helped also. Thank you I think you way is better.

    Thank you very much for the quick response. I really appriciate it :-)

    Monday, August 20, 2018 6:05 PM
  • deleted

    Tuesday, August 21, 2018 9:49 AM
  • You're messing up the script. Opening brackets should be on the same line. Indenting is wrong. It's hard to read. You don't have to specify the $userName = $user.username, stuff. It was just a minor error that can easily be fixed:

    Import-Csv C:\scripts\scratch\CreateBulkUserAdmins.csv | ForEach-Object {
        if (Get-ADUser -Filter "SamAccountName -eq $($_.username)") {
            Write-Warning "$($_.username) already exists in AD"
        } else {
            # Fill a HashTable with properties from CSV
            $newADUser = @{
                SamAccountName        = $_.username
                UserPrincipalName     = "$($_.username)@testenvironment.local"
                Name                  = "$($_.firstname) $($_.lastname)"
                GivenName             = $_.firstname
                SurName               = $_.lastname
                Enabled               = $true
                DisplayName           = "$($_.firstname) $($_.lastname)"
                Path                  = $_.ou
                EmailAddress          = $_.email
                AccountPassword       = (ConvertTo-SecureString $_.password -AsPlainText -Force)
                ChangePasswordAtLogon = $true
            }
    
            # Create the user by splatting the HashTable
            New-ADUser @newADUser
        }
    
        # Now add user to group here
        # e.g. Add-ADGroupMember ...
    }

    Please take some time to read previous answers. It seems like you aren't reading any of them, you just start over to create another mess. jrv already explained how to use the filter (with double quotes instead of curcly braces).

    • Proposed as answer by jrv Wednesday, August 22, 2018 7:06 PM
    • Marked as answer by Richard MuellerMVP Wednesday, August 29, 2018 1:14 PM
    Tuesday, August 21, 2018 10:03 AM
  • deleted

    Tuesday, August 21, 2018 10:15 AM
  • deleted

    Tuesday, August 21, 2018 12:37 PM
  • Ok I will read some more powershell guidelines and come back in a year  I guess. Thanks for all the help, this was a good start actually.

    I atually learned alot. I really appriciate it :-)

    You can learn correct style and formatting in an afternoon.   You can learn basic PowerShell in less than a weekend.

    1. PowerShell Style Guidelines
    2. Learn PowerShell  
    3. PowerShell Documentation


    \_(ツ)_/

    Tuesday, August 21, 2018 2:35 PM
  • deleted
    Tuesday, August 21, 2018 4:35 PM
  • I cant read very well I need practice to learn. I will pay for some powershell course classes instead.

    Start by taking classes in how to read.  You will need it.

    If it is a language issue, most books are published in multiple languages.


    \_(ツ)_/

    Tuesday, August 21, 2018 4:40 PM
  • deleted

    Wednesday, August 22, 2018 12:56 PM
  • I have many friends with dyslexia that have learned how to read.  It takes professional help but Is well worth it.

    Many famous people including many advanced programmers are dyslexic and have learned how to read.

    For ease of learning you can use the videos.

    My statements were not meant to be critical. They are suggestions to help you understand how to learn PowerShell.

    Start with the video tutorial.   https://mva.microsoft.com/en-us/training-courses/getting-started-with-microsoft-powershell-8276

    It will not matter if you are dyslexic.   You can pause the video at any point to help with reading the screen.


    \_(ツ)_/

    Wednesday, August 22, 2018 7:03 PM
  • Regarding the message you got about your reply being flagged as spam:

    It is a very poorly worded message. Your replies are not marked spam. Instead, your account just needs to be verified. You should reply in this thread with a request to have your account verified:

    https://social.technet.microsoft.com/Forums/en-US/090972cb-b81f-498f-b718-948caca975c4/verify-account-41?forum=reportabug

    Be patient when you open the thread. It is very large and will be slow to load. Unfortunately, this is the only way to get the forum admins to verify your account.


    Richard Mueller - MVP Enterprise Mobility (Identity and Access)

    Wednesday, August 29, 2018 1:21 PM