2009-07-24 70 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