Is the new version of this code more difficult to read and understand than the original? How can it be improved?

Question

I was asked to do a code review for the following block of code. It implemented a bug fix to prevent some values being added to some drop down lists depending on the user's domain, in an ASP.NET WebForms app. The three lines adding items to the lists were from the pre-bug-fix code (not part of the fix), so I didn't review it.

        //Only add corporate AD domain if the user is a member of it.
        string domainName = "";
        if (Thread.CurrentPrincipal == null) 
        {
            //"Missing the thread identity";
        }
        else if (Thread.CurrentPrincipal.Identity.Name.IndexOf("\\") < 0)
        {
            //"No domain detected leave domain name blank.";
        }
        else
        {
            domainName = Utils.GetUserDomain(Thread.CurrentPrincipal).ToUpper();
        }

        if (domainName == Config.AppSettings.Domain.ToUpper())
        {
              domainUserDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainOfficeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainTypeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
        }

I reviewed the code and suggested changing the code to the following:

        //Only add corporate AD domain if the user is a member of it.
        var currentPrincipal = Thread.CurrentPrincipal;
        var hasDomain = currentPrincipal != null && currentPrincipal.Identity.Name.Contains("\\");
        var domainName = hasDomain ? Utils.GetUserDomain(currentPrincipal) : null;

        if (string.Equals(domainName, Config.AppSettings.Domain, StringComparison.OrdinalIgnoreCase))
        {
              domainUserDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainOfficeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainTypeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
        }

When the original developer saw the changes, he agreed with some of it (change of IndexOf to Contains, and ToUpper to String.Equals for case insensitive search), but not all of it. His main complaint was that the new code was more difficult to read and understand, and that the if blocks were clearer than the ternary operator. We weren't able to come to a point of agreement.

Could you please review both the original and new code and suggest improvements to either. Also, please point out any problems in the new code, especially where you think it is worse than the original code.

Answer 1

//These two lines of code tells you exactly what's happening without burying
//you with the details. It almost reads like plain English instead of code.
if(IsMemberOfCorporateDomain(Thread.CurrentPrincipal))
    AddCorporateDomainToDropDowns();


private void AddCorporateDomainToDropDowns()
{    
   //I don't think you need to set both Text and Value, 
   //check http://msdn.microsoft.com/en-us/library/system.web.ui.webcontrols.listitem.aspx

   domainUserDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain));
   domainOfficeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain));
   domainTypeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain));
}

private bool IsMemberOfCorporateDomain(IPrincipal principal)
{
    var domainName = GetDomainName(principal);

    return domainName != null && domainName.Equals(Config.AppSettings.Domain, StringComparison.OrdinalIgnoreCase);
}

//This function's only job is to get domain name from a given principal.
//This will allow you to use this function elsewhere if needed. 
private string GetDomainName(IPrincipal principal)
{
    if(principal == null || principal.Identity == null || principal.Identity.Name == null)
        return null;

    if(principal.Identity.Name.Contains("\\"))
        return null;

    return Utils.GetUserDomain(principal.Identity.Name);
}

That's how I would do it. As I wrote in my comment elsewhere, the advantages are:

1. The code becomes self documenting. Now you know the 'semantics' of the code without having to read the actual code.
2. If for some reason there is a bug in a function (say GetDoaminName) and it throws an Exception, the last function in stack trace will be 'GetDomainName'. It immediately makes it clear where the problem is.

Answer 2

Disclaimer: I'm not too familiar with C#.

The modified form has expressions that are a little too complex for me, but either would be ok. My only comment is that the initial comment line is screaming for some notice:

// Only add corporate AD domain if the user is a member of it.

Why not take all the tests and put them into a function?

if ( IsMemberOfCorporateDomain( Thread.CurrentPrincipal ) ) ...

Then, depending on how you both feel about early exits:

private bool IsMemberOfCorporateDomain( IPrincipal currentPrincipal )
{
    if ( currentPrincipal == null ) return false;
    if ( !currentPrincipal.Identity.Name.Contains("\\") ) return false;

    var domainName = Utils.GetUserDomain(currentPrincipal);
    return string.Equals( domainName, Config.AppsSettings.Domain, StringComparison.OrdinalIgnoreCase )
}

