7
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I don't see any lines in that snippet that wouldn't fit on the screen if you actually used meaningful variable names. You only use one or two variables per line, and you've still got lots of free horizontal space. \$\endgroup\$ – Servy Jul 18 '16 at 18:11
  • \$\begingroup\$ @Servy Bear in mind that this code in the file is indented twice, once for the namespace and once for the class. After that, the line foreach (var groupPrincipal in principalSearchResult.GetGroupPrincipals()) is indented six times and with the names spelled out like that, is 98 chars long. \$\endgroup\$ – Michael Brandon Morris Jul 18 '16 at 18:16
  • \$\begingroup\$ Then don't intent the code so much; clearly it's not aiding readability. Of course, if you really want to keep them, then just refactor that portion out into a method. \$\endgroup\$ – Servy Jul 18 '16 at 18:20
5
\$\begingroup\$
  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.

\$\endgroup\$
  • \$\begingroup\$ care to join me in The Second Monitor and help me to understand why you don't like my suggestion? I am open to learning more about C# than I already do. thank you \$\endgroup\$ – Malachi Jul 18 '16 at 21:09
  • \$\begingroup\$ @Malachi it's not I don't like your suggestion, it's just not correct because the resource will be disposed just after everything is done inside the context of the using statement, that's why it may run faster for you. Why should we chat, instead of showing our discussion to the OP? Maybe he will be able to validate our suggestions, if he will still be able to see our comments. \$\endgroup\$ – extremecsharper Jul 18 '16 at 21:13
  • \$\begingroup\$ because it is a long conversation that is not necessarily about the OP's code at all \$\endgroup\$ – Malachi Jul 18 '16 at 21:23
  • \$\begingroup\$ @Malachi rather it's necessary to make a discussion here. He could follow your suggestion of returning before the using statement is completely finished and without awareness, which isn't right. Please, elaborate if you need to contradict me. \$\endgroup\$ – extremecsharper Jul 18 '16 at 21:28
  • \$\begingroup\$ @Malachi thank you for strutting about something we've already talked about in chat and for downvoting just because I forgot to edit it after both us realized we were wrong when we saw the IL code. Yes, you were wrong too, as you speculated it was faster. \$\endgroup\$ – extremecsharper Jul 19 '16 at 15:48
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Where you put the return is a bit subjective and largely down to personal preference. The method still has to perform the finally block before data is returned. In this particular situation, I would put the return statement after the using blocks, because where a method has only one return, I like it to be at the end. However where the method has multiple returns (for example parameter validation) I'd probably agree with you and put it in the using block. \$\endgroup\$ – forsvarir Jul 18 '16 at 23:04
  • \$\begingroup\$ Does the assembly look different depending on where the return is (is the behavior different/better either way)? \$\endgroup\$ – Michael Brandon Morris Jul 19 '16 at 11:01
  • 1
    \$\begingroup\$ @MichaelBrandonMorris we talked about it a little over 12 hours ago in Chat. \$\endgroup\$ – Malachi Jul 19 '16 at 11:25
  • \$\begingroup\$ @forsvarir, you are right, it is really a preference, i haven't been able to find anything so far to say that it is a standard or anything. if I were to review the code I would ask "why put the return outside of the using statement?" \$\endgroup\$ – Malachi Jul 19 '16 at 16:41
  • \$\begingroup\$ @Malachi because "the last thing the method needs to do is return its result" :). Don't get me wrong, I think you have a valid point, which is why I voted, but I think where I put the return depends a lot on what the contents of the rest of the method looks like (probably as well as other irrelevant things like what else I've done that day which impact the way I'm thinking etc) \$\endgroup\$ – forsvarir Jul 19 '16 at 16:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.