Get users from groups in active directory, disposing of everything

Question

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<ExpandoObject> GetOuGroupsUsersData(
    PrincipalContext pc)
{
    var data = new List<ExpandoObject>();
    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<ExpandoObject>());
                    }
                }
            }
        }
    }
    return data;
}

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

Answer 1

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

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

3. 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>, 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;
        }
    }

usage:

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

                }

            }

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.

Answer 2

like ExtremeCSharper said, you can remove some of the Curly Braces for the using statements. you should also return as soon as possible, so inside the using statement.

this is how I would clean it up

private IEnumerable<ExpandoObject> GetOuGroupsUsersData(
    PrincipalContext pc)
{
    var data = new List<ExpandoObject>();
    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<ExpandoObject>());
            }
        }    
        return data;
    }
}

The reason that I would write it this way is because I want to return the data before I do the clean up and dispose all the things I just opened.

If I were to write this the old school way, with try/catch/finally it would look like this (I left out the catches because we don't want to catch anything we just want to make sure that everything is disposed of to prevent memory leaks)

private IEnumerable<ExpandoObject> GetOuGroupsUsersData(
    PrincipalContext pc)
{
    var data = new List<ExpandoObject>();
    try {
        var gp = new GroupPrincipal(pc);
        var ps = new PrincipalSearcher(gp);
        var psr = ps.FindAll();
        foreach (var g in psr.GetGroupPrincipals())
        {
            try {
                var gpsr = g.GetMembers();
                data.AddRange(
                    gpsr.GetUserPrincipals().Select(
                        u => GenerateData(g, u))
                        .Cast<ExpandoObject>());
            }
            finally{
                gpsr.Dispose();
            }
        }
        return data;
    }
    finally {
        gp.Dispose();
        ps.Dispose();
        psr.Dispose();
    }
    //Here is where you were returning the variable 
}

To me, it looks right to return from inside the block.

I really want my code to tell a story, I don't want to be burdened with remembering all the clean up stuff, so that is why I use the using blocks, I return as soon as I can because that is the end of the story I am telling. and inside the main using block, after the foreach is the first spot that I can return, no need to keep reading.