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

Active Directory Tool - WPF application to query active directory and display results

Submitted by: @import:stackexchange-codereview··
0
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 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 Path to 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 ActiveDirectoryScope for 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 ActiveDirectorySearcher on that scope.



  • ViewModel retrieves the appropriate query from the searcher.



  • ViewModel passes the query results into a DataPreparer with the appropriate attributes/fields to load.



  • DataPreparer iterates through the query results and returns a list of dynamic objects with the appropriate attributes/fields.



  • ViewModel transforms the list into a DataView and hands it to the View's DataGrid.



Notes:

  • For now, the attributes are specified in the form of readonly arrays. 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 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 so

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