5
\$\begingroup\$

I'm a newbie in the use of LinqToXml and I wonder if I can improve this snippet.

 Shared Function GetListByType(ByVal intIndex As Integer) As List(Of FileInfo)

            Dim strWorkingPath As DirectoryInfo = New DirectoryInfo(GetWorkspaceDir)
            Dim lstAllFiles As FileInfo() = strWorkingPath.GetFiles("*.xml", SearchOption.AllDirectories)
            Dim lstFiltredList As New List(Of FileInfo)
            Dim xelement As XElement
            Dim contratTypeIndex As String
            Dim contratVersion As Integer

            For Each fic In lstAllFiles
                xelement = xelement.Load(fic.FullName)
                contratTypeIndex = xelement.Element("TypeIndex").Value
                contratVersion = xelement.Element("Version").Value
                If (contratTypeIndex = intIndex.ToString And contratVersion = 0) Then
                    lstFiltredList.Add(fic)
                End If
            Next

            Return lstFiltredList.Distinct.ToList

        End Function
\$\endgroup\$
8
\$\begingroup\$

Option Strict

You should always set Option Strict = On.

Short circuit evaluation

You should use the AndAlso operator instead of the And operator. By using And both conditions will be evaluated. By using AndAlso the second condition will only be evaluated if the first returns true.

List(Of FileInfo)

Instead of returning a List(Of FileInfo) you should consider to return an IEnumerable(Of FileInfo).

Naming

You shouldn't prefix your variables like strWorkingPath or lstAllFiles.

GetFiles() vs EnumerateFiles()

From the docs

The EnumerateFiles and GetFiles methods differ as follows: When you use EnumerateFiles, you can start enumerating the collection of names before the whole collection is returned; when you use GetFiles, you must wait for the whole array of names to be returned before you can access the array. Therefore, when you are working with many files and directories, EnumerateFiles can be more efficient.

Refactoring

First we will extract the loading of the file and reading the properties to separate methods and add a private class holding the TypeIndex and Version properties of the xml file.

Private Shared Function DoesFileMatch(file As FileInfo, index As String) As Boolean
    Dim version As XmlVersion = GetXmlVersion(file)
    Return (version.Version = "0" AndAlso version.TypeIndex = index)
End Function

Private Shared Function GetXmlVersion(file As FileInfo) As XmlVersion
    Return New XmlVersion(XElement.Load(file.FullName))
End Function

Private Class XmlVersion
    Public Property TypeIndex As String
    Public Property Version As String
    Public Sub New(element As XElement)
        TypeIndex = element.Element("TypeIndex").Value
        Version = element.Element("Version").Value
    End Sub
End Class

Next we add an overloaded GetListByType() method which takes a DirectoryInfo and a String as input parameter.

Shared Function GetListByType(directory As DirectoryInfo, ByVal typeIndex As String) As IEnumerable(Of FileInfo)

End Function

Which we call in our former method like

Shared Function GetListByType(ByVal typeIndex As Integer) As IEnumerable(Of FileInfo)
    Return GetListByType(typeIndex.ToString(), New DirectoryInfo(GetWorkspaceDir))
End Function  

The overloaded method can then be simplified, using all above, to

Private Shared Function GetListByType(directory As DirectoryInfo, ByVal typeIndex As String) As IEnumerable(Of FileInfo)

    Dim filteredFiles As New List(Of FileInfo)

    For Each fic In directory.EnumerateFiles("*.xml", SearchOption.AllDirectories)
        If (DoesFileMatch(fic, typeIndex)) Then
            filteredFiles.Add(fic)
        End If
    Next

    Return filteredFiles.Distinct()

End Function

But wait, we can do this a lot more lambda like

Private Shared Function GetListByType(directory As DirectoryInfo, ByVal typeIndex As String) As IEnumerable(Of FileInfo)

    Return directory.EnumerateFiles("*.xml", SearchOption.AllDirectories) _
        .Where(Function(x) DoesFileMatch(x, typeIndex)) _
        .Distinct()

End Function
\$\endgroup\$
0
3
\$\begingroup\$

I am impressed by the above answer but would like to add some minor things:

You should almost always set Option Infer On:

Private Shared Function DoesFileMatch(file As FileInfo, index As String) As Boolean
    Dim version = GetXmlVersion(file)
    Return version.Version = "0" AndAlso version.TypeIndex = index
End Function

Don't use unnecessary parentheses (that's for C# programmers).

\$\endgroup\$
1
  • \$\begingroup\$ +1 for the jab at C#. I often end up writing VB#, so it have me a good chuckle. \$\endgroup\$
    – RubberDuck
    Dec 14 '14 at 1:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.