locked
Feedback on my code RRS feed

  • General discussion

  • Hello,

    Basically I'm asking for feedback on my code:

    I'm just getting into powershell and currently working on fetching some AD results. I got this piece of code producing expected result, but as a newbie I'm wondering if this is the proper way to script or is there a better way to get the result?

    What I want to achive is to return (name, givenname, surname, True/False) for a list of users in an AD group

    $Group = Get-ADGroup -Identity "Group"
    $users = @(
    	'user1',
    	'user2',
    	'user3',
    	'user4',
    	'user5')
    ForEach ($user In $users){
    	$ADUser = Get-ADUser -Identity $user -Properties *
    	if($ADUser.MemberOf -contains $Group.DistinguishedName) {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"True"}}
    	} else {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"False"}}
       }
    }


    • Changed type jrv Thursday, March 29, 2018 9:57 AM not a question
    Thursday, March 29, 2018 6:58 AM

All replies

  • Very good. You are on the right track.  Keep doing what you are doing.


    \_(ツ)_/

    Thursday, March 29, 2018 7:09 AM
  • Hi plheaugusto,

    Welcome to the Technet forums!

    I have test your script and the results are positive, the information you want to achieve is properly visible. I am wondering in wich context you wanne use this script.

    Are you want to query the name, givenname, surname only from specific users in a group? If you want to have the information of all users in a group, you need to do a small modification to your script.

    $users = Get-ADGroupMember -Identity "Group"
    ForEach ($user In $users){
    	$ADUser = Get-ADUser -Identity $user -Properties *
    	if($ADUser.MemberOf -contains $Group.DistinguishedName) {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"True"}}
    	} else {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"False"}}
       }
    }

    Furthermore, your script for a beginner in Powershell looks good.


    Sincerely, Martien van Dijk. Please remember to mark the replies as answers if they help and unmark them if they provide no help. Check out My Blog!



    Thursday, March 29, 2018 7:09 AM
  • Hi plheaugusto,

    Welcome to the Technet forums!

    I have test your script and the results are positive, the information you want to achieve is properly visible. I am wondering in wich context you wanne use this script.

    Are you want to query the name, givenname, surname only from specific users in a group? If you want to have the information of all users in a group, you need to do a small modification to your script.

    $users = Get-ADGroupMember -Identity "Group"
    ForEach ($user In $users){
    	$ADUser = Get-ADUser -Identity $user -Properties *
    	if($ADUser.MemberOf -contains $Group.DistinguishedName) {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"True"}}
    	} else {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"False"}}
       }
    }

    Furthermore, your script for a beginner in Powershell looks good.


    Sincerely, Martien van Dijk. Please remember to mark the replies as answers if they help and unmark them if they provide no help. Check out My Blog!



    Your code has bugs, it has design flaws..

    #line1: group can be member of a group, then rest of the code will throw an error
    #line4: there is no reference to $group variable

    .. by your logic..

    1. select group => get its members => check membership if they contain a group

    The code doesn't make sense, because if u query users for their group membership, the result will always be True, since u checking only members received by get-adgroupmembership.

    Thursday, March 29, 2018 9:45 AM
  • Hello,

    Basically I'm asking for feedback on my code:

    I'm just getting into powershell and currently working on fetching some AD results. I got this piece of code producing expected result, but as a newbie I'm wondering if this is the proper way to script or is there a better way to get the result?

    What I want to achive is to return (name, givenname, surname, True/False) for a list of users in an AD group

    $Group = Get-ADGroup -Identity "Group"
    $users = @(
    	'user1',
    	'user2',
    	'user3',
    	'user4',
    	'user5')
    ForEach ($user In $users){
    	$ADUser = Get-ADUser -Identity $user -Properties *
    	if($ADUser.MemberOf -contains $Group.DistinguishedName) {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"True"}}
    	} else {
    		$ADUser | Select-Object name, givenname, surname, @{l="Result";e={"False"}}
       }
    }


    just another way

    $group = (get-adgroup "YourGroup").distinguishedname
    $users = @('user1','user2','user3')
    $users | %{get-aduser $_ -properties memberof |select name, givenname,surname,@{n="memberof";e={if($_.memberof -match $group){"true"}else{"False"}}}}
    

    Thursday, March 29, 2018 9:47 AM
  • Minor point: rather than retrieve all properties that have values with Get-ADUser, I would list just the ones you need with the -Properties parameter. This reduces the size of the resultset considerably. Actually, Name, Surname, and GivenName are default properties, so they are always retrieved. So the only property you need to specify with -Properties is memberOf.

    Richard Mueller - MVP Enterprise Mobility (Identity and Access)

    Thursday, March 29, 2018 9:48 AM
  • Minor point: rather than retrieve all properties that have values with Get-ADUser, I would list just the ones you need with the -Properties parameter. This reduces the size of the resultset considerably. Actually, Name, Surname, and GivenName are default properties, so they are always retrieved. So the only property you need to specify with -Properties is memberOf.

    Richard Mueller - MVP Enterprise Mobility (Identity and Access)

    Yes. Forgot to mention that, although it crossed my mind as well.  

    No big deal though, since the usernames are manually typed arraylist, not a query against thousands of users.

    Thursday, March 29, 2018 9:54 AM
  • As I noted above., the code looks good and is on the right track.  The next step would be to run and debug it.

    In my mind it is much better if new users discover these things one at a time and see how things are very interdependent.  The early lessons make for better coders.

    The OP started with well formatted code and a simple approach to the solution.  This is excellent for a first timer.  Better than most who just paste sloppy code with little thought.

    Richard has a good criticism as it would not be discovered in this code. 

    My only critique is to not use "True" and "False" strings.  Use the Booleans $true and $false.  They are more useful.


    \_(ツ)_/

    Thursday, March 29, 2018 9:56 AM