4
\$\begingroup\$

I am currently using this code, but I am pretty sure there is a better way of doing it:

private string ReturnEmployeeName(string userName)
{
    try
    {
        // Username coming is     RandomDomain\RandomLengthUsername.whatever
        string[] sAMAccountName = userName.Split('\\');
        return sAMAccountName[1];

How can I make this faster? I am not sure if my try block will catch any exceptions that may arise because of this line.

\$\endgroup\$
  • 1
    \$\begingroup\$ If you're talking about catching exceptions, you should also show us your catch block. \$\endgroup\$ – svick Oct 11 '12 at 9:36
  • \$\begingroup\$ Its just a simple catch exception that logs "exception ex" to event receiver, however main concern is splitting string and getting what I want :) \$\endgroup\$ – Muhammad Raja Oct 11 '12 at 9:46
  • \$\begingroup\$ Including the catch block would also tell us what you return if you don't find a proper name. \$\endgroup\$ – ANeves Oct 11 '12 at 10:15
  • \$\begingroup\$ just passing back a Constant which is just a generic default user name address \$\endgroup\$ – Muhammad Raja Oct 11 '12 at 10:26
4
\$\begingroup\$

"Fast" is not too relevant here, since it's such a simple method.
And the canned response also applies: if you want to improve the speed, first do some benchmarking to know where the bottlenecks really are.

But here my proposal of a better way:

private const string DefaultEmployeeName = "JDoe";
/// <param name="userName">Format expected: "Domain\Name"</param>
private string ParseEmployeeName(string userName) {
    if (userName == null) {
        return DefaultEmployeeName;
    }
    // Username coming is "RandomDomain\RandomLengthUsername.whatever"
    // Let's split by '\', returning maximum 2 substrings.
    // The second (n) will contain everything from the first (n-1) separator onwards.
    string[] parts = userName.Split(new char[] { '\\' }, 2);
    if (parts.Length < 2) {
        return DefaultEmployeeName;
    }
    // Let's remove whitespace, just in case.
    string name = parts[1].Trim();
    if (string.IsNullOrEmpty(name)) {
        return DefaultEmployeeName;
    }
    return name; // "RandomLengthUsername.whatever"
}

The comment with /// on top is a "documentation comment".
Those will enrich the Intellisense view of the method, providing extra information.

\$\endgroup\$
  • \$\begingroup\$ looks good, can you add few comments on each line as whats happening as well please as I am bit confused with code lines as "string[] parts = userName.Split(new char[] { '\\' }, 2);" or "string name = parts[1] ?? "";" Cheers \$\endgroup\$ – Muhammad Raja Oct 11 '12 at 10:29
  • 1
    \$\begingroup\$ ?? is the null-coalescing operator. Doing string name = user.name ?? "John Doe" will store in name the value in user.name, or "John Doe" if null. (You should be aware that an empty string is not null.) I removed it from my code in the answer, because string.Split will never return nulls. (Woops!) \$\endgroup\$ – ANeves Oct 11 '12 at 11:57
7
\$\begingroup\$

Personally, I think you would be better off moving this code into a separate class as it's probably logic you are going to want in multiple places in your application and is most likely violating SRP (Single Responsibility Principle) wherever it is at the moment.

If we create a username class, we can encapsulate the logic in there and then re-use it from wherever we want in the application:

public class UserName
{
    private UserName() {}
    public string AccountName { get; private set; }
    public string DomainName { get; private set; }

    public static UserName Parse(string userName)
    {
        if (userName == null)
        {
            throw new ArgumentNullException("userName");
        }

        if (userName.Contains(@"\"))
        {
            var parts = userName.Split('\\');

            return new UserName { DomainName = parts[0], AccountName = parts[1] };
        }

        if (userName.Contains("@"))
        {
            var parts = userName.Split('@');

            return new UserName { DomainName = parts[1], AccountName = parts[0] };
        }

        return new UserName { AccountName = userName };
    }
}

You can then test the logic for this class in isolation:

public class UserNameTests
{
    public class WhenCallingParseWithANullUserName
    {
        [Fact]
        public void AnArgumentNullExceptionShouldBeThrown()
        {
            Assert.Throws<ArgumentNullException>(() => UserName.Parse(null));
        }
    }

    public class WhenCallingParseWithAUserNameWithNoDomainName
    {
        private UserName userName;

        public WhenCallingParseWithAUserNameWithNoDomainName()
        {
            userName = UserName.Parse("JohnSmith");
        }

        [Fact]
        public void TheDomainNameShouldBeNull()
        {
            Assert.Null(userName.DomainName);
        }

        [Fact]
        public void TheUserNameShouldEqualTheWholeUserName()
        {
            Assert.Equal("JohnSmith", userName.AccountName);
        }
    }

    public class WhenCallingParseWithDomainSlashUserName
    {
        private UserName userName;

        public WhenCallingParseWithDomainSlashUserName()
        {
            userName = UserName.Parse(@"Domain\JohnSmith");
        }

        [Fact]
        public void TheDomainNameShouldBeSetToTheDomainName()
        {
            Assert.Equal("Domain", userName.DomainName);
        }

        [Fact]
        public void TheUserNameShouldBeSetToTheUserName()
        {
            Assert.Equal("JohnSmith", userName.AccountName);
        }
    }

    public class WhenCallingParseWithUserNameAtDomain
    {
        private UserName userName;

        public WhenCallingParseWithUserNameAtDomain()
        {
            userName = UserName.Parse(@"JohnSmith@Domain");
        }

        [Fact]
        public void TheDomainNameShouldBeSetToTheDomainName()
        {
            Assert.Equal("Domain", userName.DomainName);
        }

        [Fact]
        public void TheUserNameShouldBeSetToTheUserName()
        {
            Assert.Equal("JohnSmith", userName.AccountName);
        }
    }
}
\$\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.