Answer 3

Both solutions have their merits. The original code is very easy to read, well commented and employs good use of vertical space. Beyond the points you already mentioned, I find that the conditional feels backwards. It may have been easier to read as

if (we have a domain name) {
  domainName = ...;
} else {
  // nope
}

i.e. we test for the positive case, not for negations. Hiding the important part in an else is dubious.

Your code does this, but presents two rather cluttered lines. It is slightly more self-documenting, but lacks visual structure. We can add parens to make the ?: stand out more:

(hasDomain) ? (Utils.GetUserDomain(currentPrincipal)) : (null)

Or, preferably, use an if-statement again:

var domainName = "";
if ( currentPrincipal != null
  && currentPrincipal.Identity.Name.Contains("\\")  // must detect a domain
) {
    domainName = Utils.GetUserDomain(currentPrincipal);
}

This splits the condition across multiple lines, thus making them easy to grok. Specifically, the formatting emphasizes operator precedence. On the right of each part of the condition, space for comments remains. I prefer "" over null (self-documenting), but you may have style guides that mandate one solution.

How the closing paren of the condition is formatted is an open question. I've shown the solution I use, but indenting the ) { by two spaces may be preferable, or even splitting them across two lines:

if (  condA
   && condB
   )
{
  ...;
}

You may have style guides which mandate a certain style.

The currentPrinciple variable is a valuable addition, as it simplifies the remaining code.

The not-reviewed code is sightly painful:

1. it should use a loop to perform the same action on multiple objects.
2. it should have been vertically aligned when no loop is used.
3. domainXDropDownList is reverse Hungarian Notation. The prefix domain indicates you may want to use an object with properties User, Office and Type. The suffix DropDownList is type information that should rather be expressed by the type system, so maybe another class.

Answer 4

First of all I am reviewing your changes only, as per FAQ. "How to do a codereview" might be a question more suitable for programmers SE.

Glenn has already pointed out that the newly added code should be extracted to another method(+1). I suggest a different signature. Before the method had no knowledge of thread or user principles, therefore it need not now either. And pulling the domain name from configuration in two different places is not a good idea.

        string corporateDomain = Config.AppSettings.Domain;

        if (canAddDomain(corporateDomain))
        {
              domainUserDropDownList.Items.Add(new ListItem(corporateDomain, corporateDomain));
              domainOfficeDropDownList.Items.Add(new ListItem(corporateDomain, corporateDomain));
              domainTypeDropDownList.Items.Add(new ListItem(corporateDomain, corporateDomain));
        }

        private canAddDomain(string domain) {
            IsMemberOfDomain(Thread.CurrentPrincipal, domain)
        }
// where IsMemberOfDomain is as Glenn implemented except domain is extracted as a parameter

As a next step refactoring these should probably be extracted as fields: Thread.CurrentPrincipal.Identity.Name, Config.AppSettings.Domain

That many dots and dependence on static properties makes your code untestable. Any bugs cannot be detected until it is pushed to production and when a bug occurs it would not be clear whether it is a configuration error or a programming error.

Answer 5

I normally format my ternary expressions (for the purposes of readability) as follows:

var hasDomain = currentPrincipal != null &&
    currentPrincipal.Identity.Name.Contains("\\");

var domainName = hasDomain ? 
    Utils.GetUserDomain(currentPrincipal) : 
    null;

Perhaps this is an acceptable compromise?

Answer 6

• I would extract a static method named GetDomainNameIfPresent() (GetDomainNameOrNull() might also be a good name for that).

• I would extract another method: AddDomainFromAppSettingsToDropDownLists()

By extracting these methods, you get a more self-documenting code (that nedds less comments) and that is more likely to be re-used instead of re-written.

But looking at the bigger picture, you might want to take advantage of MVVM and data binding (WPF). Then you would not operate on the Items property of the ComboBoxes, but on the collection(s) that are bound to their ItemsSource property.