2009-07-24 28 views
3

我最近啓動了一個項目,需要通過DirectoryServices與LDAP進行一些集成。我已經在其他應用程序中完成了這項工作,所以我進入其中一個來看看我是如何做到的 - 爲什麼要重新發明這個輪子?那麼,這個輪子的工作原理是幾年前開發的,有點氣味(它是木質的,牢牢地固定在前一輛車上,很難修理,並且可能會產生顛簸)。如何進行重構類? (VB.Net)

所以我認爲,這是重構這隻小狗的最佳時機,它使它更便攜,可重複使用,更可靠,更易於配置等。現在,這很好,很花哨,但後來我開始覺得有點不知所措從哪兒開始。它應該是一個單獨的圖書館嗎?它應該如何配置?它應該使用IoC嗎? DI?

所以我的[被公認是主觀的]問題是這樣的 - 給定一個相對較小,但非常有用的類,如下所示,什麼是重構它的好方法?你問什麼問題,你如何決定實施或不實施?你在哪裏繪製配置靈活性的線?

[注意:請不要打這個代碼太多好嗎?這是很久以前寫的,並已運作得很好烤成在內部應用程序]

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 

澄清:
- 有用戶(簡單的POCO)
一個單獨的類 - 有ISN因爲所有現在使用的都是ID列表,可能對添加有用

+0

大概在開始黑客入侵之前,您已經完成了一套全面的測試。 – MarkJ 2009-07-24 17:18:31

+0

優秀的建議。 ;) – 2009-07-27 16:00:24

回答

2

這裏是一個重構的代碼示例的例子:

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 

我認爲你可以永遠藉此進一步通過提取常數等,但這種刪除大部分的重複,等讓我知道你認爲。

方式,太遲了,我意識到...但也想解決你問的一些問題。請參閱http://chrismelinn.wordpress.com/2011/06/18/questions-before-refactoring-2/

0

立即跳出我的第一件事是有很多涉及用戶的函數看起來並不像與您創建的用戶類關聯。

我會嘗試以下一些方法:

  • 以下內容添加到用戶類:
    • 路徑
    • 進行身份驗證(字符串密碼) - 這可能是一個靜態方法,而不是確定這裏的用例。
    • 組 - 我還建議爲組創建一個實際的域對象。它可能有一組用戶作爲屬性開始。
1

我會做的第一件事就是刪除任何重複。刪除通用功能以分離方法。

此外,認爲沿着每個班級應該有一個單一的角色/責任 - 你可能想創建單獨的用戶和組類。

有一個目錄常見的重構這裏:

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行縮進。刪除縮進實際上可以隱藏實際的複雜性,因此不要過分強調這個規則,但對於所提供的代碼,我一定會使用提前返回。

2

我相信修改異常處理也很重要。我的方法中在上面看到的圖案是:

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

上面的代碼基本上隱藏(吞嚥)其異常。這可能是最初實現的,因爲某些類型的異常是由於未找到用戶而引發的,並且返回False或Nothing可能是合適的。但是,您可能會在您的應用程序中獲取其他類型的異常(您可能永遠不知道)(例如,OutOfMemoryException等)。

我建議僅捕獲特定類型的異常,您可能想合法地返回false/Nothing。對於其他人,讓例外冒出來,或者至少記錄下來。

有關異常處理的其他提示,請參閱this helpful post