7
\$\begingroup\$

I have a very simple helper class to validate credentials. Sometimes the domain has a value, sometimes not. Depending on this, a different constructor for PrincipalContext must be used. PrincipalContext is disposable, so I am using using. It looks ugly, and recommendations?

public class LDAPManager : ILDAPManager
{
    public bool ValidCredentials(string username, string password, string domain)
    {
        using (var context = string.IsNullOrWhiteSpace(domain) ? new PrincipalContext(ContextType.Domain) : new PrincipalContext(ContextType.Domain, domain))
        {
            return context.ValidateCredentials(username, password);
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

As the constructor of PrincipalContext which only takes a ContextType as parameter internally calls this constructor

public PrincipalContext(ContextType contextType, string name, string container,  
      ContextOptions options, string userName, string password){}  

by passing null for the name I would change your method to

public class LDAPManager : ILDAPManager
{
    public bool ValidCredentials(string username, string password, string domain)
    {
        if (string.IsNullOrWhiteSpace(domain)) {
            domain = null;
        }
        using (var context = new PrincipalContext(ContextType.Domain, domain))
        {
            return context.ValidateCredentials(username, password);
        }
    }
}  

based on the valid comment of Lightness Races in Orbit it can be that this implementation will change sometime. So be careful.

After getting this comment, I also noticed some naming issue with this method. Based on the naming guidlines, you should use verbs or verb phrases to name your methods.
So the method would be better named ValidateCredentials().

\$\endgroup\$
  • \$\begingroup\$ Thank you. Didn't think to check how the constructor works internally. \$\endgroup\$ – Espo Oct 17 '14 at 9:45
  • \$\begingroup\$ Just be careful - this somewhat relies on implementation details. Is it guaranteed by the interface of PrincipalContext? Is it stated in the documentation? If not, I wouldn't go down this route. \$\endgroup\$ – Lightness Races with Monica Oct 17 '14 at 13:27
  • \$\begingroup\$ @LightnessRacesinOrbit You are right, as these sources aren't avaible. I have updated my answer. \$\endgroup\$ – Heslacher Oct 17 '14 at 13:41
  • \$\begingroup\$ @LightnessRacesinOrbit The documentation does explain what null name means. \$\endgroup\$ – svick Oct 18 '14 at 17:13
  • \$\begingroup\$ @svick: Yep this looks fine then. Actually it would appear to contradict the question as the name parameter is not optional and there is only one constructor? But perhaps that's just me not understanding C# (as I am not a C# developer). Carry on :) \$\endgroup\$ – Lightness Races with Monica Oct 18 '14 at 17:27
3
\$\begingroup\$

If you really want to run with one method, I'd use some spacing.

However, since you're essentially using "domain" as an optional parameter. Consider two methods instead. This will require a change to your interface, probably.

Also, there's no reason this could not be a static method, except for the fact you have it in an interface.

Finally, a class with one method is not really doing much. Is there somewhere else this could be better placed? Perhaps as an extension method?

With all that in mind:

If you're hell-bent on sticking with one method:

public class LDAPManager : ILDAPManager
{
    public bool ValidCredentials(string username, string password, string domain)
    {
        using (var context = string.IsNullOrWhiteSpace(domain) ? 
            new PrincipalContext(ContextType.Domain) : 
            new PrincipalContext(ContextType.Domain, domain))
        {
            return context.ValidateCredentials(username, password);
        }
    }
}

If you want a more sensible multiple method system

public class LDAPManager : ILDAPManager
{
    public bool ValidCredentials(string username, string password, string domain)
    {
        using (var context = new PrincipalContext(ContextType.Domain, domain))
        {
            return context.ValidateCredentials(username, password);
        }
    }

    public bool ValidCredentials(string username, string password)
    {
        using (var context = new PrincipalContext(ContextType.Domain))
        {
            return context.ValidateCredentials(username, password);
        }
    }
}
\$\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.