Single responsibility of a function

Question

I would like to find out if I understand single responsibility principle correctly.

The function below is suppose to return user ID store in database by using the SAM account name pass into it.

Look at the commented codes below, is it true about the single responsibility of a function/method?

Do this function violate single responsibility principle if those codes are not commented?

public static int GetUserId(string domainName)
{
    string message = "";

    //Do not get badge number from this function,
    //function should stay as single responsibility

    //string badgeNumber = AsmUser.AsmUserHandler.GetBadgeNumber(domainName);
    //if (badgeNumber == null)
    //{
    //    message = "Badge number is null.";
    //    LogHandler.LogDebug(logger, message);
    //    return 0;
    //}
    //else
    {
        using (var db = new SurfaceTreatment.SurfaceTreatmentEntities())
        {
            string domain = domainName.Split('\\')[0].Trim();
            string samAccountName = domainName.Split('\\')[1].Trim();
            var user = db.t_ST_User
                //.Where(_user => string.Compare(
                    // badgeNumber, 
                    // _user.badgeNumber, 
                    // StringComparison.OrdinalIgnoreCase) == 0)
                .Where(_user => string.Compare(
                    domain, 
                    _user.domainName.Trim(), 
                    StringComparison.OrdinalIgnoreCase) == 0)
                .Where(_user => string.Compare(
                    samAccountName, 
                    _user.samAccountName.Trim(), 
                    StringComparison.OrdinalIgnoreCase) == 0)
                .Where(_user => _user.deleted == false)
                    .FirstOrDefault();
            if (user == null)
            {
                message = "Failed to get user by badge number: " + domainName;
                LogHandler.LogDebug(logger, message);
                return 0;
            }
            else
            {
                return user.userId;
            }
        }
    }
}

Answer 1

This functions single responsibility is to get the user id associated with a domain name. Unfortunately in this system that is, apparently, not a simple task. Having a single responsibility doesn't mean the task is simple. It means the task is focused. It should only ever have one reason to change.

My biggest problem with this code is right here:

 message = "Failed to get user by badge number: " + domainName;

There are 3 other ways to get a null user and end up here, yet this log message always blames the badge number. If I was trying to debug this you wouldn't want me knowing where you live.

That is a traceability problem. Not a single responsibility problem. Where this might be a single responsibility problem is if eliminating users based on badgeNumber isn't needed to weed down to one result. As if you were only doing it as some crazy kind of integrity check. That would be a pointless extra responsibility. I mean, what happens if AsmUser.AsmUserHandler.GetBadgeNumber(domainName) decides it wants to be sure the domainName produces a user with a userId? That will just run you in circles. No. Only make badgeNumber part of getting a userId if you really need it to get down to one userId. Do your integrity checks separately.

And Euphoric has a point about returning 0. This looks like C# code. Any reason you're not throwing exceptions?

Answer 2

SRP applies to classes (or modules), not functions, so this is sort of an odd question. I will interpret your question to mean "How can I improve the structure of this code for maintainability?" without reference to any specific principle.

Personally I would consider splitting the function into separate functions under the same class. That way I can have small, narrowly-defined functions that do one specific thing (have a "single responsibility," so to speak). The functions that I don't want exposed I will scope as private.

In this case it's a class about a user, so I'll call it MyUserClass (you should probably change the name).

class MyUserClass
{
    private readonly string _domainNameString;
    private int _badgeNumber = 0;

    public MyUserClass(string domainName)
    {
        _domainNameString = domainName;
    }

    private int GetBadgeNumber()
    {
        var result = AsmUser.AsmUserHandler.GetBadgeNumber(_domainNameString);
        if (result == 0)
        {
            throw ArgumentException(String.Format("No badge number found for domain {0}", _domainNameString), "domainName");
        }
        return result;
    }

    private int GetBadgeNumberWithCache()
    {
        if (_badgeNumber == 0) 
        {
            _badgeNumber = GetBadgeNumber();
        }
        return _badgeNumber;
    }

    private string Domain
    {
        get 
        {
            return _domainNameString.Split('\\')[0].Trim();
        }
    }

