3

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;
            }
        }
    }
}
  • I think it would help if you explained the purpose of the different parts of the program. – Adam B Nov 6 '17 at 4:01
  • I would first question return of 0 and the message thing. – Euphoric Nov 6 '17 at 5:22
  • SRP doesn't apply to methods. It is an OO principle that applies to classes. Unlike classes (where the principle is "should have only one reason to change"), methods really should do only one thing. – Robert Harvey Nov 6 '17 at 16:44
  • 3
    BTW, it's not "codes." It's computer code. "Code" is never pluralized, unless you're talking about encryption. Excise the plural form from your vocabulary and you'll sound far more professional in your conversations with your fellow coders. – Robert Harvey Nov 6 '17 at 16:48
  • If you are not sure then ask yourself can I fully describe it in one sentence without using the word “and”. Like everything in programming this isn’t always true, but it’s a good rule of thumb. In this case you did describe it in one sentence without using “and” which is a good indication that it has a single responsibility. – bikeman868 Nov 7 '17 at 3:10
4

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?

  • To the system, useId == 0 means user do not exist. I should have change the message if I want to get rid of the badge number parts in the function. – Pop Nov 6 '17 at 6:24
  • 5
    @Pop: Then it should not be returning 0, it should be returning User.NonExistingUserId. The code should explain itself to the new reader. – Eric Lippert Nov 6 '17 at 21:35
1

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();
}
0

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.

0

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.

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.