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

Get users from groups in active directory, disposing of everything

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

Problem

I am trying to get active directory groups with their users, all while disposing of all IDisposable assets when they reach the end of their usefulness. The groups and users are passed into a method that extracts certain properties and adds them to ExpandoObjects. This requires a decent number of nested using blocks.

Since the method is private, and most of the variables only exist in the using blocks, I gave the variables very short names. Naming them properly as groupPrincipal or principalSearchResult would involve breaking even more lines to fit the long names.

private IEnumerable GetOuGroupsUsersData(
    PrincipalContext pc)
{
    var data = new List();
    using (var gp = new GroupPrincipal(pc))
    {
        using (var ps = new PrincipalSearcher(gp))
        {
            using (var psr = ps.FindAll())
            {
                foreach (var g in psr.GetGroupPrincipals())
                {
                    using (var gpsr = g.GetMembers())
                    {
                        data.AddRange(
                            gpsr.GetUserPrincipals().Select(
                                u => GenerateData(g, u))
                                .Cast());
                    }
                }
            }
        }
    }
    return data;
}


Is there anything I can do to reduce the number of using blocks? Am I even disposing of everything properly?

Solution

-
You can remove braces between using statements, leaving the last one. That would help you to remove a few of lines, but still not so deeply.

-
Wrapping the IDisposable interface is a good choice to do as an alternative to a dozen of using statements.

-
I'd suggest you using more appropriate names, maybe by lowercasing just a character of your class/method when you instantiate/call them.

Add this class to your project:

public class IDisposableList : List, IDisposable
    {
        public void Dispose()
        {
            if (this.Count > 0)
            {
                List exceptions = new List();

                foreach (var disposableItem in this)
                {
                    try
                    {
                        disposableItem.Dispose();
                    }
                    catch (Exception ex)
                    {
                        exceptions.Add(ex);
                    }
                }

                base.Clear();

                if (exceptions.Count > 0)
                    throw new AggregateException(exceptions);
            }
        }

        public T Add(Func item) where T : IDisposable
        {
            var disposable = item();
            base.Add(disposable);

            return disposable;
        }
    }


usage:

var data = new List();

            using (var disposables = new IDisposableList())
            {

                var gp = disposables.Add(() => new GroupPrincipal(pc));

                var ps = disposables.Add(() => new PrincipalSearcher(gp));

                var psr = disposables.Add(() => ps.FindAll());

                foreach (var g in psr.GetGroupPrincipals())
                {

                    var gpsr = disposables.Add(() => g.GetMembers());

                    data.AddRange(
                            gpsr.GetUserPrincipals().Select(
                                u => GenerateData(g, u))
                                .Cast());

                }

            }


this is just a more elegant way to do what you asked.

Remember that using a foreach is not as performant as a for loop.

Code Snippets

public class IDisposableList : List<IDisposable>, IDisposable
    {
        public void Dispose()
        {
            if (this.Count > 0)
            {
                List<Exception> exceptions = new List<Exception>();

                foreach (var disposableItem in this)
                {
                    try
                    {
                        disposableItem.Dispose();
                    }
                    catch (Exception ex)
                    {
                        exceptions.Add(ex);
                    }
                }

                base.Clear();

                if (exceptions.Count > 0)
                    throw new AggregateException(exceptions);
            }
        }

        public T Add<T>(Func<T> item) where T : IDisposable
        {
            var disposable = item();
            base.Add(disposable);

            return disposable;
        }
    }
var data = new List<ExpandoObject>();

            using (var disposables = new IDisposableList())
            {

                var gp = disposables.Add(() => new GroupPrincipal(pc));

                var ps = disposables.Add(() => new PrincipalSearcher(gp));

                var psr = disposables.Add(() => ps.FindAll());

                foreach (var g in psr.GetGroupPrincipals())
                {

                    var gpsr = disposables.Add(() => g.GetMembers());

                    data.AddRange(
                            gpsr.GetUserPrincipals().Select(
                                u => GenerateData(g, u))
                                .Cast<ExpandoObject>());

                }

            }

Context

StackExchange Code Review Q#135208, answer score: 5

Revisions (0)

No revisions yet.