    private string AccountName
    {
        get
        {
            return _domainNameString.Split('\\')[1].Trim();
        }
    }

    private bool IsMatch(User candidate)
    {
        return string.Compare(candidate.domainName, this.Domain, StringComparison.OrdinalIgnoreCase) == 0
            && string.Compare(candidate.samAccountName, this.AccountName, StringComparison.OrdinalIgnoreCase) == 0
            && string.Compare(candidate.badgeNumber, this.GetBadgeNumberWithCache(), StringComparison.OrdinalIgnoreCase) == 0;
    }

    private User GetUser()
    {
        using (var db = new SurfaceTreatment.SurfaceTreatmentEntities())
        {
            var user = db.t_ST_User
                .Where(u => !u.deleted && this.IsMatch(u))
                .SingleOrDefault();
            if (user == null)
            {
                throw new InvalidOperationException("Failed to get user.");
            }
            return user;
        }
    }

    public int GetUserID(string domainName)
    {
        return this.GetUser().userId;
    }
}

This approach separates the logic for

• Parsing the domain string to get the domain
• Parsing the domain string to get the user name
• Comparing two users to see if they match
• Calling a service to get the badge number
• Cache service call results to avoid retrieving the same badge number repeatedly
• Interpreting any magic numbers that come back from the service (e.g. 0)

You would then use the code like this:

public static int GetUserID(string domainName)
{
    var u = new MyUserClass(domainName);
    return u.GetUserID();
}

Answer 3

I will approach this from a logic perspective.

The commented code, if ever uncommented (executed), introduces a precondition for the rest of the (existing) code. If GetBadgeNumber returns null, the rest of the existing code wouldn't run. In other words, what the function does is GetUserIdIfBadgeNotNull.

Will this new precondition change the logic and flow of the whole function significantly? The answer to this question depends significantly on the relationship between the badge number data and the string lookup by domain name.

---

Situation 1 - if they are all consistent

What it means is, the following three conditions are either all-true, or all-false.

• domain name exists in database
• badge number is returned from AsmUser.AsmUserHandler.GetBadgeNumber
• badge number and domain name matches what is in the database.

If they are all consistent, it doesn't alter the logic of the function. It can be seen as some database query caching or optimization hack. It also provides a different error message than if the database query didn't return a result.

These are the responsibilities under situation 1: Typically, in order to be consistent with a (live) database, you better let the database handle it. If you intend the first badge number lookup as a cache, then you should perform a lookup with the database even if the cache didn't hit. Finally, you must ensure a domain name is always linked to the same badge number (and vice versa), even if a domain name is briefly deactivated and then re-activated in the system. This typically means you never completely delete the database row; it is simply marked as inactive so that it can be restored if necessary.

---

Situation 2 - if they are not intended to be consistent (same), and the result of GetBadgeNumber is more important than the result from the database query.

This will make AsmUser.AsmUserHandler.GetBadgeNumber behave like a gate keeper. If it returns null, no database query happens.

In this case I would recommend renaming the function to GetUserIdIfBadgeNotNull to make it obvious to other programmers.

---

Situation 3 - if they are not intended to be consistent (same), but GetBadgeNumber is less important than the result from the database query.

Congratulations! You have found / planted a bug in the system.

Answer 4

Depends on what single responsibility you want this function to provide. Looks like the commented out sections were simply enforcing that this function only return a userId of those with badge numbers in your system.

If you rename the method to make it more clear, such as getUserIdWithValidBadgeNumber, it will make your code easier to understand -- but then colleagues that have been taught method names should not be longer than X characters will balk at it.

Also, consider factoring out the snippet that validates the badge number exists, and put that code in another method called hasBadgeNumber or something to that effect. Put the details of how it is determined if the badge number exists or not into that method, and now you have one simple line to check the badge number before proceeding.

This will facilitate maintainability of the code, because it is very likely that there will be more than one place in your code base where you are checking a badge number's existence. If in all those places you are calling this one method, if you ever have to change the underlying data structure which impacts your logic on determining a badge number, you have one place to change to make that happen.

Your code would also benefit by adding dedicated methods to parsing the domain and userid out of domainName such as MYDOMAIN\myId.