4
\$\begingroup\$

I have many different methods like ones below, but is there way I could improve them and remove duplicate code ?

    public static ADUser Load(string userName)
    {
        using (PrincipalContext pc = MyUtilities.GetPrincipalContext())
        using (UserPrincipalExtension user = UserPrincipalExtension.FindByIdentity(pc, IdentityType.SamAccountName, userName))
        {
            if (user != null)
            {
                return Utilities.LoadUser(user);
            }
        }

        return null;
    }

    public static ADUser LoadBySid(string sid)
    {
        using (PrincipalContext pc = MyUtilities.GetPrincipalContext())
        using (UserPrincipalExtension user = UserPrincipalExtension.FindByIdentity(pc, IdentityType.Sid, sid))
        {
            if (user != null)
            {
                return Utilities.LoadUser(user);
            }
        }

        return null;
    }
\$\endgroup\$
6
\$\begingroup\$

I have limited knowledge in C#, but could you not extract something like this?

private static ADUser InternalLoad( IdentityType type, string identity ) {
    using (PrincipalContext pc = MyUtilities.GetPrincipalContext())
    using (UserPrincipalExtension user = UserPrincipalExtension.FindByIdentity(pc, type, identity))
    {
        if (user != null)
        {
            return Utilities.LoadUser(user);
        }
    }

    return null;
}
\$\endgroup\$
  • \$\begingroup\$ can I further breakdown PC and UPE so I can get UPE from another method ? thanks \$\endgroup\$ – Mathematics Sep 21 '15 at 7:08
  • \$\begingroup\$ @PleaseTeach I'm not sure I understand. Do you mean taking in your UserPrincipalExtension as a parameter, or returning it as a result? Either should be pretty easy. If not, please elaborate! \$\endgroup\$ – Jacob Raihle Sep 21 '15 at 8:04
5
\$\begingroup\$

The only suggestions I would give above the very good answer from Jacob is following:

Variable names:

Avoid using names like pc, they don't have any meaning, nor to you, nor to other people looking at your code. In this case, replace it with context for example.

InternalLoad is also not the right name for a method that returns an ADUser object, try GetInternalUser for example.


The var keyword:

From the C# Programming Guide:

The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

using (PrincipalContext context = MyUtilities.GetPrincipalContext())

would become:

using (var context = MyUtilities.GetPrincipalContext())

This is not mandatory, but it might be used when the type is already known.


Complete code:

private static ADUser GetInternalUser(IdentityType type, string identity)
{
    using (var context = MyUtilities.GetPrincipalContext())
    using (var user = UserPrincipalExtension.FindByIdentity(pc, type, identity))
    {
        if (user != null)
        {
            return Utilities.LoadUser(user);
        }
    }
    return null;
}
\$\endgroup\$

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.