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 " & _WhouseToComboBox.textBox.Text & " ? ", 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 & "\" & FolderTarget) Then

File.WriteXml(subdirectory & "\" & FolderTarget & "\" & File.Tables(0).Rows(0).Item("InvoiceID") & ".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(By

StackExchange Code Review Q#35139, answer score: 4