All geek questions in one place

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

I recently started a project and I needed to integrate with LDAP through DirectoryServices. I did this in other applications, so I went into one of them to see how I did it - why reinvent the wheel correctly? Well, while this wheel is working, it was developed many years ago, and there are curious smells (it is wooden, firmly attached to the previous car, difficult to repair and produces a potentially bumpy ride).

So, I thought to myself: this is the ideal time to reorganize this puppy, make it more portable, reusable, reliable, easier to configure, etc. Now that everything is fine and dandy, but then I began to feel a little depressed as to where to start. Should it be a separate library? How to configure it? Should he use IoC? DI?

So my [admittedly subjective] question is this - considering the relatively small but rather 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 to implement? Where do you draw the line regarding configuration flexibility?

[Note: Please do not bash this code too good? It was written a long time ago and worked perfectly in one 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

Explanations:
- there is a separate class for the user (simple poco)
- there is no group class, since everything that is used right now is a list of identifiers, it may be useful to add though

+3
source share
5 answers

The following is an example of refactored code:

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 do it further by extracting constants, etc., but this eliminates most of the duplication, etc. Let me know what you think.

, , ... , . . http://chrismelinn.wordpress.com/2011/06/18/questions-before-refactoring-2/

+2

. , :

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

() . , , - , , False Nothing, , . , , , (, OutOfMemoryException ..).

, , , false/Nothing. , .

.

+2

, , . .

, , / - .

:

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

, ..

+1

, . , , :

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

:

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

, 8 . , , , .

+1

, , , , , , , .

:

  • :
    • Authentication (string password) - this may be a static method that is not sure of the use here.
    • Groups. I would also recommend creating an actual domain object for groups. It can have a collection of users as a property to start with.
0
source

Source: https://habr.com/ru/post/1713451/

More articles:


All Articles