Return to Revisions

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 < 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
 });
 }
 }
 } 

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 < 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);
 }
 }
 }

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.