patterncsharpMinor
Active Directory Tool - WPF application to query active directory and display results
Viewed 0 times
directoryactiveapplicationwpfqueryandresultsdisplaytool
Problem
This is a Windows Presentation Foundation application that queries active directory and can display lists of users, users by group, groups, and users by manager (direct reports). It operates on whatever active directory server the host computer belongs to, and dynamically builds a
Once a user has selected an Organizational Unit, they can use one of the buttons to start a query. The results of the query are then displayed in a
Complete Process:
Notes:
TreeView of all the Organizational Units in the active directory for the user to navigate through.Once a user has selected an Organizational Unit, they can use one of the buttons to start a query. The results of the query are then displayed in a
DataGrid.Complete Process:
- Application Launch
- During the launch, the host AD server is contacted and a list of all OUs is retrieved.
- The OU list is parsed into a
TreeView. Since the OUs are in the form of LDAP paths, this process involves:
- Storing the whole path as
Pathto be used later.
- Stripping the prefix, domain components, and OU individual prefixes, splitting into an array of individual OUs, and flipping the array (LDAP paths are "backwards").
- Creating an
ActiveDirectoryScopefor each individual OU, and assembling into a tree.
- Application Use
- User selects a scope from the
TreeView.
- User selects a query.
- View notifies ViewModel that query has been requested.
- ViewModel retrieves current scope, and opens an
ActiveDirectorySearcheron that scope.
- ViewModel retrieves the appropriate query from the searcher.
- ViewModel passes the query results into a
DataPreparerwith the appropriate attributes/fields to load.
DataPrepareriterates through the query results and returns a list of dynamic objects with the appropriate attributes/fields.
- ViewModel transforms the list into a
DataViewand hands it to the View'sDataGrid.
Notes:
- For now, the attributes are specified in the form of
readonlyarrays. This will be changed later to user input.
- The querying process will later be run in a background thread so that the UI doesn't freeze.
- I know my implementation of MVVM is incorrect
Solution
Only scratching the surface...
ActiveDirectoryScope
The
Right now you are acessing that property at least four times and if an
So changing to
will result in only one call to
But this can still be improved by extracting the creation of the
ActiveDirectorySearcher
The almost silently swallowing of the exception in GetUserGroupsFromUsers() isn't that good, in addition catching Exception only shouldn't be done either. You should always catch the most narrowed down exception.
As you have commented
The exception catching is because of some error in the client's active directory. It is an "Unknown Error" that only throws an Exception
I would like to suggest to add a comment to that catching of
internal static class Extensions
here you already have a
ActiveDirectoryScope
The
AddDirectoryScope() method can be improved by storing the value of the organizationalUnit.Split property (which by the way reads more like a method) into a variable.Right now you are acessing that property at least four times and if an
ActiveDirectoryScope with the level isn't contained in parent.Children it will be called one more time.So changing to
internal void AddDirectoryScope(OrganizationalUnit organizationalUnit)
{
string[] levels = organizationalUnit.Split;
if (levels == null ||
levels item.Name.Equals(level));
}
else if (level == levels[lastLevel])
{
parent.Children.Add(new ActiveDirectoryScope
{
Name = level,
Path = organizationalUnit.Path
});
}
}
}will result in only one call to
Split. I used the name levels but I am unsure if that will be good. I just went along with level.But this can still be improved by extracting the creation of the
ActiveDirectoryScope out of the if statement like sointernal void AddDirectoryScope(OrganizationalUnit organizationalUnit)
{
string[] levels = organizationalUnit.Split;
if (levels == null ||
levels item.Name.Equals(level));
}
else if (level == levels[lastLevel])
{
scope.Path = organizationalUnit.Path;
parent.Children.Add(scope);
}
}
}ActiveDirectorySearcher
The almost silently swallowing of the exception in GetUserGroupsFromUsers() isn't that good, in addition catching Exception only shouldn't be done either. You should always catch the most narrowed down exception.
As you have commented
The exception catching is because of some error in the client's active directory. It is an "Unknown Error" that only throws an Exception
I would like to suggest to add a comment to that catching of
Exception so it is clear to any future reader of your code why you are doing it in the way it is.internal static class Extensions
var data = items.ToArray();
if (!data.Any())
{
return null;
}here you already have a
dynamic[] so you should use data.Length == 0 which doesn't involve an enumerator like the Any() method.Code Snippets
internal void AddDirectoryScope(OrganizationalUnit organizationalUnit)
{
string[] levels = organizationalUnit.Split;
if (levels == null ||
levels < 1)
{
throw new ArgumentException(
"The organizational units array is null or empty!");
}
var parent = this;
var lastLevel = levels.Length - 1;
foreach (var level in levels)
{
if (parent.Children.Contains(new ActiveDirectoryScope
{
Name = level
}))
{
parent = parent.Children.Find(
item => item.Name.Equals(level));
}
else if (level == levels[lastLevel])
{
parent.Children.Add(new ActiveDirectoryScope
{
Name = level,
Path = organizationalUnit.Path
});
}
}
}internal void AddDirectoryScope(OrganizationalUnit organizationalUnit)
{
string[] levels = organizationalUnit.Split;
if (levels == null ||
levels < 1)
{
throw new ArgumentException(
"The organizational units array is null or empty!");
}
var parent = this;
var lastLevel = levels.Length - 1;
foreach (var level in levels)
{
var scope = new ActiveDirectoryScope
{
Name = level
}
if (parent.Children.Contains(scope)
{
parent = parent.Children.Find(
item => item.Name.Equals(level));
}
else if (level == levels[lastLevel])
{
scope.Path = organizationalUnit.Path;
parent.Children.Add(scope);
}
}
}var data = items.ToArray();
if (!data.Any())
{
return null;
}Context
StackExchange Code Review Q#132312, answer score: 3
Revisions (0)
No revisions yet.