3
\$\begingroup\$

This application is designed to query an active directory, and at the moment, performs only two tasks:

  1. Save a list of all users to a file.
  2. Save a list of all groups that all users are in to a file.

I tried to implement a print all groups method, but ended up removing it. I believe I removed all references to it from this code, but if you see one, please ignore (or fuss at me for not scrutinizing enough).

The method that saves all user groups uses a background worker due to how much longer it takes to run. I understand I should probably add a background worker for the save users method too.

Users are able to change both the domain and the organizational units using text boxes.

ActiveDirectoryTool.cs:

public partial class ActiveDirectoryTool : Form
{
    private Backend backend;

    public ActiveDirectoryTool()
    {
        InitializeComponent();
        backend = new Backend();
        UpdateDisplay();
    }

    private void getAllUsers_Click(object sender, EventArgs e)
    {
        this.Enabled = false;
        if (backend.PrintAllUsersToFile())
        {
            this.Enabled = true;
        }
    }

    private void printAllUserGroups_Click(object sender, EventArgs e)
    {
        if (!backgroundWorker1.IsBusy)
        {
            this.Enabled = false;
            backgroundWorker1.RunWorkerAsync();
        }
    }

    private void backgroundWorker1_DoWork(object sender, DoWorkEventArgs e)
    {
        backend.PrintAllUserGroupsToFile();
    }

    private void backgroundWorker1_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
        this.Enabled = true;
    }

    private void UpdateBackend()
    {
        backend.Domain = domainBox.Text;
        backend.OrganizationalUnits = organizationalUnitBox.Text;
        backend.WriteSettings();
    }

    private void UpdateDisplay()
    {
        domainBox.Text = backend.Domain;
        organizationalUnitBox.Text = backend.OrganizationalUnits;
        scopeDisplay.Text = backend.Scope;
    }

    private void organizationalUnitBox_TextChanged(object sender, EventArgs e)
    {
        UpdateBackend();
        UpdateDisplay();
    }

    private void domainBox_TextChanged(object sender, EventArgs e)
    {
        UpdateBackend();
        UpdateDisplay();
    }
}

Backend.cs:

