Single responsibility of a function

single-responsibility

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;
            }
        }
    }
}

Best Answer

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?

Related Topic