Create CSV file of domain groups and their users

Question

I'm fairly new to scripting in Python. I'm looking for suggestions on improving any of the following in the below script: my coding style, performance, readability, commenting, or anything else.

"""
META
Author: James Nicholson
Date: 8/16/2013

DESCRIPTION
This script takes a list of groups from a text file and creates a new text file
in CSV format that contains group members in the below format:

group_name1,user_name1
group_name1,user_name2
etc...

VARIABLES
server = 'domain name'
glist = 'path to text file containing desired groups'
"""

# Import the win32 module needed for accessing groups and members
import win32net

# Set server name and group_list text file variables
server = # enter server name here
group_list = 'groups.txt'
grps = []

# Loop over lines in group_list and store group names in a list
with open(group_list, 'rb') as my_groups:
    for line in my_groups:
        grps.append(line.rstrip('\r\n'))

# Loop over groups and store members in desired format in new text file 'groups_members_output.txt'
with open('groups_members_output.txt', 'wb') as blank_file:
    for group in grps:
        usrs = win32net.NetGroupGetUsers(server,group,0)
        for user in usrs[0]:
            blank_file.write('{},{}\r\n'.format(group,user['name']))

Answer 1

Some notes:

• Comments: A lot (if not all) of the comments are pretty much repeating what the code already says. I'd remove them.

• server = # enter server name here. Create functions with configurable elements as arguments instead of global variables.

• with open('groups_members_output.txt', 'wb') as blank_file. Try to give more meaningful names to variables, be declarative. blank_file says hardly anything useful about the variable.

• grps/usrs: Just for saving one or two characters now variables are harder to read.

• Put spaces after commas.

• Use (functional) list-comprehensions expressions instead of (imperative) for-loop statements whenever possible.

• Use always "\n" for line endings (read and write), Python takes care of portability in all platforms.

I'd write:

import win32net

def write_group_members(server, input_groups, output_groups):
    with open(input_groups) as input_groups_fd:
        groups = [line.rstrip() for line in input_groups_fd]

    with open(output_groups, "w") as output_groups_fd:
        for group in groups:
            users = win32net.NetGroupGetUsers(server, group, 0)[0]
            for user in users:
                contents = "{0},{1}\n".format(group, user["name"])
                output_groups_fd.write(contents)

write_group_members("someserver.org", "groups.txt", "groups_members_output.txt")

Answer 2

The code looks good (it's short, which is always a good thing).

Make sure to follow PEP 8. A recent revision of PEP 8 allowed 99 characters lines to "improve readability", but your # Loop over ... comment would still be easier to read on small screens (or on CodeReview!) if splitted.

An issue I often deal with myself in for entity in collection statements is naming properly entity and collection when you really can't provide distinct names. I don't think group/grps is a good choice. I'd suggest group/group_list, while renaming group_list to group_filename or something similar. PEP 20 says "Explicit is better than implicit." very early.

Your comments could be improved too: a common advice to beginners is to avoid stating what the line i (eg. # increment n), but say what it means (eg. # adding a new unit). You're actually doing both: you just have to remove the useless part!

• # Loop over lines in group_list and store group names in a list becomes # Store group names in a list
• # Import the win32 module needed for accessing groups and members becomes # Accessing groups and members.