How to approach refactoring a class? (VB.Net)

Question

I recently started a project and needed to do some integration with LDAP via DirectoryServices. I've done this in other apps so I went into one of them to see how I did it -- why reinvent the wheel right? Well, while this wheel works, it was developed years ago and kinda smells (it's wooden, firmly attached to the previous vehicle, hard to repair and produces a potentially bumpy ride).

So I thought to myself, this is the perfect time to refactor this puppy and make it more portable, reusable, reliable, easier to configure etc. Now that's fine and dandy, but then I started feeling a tad overwhelmed regarding to where to start. Should it be a separate library? How should it be configured? Should it use IoC? DI?

So my [admittedly subjective] question is this -- given a relatively small, but quite useful class like the one below, what is a good approach to refactoring it? What questions do you ask and how do you decide what to implement or not implement? Where do you draw the line regarding configuration flexibility?

[Note: Please don't bash this code too much okay? It was written a long time ago and has functioned perfectly well baked into an in house application.]

Public Class AccessControl

Public Shared Function AuthenticateUser(ByVal id As String, ByVal password As String) As Boolean
    Dim path As String = GetUserPath(id)
    If path IsNot Nothing Then
        Dim username As String = path.Split("/")(3)
        Dim userRoot As DirectoryEntry = New DirectoryEntry(path, username, password, AuthenticationTypes.None)
        Try
            userRoot.RefreshCache()
            Return True
        Catch ex As Exception
            Return False
        End Try
    Else
        Return False
    End If
End Function

Private Shared Function GetUserPath(ByVal id As String) As String
    Dim root As DirectoryEntry = New DirectoryEntry("LDAP://XXXXX/O=YYYY", String.Empty, String.Empty, AuthenticationTypes.None)
    Dim searcher As New DirectorySearcher
    Dim results As SearchResultCollection
    Dim result As SearchResult
    Try
        With searcher
            .SearchRoot = root
            .PropertiesToLoad.Add("cn")
            .Filter = String.Format("cn={0}", id)
            results = .FindAll()
        End With
        If results.Count > 0 Then
            result = results(0)
            Return result.Path.ToString()
        Else
            Return Nothing
        End If
    Catch ex As Exception
        Return Nothing
    End Try
End Function

Public Shared Function GetUserInfo(ByVal id As String) As UserInfo
    Dim root As DirectoryEntry = New DirectoryEntry("LDAP://XXXXX/O=YYYY", String.Empty, String.Empty, AuthenticationTypes.None)
    Dim searcher As New DirectorySearcher
    Dim results As SearchResultCollection
    Dim props() As String = {"id", "sn", "mail", "givenname", "container", "cn"}
    Try
        With searcher
            .SearchRoot = root
            .PropertiesToLoad.AddRange(props)
            .Filter = String.Format("cn={0}", id)
            results = .FindAll()
        End With
        If results.Count > 0 Then
            Dim properties As PropertyCollection = results(0).GetDirectoryEntry().Properties
            Dim user As New UserInfo(properties("id").Value)
            user.EmailAddress = properties("mail").Item(0).ToString
            user.FirstName = properties("givenname").Item(0).ToString
            user.LastName = properties("sn").Item(0).ToString
            user.OfficeLocation = properties("container").Item(0).ToString
            Return user
        Else
            Return New UserInfo
        End If
    Catch ex As Exception
        Return Nothing
    End Try
End Function

Public Shared Function IsMemberOfGroup(ByVal id As String, ByVal group As String) As Boolean
    Dim root As DirectoryEntry = New DirectoryEntry("LDAP://XXXXX/O=YYYY", String.Empty, String.Empty, AuthenticationTypes.None)
    Dim searcher As New DirectorySearcher
    Dim results As SearchResultCollection
    Dim result As SearchResult
    Dim props() As String = {"cn", "member"}
    Try
        With searcher
            .SearchRoot = root
            .PropertiesToLoad.AddRange(props)
            .Filter = String.Format("cn={0}", group)
            results = .FindAll()
        End With
        If results.Count > 0 Then
            For Each result In results
                Dim members As PropertyValueCollection = result.GetDirectoryEntry().Properties("member")
                Dim member As String
                For i As Integer = 0 To members.Count - 1
                    member = members.Item(i).ToString
                    member = member.Substring(3, member.IndexOf(",") - 3).ToLowerInvariant
                    If member.Contains(id.ToLowerInvariant) Then Return True
                Next
            Next
        End If
        Return False
    Catch ex As Exception
        Return False
    End Try
End Function

Public Shared Function GetMembersOfGroup(ByVal group As String) As List(Of String)
    Dim groupMembers As New List(Of String)
    Dim root As DirectoryEntry = New DirectoryEntry("LDAP://XXXXX/O=YYYY", String.Empty, String.Empty, AuthenticationTypes.None)
    Dim searcher As New DirectorySearcher
    Dim results As SearchResultCollection
    Dim result As SearchResult
    Dim props() As String = {"cn", "member"}
    Try
        With searcher
            .SearchRoot = root
            .PropertiesToLoad.AddRange(props)
            .Filter = String.Format("cn={0}", group)
            results = .FindAll()
        End With
        If results.Count > 0 Then
            For Each result In results
                Dim members As PropertyValueCollection = result.GetDirectoryEntry().Properties("member")
                Dim member As String
                For i As Integer = 0 To members.Count - 1
                    member = members.Item(i).ToString
                    member = member.Substring(3, member.IndexOf(",") - 3).ToLowerInvariant
                    groupMembers.Add(member)
                Next
            Next
        End If
    Catch ex As Exception
        Return Nothing
    End Try
    Return groupMembers
End Function

End Class  

Clarifications:
- there is a separate class for user (simple poco)
- there isn't a group class since all that's used right now is the list of ids, could be useful to add though

Answer 1

Here is an example of a refactored code sample:

Public Class AccessControl

    Public Shared Function AuthenticateUser(ByVal id As String, ByVal password As String) As Boolean
        Dim path As String
        Dim username As String
        Dim userRoot As DirectoryEntry

        path = GetUserPath(id)

        If path.Length = 0 Then
            Return False
        End If

        username = path.Split("/")(3)
        userRoot = New DirectoryEntry(path, username, password, AuthenticationTypes.None)

        Try
            userRoot.RefreshCache()
            Return True
        Catch ex As Exception
            ' Catching Exception might be accepted way of determining user is not authenticated for this case
            ' TODO: Would be better to test for specific exception type to ensure this is the reason
            Return False
        End Try
    End Function

    Private Shared Function GetUserPath(ByVal id As String) As String
        Dim results As SearchResultCollection
        Dim propertiesToLoad As String()

        propertiesToLoad = New String() {"cn"}
        results = GetSearchResultsForCommonName(id, propertiesToLoad)

        If results.Count = 0 Then
            Return String.Empty
        Else
            Debug.Assert(results.Count = 1)
            Return results(0).Path
        End If
    End Function

    Public Shared Function GetUserInfo(ByVal id As String) As UserInfo
        Dim results As SearchResultCollection
        Dim propertiesToLoad As String()

        propertiesToLoad = New String() {"id", "sn", "mail", "givenname", "container", "cn"}
        results = GetSearchResultsForCommonName(id, propertiesToLoad)

        If results.Count = 0 Then
            Return New UserInfo
        End If

        Debug.Assert(results.Count = 1)
        Return CreateUser(results(0).GetDirectoryEntry().Properties)
    End Function

    Public Shared Function IsMemberOfGroup(ByVal id As String, ByVal group As String) As Boolean
        Dim allMembersOfGroup As List(Of String)
        allMembersOfGroup = GetMembersOfGroup(group)
        Return allMembersOfGroup.Contains(id.ToLowerInvariant)
    End Function

    Public Shared Function GetMembersOfGroup(ByVal group As String) As List(Of String)
        Dim results As SearchResultCollection
        Dim propertiesToLoad As String()

        propertiesToLoad = New String() {"cn", "member"}
        results = GetSearchResultsForCommonName(group, propertiesToLoad)

        Return ConvertMemberPropertiesToList(results)
    End Function

    Private Shared Function GetStringValueForPropertyName(ByVal properties As PropertyCollection, ByVal propertyName As String) As String
        Return properties(propertyName).Item(0).ToString
    End Function

    Private Shared Function CreateUser(ByVal userProperties As PropertyCollection) As UserInfo
        Dim user As New UserInfo(userProperties("id").Value)
        With user
            .EmailAddress = GetStringValueForPropertyName(userProperties, "mail")
            .FirstName = GetStringValueForPropertyName(userProperties, "givenname")
            .LastName = GetStringValueForPropertyName(userProperties, "sn")
            .OfficeLocation = GetStringValueForPropertyName(userProperties, "container")
        End With
        Return user
    End Function

    Private Shared Function GetValueFromMemberProperty(ByVal member As String) As String
        Return member.Substring(3, member.IndexOf(",") - 3).ToLowerInvariant
    End Function

    Private Shared Function ConvertMemberPropertiesToList(ByVal results As SearchResultCollection) As List(Of String)
        Dim result As SearchResult
        Dim memberProperties As PropertyValueCollection
        Dim groupMembers As List(Of String)

        groupMembers = New List(Of String)
        For Each result In results
            memberProperties = result.GetDirectoryEntry().Properties("member")
            For i As Integer = 0 To memberProperties.Count - 1
                groupMembers.Add(GetValueFromMemberProperty(memberProperties.Item(i).ToString))
            Next
        Next
        Return groupMembers
    End Function

    Private Shared Function GetSearchResultsForCommonName(ByVal commonName As String, ByVal propertiesToLoad() As String) As SearchResultCollection
        Dim results As SearchResultCollection
        Dim searcher As New DirectorySearcher
        With searcher
            .SearchRoot = GetDefaultSearchRoot()
            .PropertiesToLoad.AddRange(propertiesToLoad)
            .Filter = String.Format("cn={0}", commonName)
            results = .FindAll()
        End With
        Return results
    End Function

    Private Shared Function GetDefaultSearchRoot() As DirectoryEntry
        Return New DirectoryEntry("LDAP://XXXXX/O=YYYY", String.Empty, String.Empty, AuthenticationTypes.None)
    End Function

End Class

I think you can ever take this further by extracting constants, etc, but this removes most of the duplication, etc. Let me know what you think.

Way, too late, I realize... but also wanted to address some of the questions you asked as well. See http://chrismelinn.wordpress.com/2011/06/18/questions-before-refactoring-2/

Answer 2

I believe it's important to modify the exception handling as well. The pattern I see in the methods above is:

Try
    ...   
Catch ex As Exception
    Return False
End Try

The code above is essentially hiding (swallowing) its exceptions. This may have been implemented initially because certain types of exceptions were thrown as a result of the user not being found, and returning False or Nothing was probably appropriate. However, you may been getting other types of exceptions in your application which you may never know about (e.g. OutOfMemoryException, etc).

I would suggest catching only specific types of exceptions that you may want to legitimately return false/Nothing. For others, let the exception bubble up, or log it at a minimum.

For other tips on exception handling, read this helpful post.

Answer 3

It might not be the most significant refactoring, but I am a big fan of the early return. So, for instance, where you have:

If results.Count > 0 Then
    Dim properties As PropertyCollection = results(0).GetDirectoryEntry().Properties
    Dim user As New UserInfo(properties("id").Value)
    user.EmailAddress = properties("mail").Item(0).ToString
    user.FirstName = properties("givenname").Item(0).ToString
    user.LastName = properties("sn").Item(0).ToString
    user.OfficeLocation = properties("container").Item(0).ToString
    Return user
Else
    Return New UserInfo
End If

I would use, instead:

If results.Count == 0 Then Return New UserInfo

Dim properties As PropertyCollection = results(0).GetDirectoryEntry().Properties
Dim user As New UserInfo(properties("id").Value)
user.EmailAddress = properties("mail").Item(0).ToString
user.FirstName = properties("givenname").Item(0).ToString
user.LastName = properties("sn").Item(0).ToString
user.OfficeLocation = properties("container").Item(0).ToString
Return user

Indentation implies complexity, and there isn't enough complexity in the special handling of the empty result case to warrant 8 lines of indentation. There is a point at which removing indentation can actually hide real complexity, so don't push this rule too hard, but for the code presented, I'd definitely use the early return.

Answer 4

The first thing I would do is remove any duplication. Strip out common functionality in to separate methods.

Also, think along the lines of every class should have a single role/responsibility - you might want to create separate User and Group classes.

There is a catalog of common refactorings here:

http://www.refactoring.com/catalog/index.html

You should only really consider Inversion of Control if you wish to swap in different classes for testing reasons etc...

Answer 5

The first thing that immediately jumps out at me is that there's a lot of functions involving users that don't seem to be associated with the user class you have created.

I would try some of the following approaches:

• Add the following to the user class:
  • Path
  • Authenticate(string password) - this could be a static method, not sure of the use cases here.
  • Groups - I would also recommend creating an actual domain object for groups. It could have a collection of users as a property to start with.