Improving my invoice generator In this function, the copy (Write XML) is inside, but I want it to be separated from the function.  How can I improve this function?

Sub ExporttoFolder(ByVal POSPath As String, ByVal dt As DataSet, ByVal FolderCopyto As String,ByVal SelectedItem As DataTable)

If MsgBox("Do you want to Export Invoice to " &amp; _WhouseToComboBox.textBox.Text &amp; " ? ", MsgBoxStyle.YesNo) = MsgBoxResult.Yes Then

Dim Whouse As String
For Each dr As DataRow In SelectedItem.Rows
Whouse = dr.Item("whouseDesc")
ProcessCopy(POSPath, dt, FolderCopyto, Whouse)
Next dr
End If

End Sub

Sub ProcessCopy(ByVal targetDirectory As String, ByVal File As DataSet, ByVal FolderTarget As String, ByVal DirectoryFather As String)

Dim subdirectoryEntries As String() = Directory.GetDirectories(targetDirectory)
Dim Filepath As String = Nothing
Dim subdirectory As String

For Each subdirectory In subdirectoryEntries
Try

If Path.GetFileName(subdirectory) = DirectoryFather Then

If IO.Directory.Exists(subdirectory &amp; "\" &amp; FolderTarget) Then

File.WriteXml(subdirectory &amp; "\" &amp; FolderTarget &amp; "\" &amp; File.Tables(0).Rows(0).Item("InvoiceID") &amp; ".xml")

End If

End If

ProcessCopy(subdirectory, File, FolderTarget, DirectoryFather)
Catch ex As Exception
End Try

Next subdirectory

End Sub
In this function, the copy (Write XML) is inside, but I want it to be separated from the function.


Shallow Observations

In no particular order:


You need to indent your code, it makes it much easier to read. Give that Tab button some lovin'!
Exceptions are like urine samples: there's a lot of information in there to analyze if and when you need to, and the last thing you want to do is to swallow them.




Separation of Concerns

I think both procedures do more than they claim. Let's break down what needs to happen:


Prompt the user for confirmation, do nothing without user's consent.
Get the names of all target folders.
Find the destination folders.
Determine the name of the file.
Write the file.


Implementation

Prompt the user for confirmation, do nothing without user's consent

Asking for user confirmation before exporting stuff is probably a good idea. This doesn't mean the actual prompting needs to happen in the method that performs the actual export. Consider this:

Public Function GetUserConfirmation(ByVal prompt As String) As Boolean        
    Return (MsgBox(prompt, MsgBoxStyle.YesNo) = MsgBoxResult.Yes)
End If


Yes, it's a one-liner. That doesn't make it less of a useful function. We don't really care about MsgBoxResult, we just want to know if the user answered Yes to a question. This function abstracts away the MsgBox call and the need for knowing about MsgBoxResult, which makes it much more If-friendly - whether you're reading:

If GetUserConfirmation(confirmationMessage) Then


Or

If Not GetUserConfirmation(confirmationMessage) Then


You know exactly what's going on and you don't need to worry about how it's implemented.

Now in your code, this would translate to:

Sub ExportToFolder() 'parameters omitted
    If GetUserConfirmation(confirmationMessage) Then
    ' do all that stuff
    End If
End Sub


I think this is a bad construct, it smells like the code in the If statement is begging you to be surrounding a call to that method, so you can do this:

If GetUserConfirmation(confirmationMessage) Then ExportToFolder 'parameters omitted


And have ExportToFolder look like that:

Sub ExportToFolder() 'parameters omitted
    ' do all that stuff
End Sub


This means the logic that grabs an instance variable to come up with the prompt message is also moved outside of the method. Actually by doing this we have just separated the concerns of coming up with a prompt message and prompting the user with some message.

So we've got user confirmation. One down.

Get the names of all target folders

We have some DataTable that represents a selected item which contains a column called "whouseDesc" which is the name of the "parent" folder we want to use. Your code knows about a lot of things it doesn't need to be bothered with: in fact all this DataTable boilerplate just doesn't belong there. The method doesn't want a DataTable, it needs what's it in.

I would replace SelectedItem As DataTable with TargetSubfolderNames As IEnumerable(Of String), it makes the intent much clearer: we need an enumerable bunch of folder names, and we're going to expect they're the names of subfolders of a target folder.

Reading the DataTable that contains the folder names is out of scope here.

The procedure now looks like this, notice how much more focused it's starting to be:

Sub ExportToFolder(ByVal POSPath As String, _
                   ByVal dt As DataSet, _
                   ByVal FolderCopyto As String, _
                   ByVal TargetSubfolderNames As IEnumerable(Of String))

    Dim subfolderName As String
    For Each subfolderName In TargetSubfolderNames
        ProcessCopy(POSPath, dt, FolderCopyTo, subfolderName)
    Next

End Sub


Find the destination folder

ExportToFolder takes the POSPath parameter only to pass it down to ProcessCopy at each iteration. It's the same folder every time you call ProcessCopy, and yet every time the method runs, you're fetching the subdirectories and looping through all of them every time, verifying if the target folder exists under that subdirectory. I'm exhausted, aren't you?

If each call to ExportToFolder() exports stuff into 1 file in 1 folder, then it's not exactly clear why you're looping directories here, why the method is recursive, and how you're coming up with the filename.

My guess is that you want to be sure you're writing to a directory that exists. That's fine, but if there's only 1 file to write to, you're over-complicating it.

Determine the name of the file

If I understand correctly, you know the full file name from the start, and given InvoiceId which is dt.Tables(0).Rows(0).Item("InvoiceID") this would be your file name:

Path.Combine(Whouse, FolderCopyTo) + InvoiceId + ".xml"


So we can have a GetExportXmlFileName function that does it, and that can return an empty string if the folder doesn't exist:

Function GetExportXmlFileName(ByVal ParentFolder As String, ByVal TargetFolder As String, ByVal InvoiceId As String) As String

    Dim result = String.Format("{0}.xml", _
                   Path.Combine(ParentFolder, TargetFolder), _
                   InvoiceId)

    If Not Directory.Exists(Path.GetDirectoryName(result)) Then result = String.Empty

End Function


Your method is starting to look like this:

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportXmlFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then ProcessCopy FileName, Content
    Next

End Sub


Write the file

At this point I'm questioning the need for ProcessCopy altogether:

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then Content.WriteXml(FileName)
    Next

End Sub


Let's see, if we have 4 files to export, and the 2nd one blows up, we can't try the 3rd and 4th like this - we'll have an exception to deal with (read: not swallow).

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")
    Dim Errors As IList(Of String) = New List(Of String)

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then 
            Try

                Content.WriteXml(FileName)

                Catch ex As Exception
                    Errors.Add(ex) 'fine, don't do anything with it.
                                   'at least you can break and inspect it.
            End Try
        End If
    Next

End Sub


This is where we can extract a method to wrap the Try...Catch block and, incidentally, the Content.WriteXml call - I'll pretend you want to log errors with an instance-level NLog logger:

Sub ExportToFile(FileName as String, Content As DataSet)
    Try

        Content.WriteXml(FileName)

    Catch ex As Exception
        _logger.ErrorException(FileName, ex)

    End Try
End Sub


This last refactoring has given its final shape to the ExportToFolder method:

Sub ExportToFolder(ByVal TargetSubfolderNames As IEnumerable(Of String), _
                   ByVal TargetFolder As String, _
                   ByVal Content As DataSet)

    Dim SubfolderName As String, FileName As String
    Dim InvoiceId As String = Content.Tables(0).Rows(0).Item("InvoiceID")

    For Each SubfolderName In TargetSubfolderNames
        FileName = GetExportFileName(SubfolderName, TargetFolder, InvoiceId)
        If Not String.IsNullOrEmpty(FileName) Then ExportToFile(FileName, Content)
    Next

End Sub