internal class Backend
{
    private const string DefaultDomain = "dnet.domtar";
    private const string CommaSeparatedValuesExtension = ".csv";
    private const string PlainTextExtension = ".txt";
    private const string TabSeparatedValuesExtension = ".tsv";
    private const string DefaultExtension = PlainTextExtension;
    private const string DateTimeFormat = "yyyyMMddTHHmmss";
    private const char Hyphen = '-';
    private const char Tab = '\t';
    private const char Comma = ',';
    private const string UserListHeader = "Last\tFirst\tDisplay Name\tID\tActive\tLocked\tDescription\tHome Drive\tHome Folder\tLogin Script\tEmail\tStreet\tCity\tState\tPhone\tDistinguished Name";
    private const string GroupListHeader = "Group Name\tGroup ID\tManaged By\tDescription\tDistinguished Name";
    private const string UserGroupListHeader = "User ID\tGroup\tUser Full Name\tUser Distinguished Name";
    private string path = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments), "ActiveDirectoryTool");

    public Backend()
    {
        if (!Directory.Exists(path))
        {
            Directory.CreateDirectory(path);
        }
        ReadSettings();
    }

    private string domain;
    private string domainContext;
    private string organizationalUnits;

    public string Domain
    {
        get
        {
            return domain;
        }

        set
        {
            domain = value;
        }
    }

    public string OrganizationalUnits
    {
        get
        {
            return organizationalUnits.Replace("OU=", "").Replace(",", " ");
        }

        set
        {
            organizationalUnits = "OU=" + value.Replace(" ", ",OU=");
        }
    }

    private DirectoryEntry DefaultRootDirectory
    {
        get
        {
            return new DirectoryEntry("LDAP://RootDSE");
        }
    }

    private DirectoryEntry RootDirectory
    {
        get
        {
            if (!String.IsNullOrWhiteSpace(Domain) && !Domain.Equals(DefaultDomain))
            {
                return new DirectoryEntry("LDAP:://" + Domain);
            }
            else
            {
                return DefaultRootDirectory;
            }
        }
    }

    public string DomainContext
    {
        get
        {
            if (String.IsNullOrWhiteSpace(domainContext))
            {
                domainContext = GetDefaultDomainContext();
            }
            return domainContext;
        }
    }

    public string Scope
    {
        get
        {
            return organizationalUnits + Comma + DomainContext;
        }
    }

    private string GetDefaultDomainContext()
    {
        return RootDirectory.Properties["defaultNamingContext"].Value as string;
    }

    private PrincipalContext GetPrincipalContext()
    {
        return new PrincipalContext(ContextType.Domain, Domain, Scope);
    }

    public void WriteSettings()
    {
        ConfigurationManager.AppSettings["domain"] = domain;
        ConfigurationManager.AppSettings["organizationalUnits"] = organizationalUnits;
    }

    private void ReadSettings()
    {
        domain = ConfigurationManager.AppSettings["domain"];
        organizationalUnits = ConfigurationManager.AppSettings["organizationalUnits"];
    }

    private void ShowException(Exception e)
    {
        MessageBox.Show("Exception: " + e.Message + "\n" + e.StackTrace);
    }

    private string GenerateFilename(string fileType)
    {
        return fileType + Hyphen + DateTime.Now.ToString(DateTimeFormat) + DefaultExtension;
    }

    private bool IsActive(DirectoryEntry de)
    {
        if (de.NativeGuid == null) return false;

        int flags = (int)de.Properties["userAccountControl"].Value;

        return !Convert.ToBoolean(flags & 0x0002);
    }

    private string ConcatenateWithTabs(params string[] strings)
    {
        string concatenated = "";
        foreach (string s in strings)
        {
            concatenated += s + Tab;
        }
        return concatenated;
    }

    private string UserAsString(UserPrincipal user)
    {
        DirectoryEntry entry = user.GetUnderlyingObject() as DirectoryEntry;
        return ConcatenateWithTabs(
            user.Surname,
            user.GivenName,
            user.DisplayName,
            user.SamAccountName,
            IsActive(entry).ToString(),
            user.IsAccountLockedOut().ToString(),
            user.Description,
            user.HomeDrive,
            user.HomeDirectory,
            user.ScriptPath,
            user.EmailAddress,
            (string)entry.Properties["streetAddress"].Value,
            (string)entry.Properties["l"].Value,
            (string)entry.Properties["st"].Value,
            user.VoiceTelephoneNumber,
            user.DistinguishedName);
    }

    private string UserGroupAsString(UserPrincipal user, GroupPrincipal group)
    {
        return ConcatenateWithTabs(
            user.SamAccountName,
            group.SamAccountName,
            user.Name,
            user.DistinguishedName);
    }

    public bool PrintAllUsersToFile()
    {
        try
        {
            int count = 0;
            string filename = Path.Combine(path, GenerateFilename("AllUsers"));
            using (var searcher = new PrincipalSearcher(new UserPrincipal(GetPrincipalContext())))
            {
                using (StreamWriter sr = new StreamWriter(filename))
                {
                    sr.WriteLine(UserListHeader);
                    foreach (var result in searcher.FindAll())
                    {
                        sr.WriteLine(UserAsString((UserPrincipal)result));
                        count++;
                    }
                }
            }
            MessageBox.Show("All (" + count + ") users saved to file " + filename);
        }
        catch (Exception e)
        {
            ShowException(e);
        }
        return true;
    }

    public bool PrintAllUserGroupsToFile()
    {
        try
        {
            int count = 0;
            string filename = Path.Combine(path, GenerateFilename("UserGroups"));
            using (var searcher = new PrincipalSearcher(new UserPrincipal(GetPrincipalContext())))
            {
                using (StreamWriter sr = new StreamWriter(filename))
                {
                    sr.WriteLine(UserGroupListHeader);
                    foreach (UserPrincipal user in searcher.FindAll())
                    {
                        foreach (GroupPrincipal group in user.GetGroups())
                        {
                            sr.WriteLine(UserGroupAsString(user, group));
                        }
                        count++;
                    }
                }
            }
            MessageBox.Show("All (" + count + ") users with groups saved to file " + filename);
        }
        catch (Exception e)
        {
            ShowException(e);
        }
        return true;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you using C# 6.0? because if you are, you could use string interpolation instead of concatenating all your variables \$\endgroup\$ – Malachi May 23 '16 at 14:25
3
\$\begingroup\$

"Backend" is a compound word and thus the correct capitalization should be "BackEnd". However, that would still be a fairly meaningless name for a class, even for a namespace.


Your Backend class starts out with a dozen const; I'd put those in a separate (static) class.


Strings like "domain" and "organizationalUnits" and "OU=" get used multiple times, so they too should be const strings, and of course then moved to that static class mentioned earlier.


Why is there a method ShowException in your back-end code? Separate your UI from your logic.

The contents of that method are also up for debate: showing the stack trace?


You do a lot of string concatenation where perhaps string.Format() would be more appropriate, e.g. return fileType + Hyphen + DateTime.Now.ToString(DateTimeFormat) + DefaultExtension;.


ConcatenateWithTabs seems to reinvent string.Join() for some reason?


MessageBox.Show also is present in PrintAllUserGroupsToFile and PrintAllUsersToFile, and it shoudln't be: decouple your UI from your back-end.

\$\endgroup\$
  • \$\begingroup\$ Thanks. Somehow missed that String.Join() was a thing, and String.Format() is more eloquent than my multi-concatenations. I'll remove the UI code from the back-end and try to rename the back-end to something better. ActiveDirectoryToolBackEnd? :D \$\endgroup\$ – Michael Brandon Morris May 23 '16 at 13:27
2
\$\begingroup\$

First thing that I saw that you could make a little easier to read and less verbose is this

private void getAllUsers_Click(object sender, EventArgs e)
{
    this.Enabled = false;
    if (backend.PrintAllUsersToFile())
    {
        this.Enabled = true;
    }
}

you are setting this.Enabled to whatever backend.PrintAllUsersToFile() returns, which I am assuming is a boolean value.

So just set this.Enabled to backend.PrintAllUsersToFile() like this

private void getAllUsers_Click(object sender, EventArgs e)
{
    this.Enabled = backend.PrintAllUsersToFile();
}

but it appears that you want the button disabled between the click and the function return so we need to have the this.Enabled = false; in there.

private void getAllUsers_Click(object sender, EventArgs e)
{
    this.Enabled = false;
    this.Enabled = backend.PrintAllUsersToFile();
}

Also, I noticed somewhere that you could use some syntactic sugar to reduce indentations

public bool PrintAllUsersToFile()
{
    try
    {
        int count = 0;
        string filename = Path.Combine(path, GenerateFilename("AllUsers"));
        using (var searcher = new PrincipalSearcher(new UserPrincipal(GetPrincipalContext())))
        {
            using (StreamWriter sr = new StreamWriter(filename))
            {
                sr.WriteLine(UserListHeader);
                foreach (var result in searcher.FindAll())
                {
                    sr.WriteLine(UserAsString((UserPrincipal)result));
                    count++;
                }
            }
        }
        MessageBox.Show("All (" + count + ") users saved to file " + filename);
    }
    catch (Exception e)
    {
        ShowException(e);
    }
    return true;
}

When you have 2 using statements in a row you can "group" them together and use a single set of curly braces like this

public bool PrintAllUsersToFile()
{
    try
    {
        int count = 0;
        string filename = Path.Combine(path, GenerateFilename("AllUsers"));
        using (var searcher = new PrincipalSearcher(new UserPrincipal(GetPrincipalContext())))
        using (StreamWriter sr = new StreamWriter(filename))
        {
            sr.WriteLine(UserListHeader);
            foreach (var result in searcher.FindAll())
            {
                sr.WriteLine(UserAsString((UserPrincipal)result));
                count++;
            }
        }
        MessageBox.Show("All (" + count + ") users saved to file " + filename);
    }
    catch (Exception e)
    {
        ShowException(e);
    }
    return true;
}

if you are using C# 6.0 you can use some more syntactic sugar in the way of string interpolation on some of your messages or string concatenation, it may make things a little less cluttered.

MessageBox.Show("All (" + count + ") users saved to file " + filename);

Would become this instead

MessageBox.Show($"All ({count}) users saved to file {filename}");

And your ShowException Method

private void ShowException(Exception e)
{
    MessageBox.Show("Exception: " + e.Message + "\n" + e.StackTrace);
}

would look like this

private void ShowException(Exception e)
{
    MessageBox.Show($"Exception: {e.Message}\n {e.StackTrace}");
}

I also noticed that in your Constructor for the Backend class that you can get rid of the if statement as well

public Backend()
{
    if (!Directory.Exists(path))
    {
        Directory.CreateDirectory(path);
    }
    ReadSettings();
}

turns out that the Directory.CreateDirectory() Method creates a directory unless it already exists, so you don't need the if statement at all.

public Backend()
{
    Directory.CreateDirectory(path);
    ReadSettings();
}
\$\endgroup\$
  • \$\begingroup\$ Maybe it's just me, but I find that braces make the code easier to read. I doubt the two extra lines and four extra spaces per line are a major inconvenience to anyone working on the code, and this is C#, so file size is not an issue. \$\endgroup\$ – Michael Brandon Morris May 23 '16 at 13:20
  • 1
    \$\begingroup\$ it is a good habit to group using statements like this, otherwise you may have 4 in a row and have 4 indentations just to see what the code is. it's a style thing, and syntactic sugar like I said. personally I think that too many extraneous indentations looks messy. indenting a block of code should only happen when a new decision is being made, here there isn't really a decision all the usings are being used for the same purpose. really it is personal preference or company standards I would say. \$\endgroup\$ – Malachi May 23 '16 at 13:24
  • \$\begingroup\$ @RubberDuck Care to post an answer with a preferable way to do it? \$\endgroup\$ – Michael Brandon Morris May 23 '16 at 13:42

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.