Automatically generate a thunderbird importable contacts file for all employees

This is implemented in Ms Access 2003. The Table "People" is a ODBC linked database table on a MSSQL 2008 server. The file generated can be imported into Mozilla Thunderbird contacts to have a contact list of everyone in the company ready.

The entire thing is written under option explicit. This is mandated.

```
Private Sub GenerateContacts_Click()
'Automated Generation of an Importable .csv file for Mozilla Thunderbird contacts

'Variable Definitions
Dim fso As Object 'File System Object
Dim oFile As Object 'File Object
Dim path As String 'User selected Path (Uneditable Textbox with "set path" button
Dim currentLine As String 'current Line to be written to file
Dim rs As Recordset 'Recordset containing the data
Dim bigDelim As String ' ",,,,,,,,,,,,,,,,,,,,,,,,,," Thunderbird CSV Format delimiter
Dim db As Database 'Database object for query execution
Dim SQL As String 'SQL Query to select the Users
Dim tel As String 'Landline phone of the employee
Dim telMobi As String 'Mobile phone of the employee
Dim Email As String 'Email adress of the employee
Dim Mng As Integer 'Number of records to be processed
Dim pathDate As String 'Datediff to current date to append to filename to make files unique
Dim Name() As String 'Name of the employee

'Initialize the database
Set db = DBEngine(0)(0)

'Confirmation dialog
If Not MsgBox("Do you want to export all available contacts to thunderbird?", vbYesNo) = vbYes Then
Exit Sub
End If
If Not Me![Outputpfad] <> "" Then
MsgBox ("[Error] No output path selected.")
Exit Sub
End If

SQL = "SELECT People.Name, People.Email, People.Telefon_int, People.Telefon_mobint FROM People WHERE People.Email <> '' AND People.Name NOT LIKE 'zz_%' ORDER BY People.Eintrag DESC"
Set rs = db.OpenRecordset(SQL, dbOpenDyna

Variables

I like that you're using Option Explicit. It helps prevents "easy" mistakes from typos. I don't like that you declare all your variables in a block and I somewhat dislike the naming scheme you use:

I also used to declare all variables in a block at the start of a procedure. It's significantly easier to declare them as close as possible to their usage, though. It especially allows you to limit their scope (preventing name-conflicts and inappropriate reuse), and it lowers the "cognitive load" because you don't need to keep in mind which variables exist and which type they have.

Another thing I noticed is that you're (possibly unintentionally) mix German into your variable names: tel and telMobi would probably be declared as phone and mobilePhone by a native speaker.

Additionally the casing is inconsistent. SQL could be sqlQuery, Email should just be email and Name is an array, which should accordingly be names.

Furthermore you use some hungarian notation (oFile), but don't do that consistently (as for oFso, oRs, oDb). Since you specifically mark objects (that need to be assigned to with Set), you should mark all objects the same way. Alternatively you can grab the Rubberduck VBE Addin (which I am involved in maintaining), that has an inspection warning you when you assign to an Object-typed variable without the Set keyword making the hungarian notation useless.

Last but not least as far as I can tell bigDelim will never change. I suggest you move it from a Sub-scoped variable into a module-scoped constant.

Responsibilities

Currently this sub does a few things:

• Prompt the user for confirmation

• Validate the provided input (namely the output path)

• Run a SQL-Query against a database

• Write the Query results into a file

• (output a success message)

Each of these steps could probably be extracted into it's own method. This allows us to break down the procedure into "manageable" steps, reducing cognitive load and simplifying maintenance.

Semantics:

Some minor semantic issues I think are worth fixing:

-
You're late-binding the Scripting.FileSystemObject. I think that is a bad decision, because it means you can only declare fso as Object and accordingly lose all the help that the VBE can provide you with (autocomplete). That type should be available on all systems that this code will ever run on. No need to late-bind this.

-
the Name() you're using is a little bit messy. I'd prefer to have a firstName and a lastName to clarify usage. It should be possible to assign them as follows:

Dim split() As String
firstName = split(1)
lastName = split(2)

-
Setting objects to Nothing after usage is the same as setting Objects in java to null after usage. In my experience it's more of a hassle than the benefits you gain from it. When the object references go out of scope they're collected away anyways. I'd leave that part out :)

StackExchange Code Review Q#154702, answer score: 4