HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Active Directory Query Application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
directoryqueryactiveapplication

Problem

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

  • Save a list of all users to a file.



  • 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 =

Solution

"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.

Context

StackExchange Code Review Q#129093, answer score: 3

Revisions (0)

No revisions yet.