3
\$\begingroup\$

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, dbOpenDynaset, dbSeeChanges)
bigDelim = ",,,,,,,,,,,,,,,,,,,,,,,,,," 'Necessary for thunderbird .csv import format
pathDate = CStr(DateDiff("s", #1/1/1970#, Now()))
path = Me![Outputpfad] & "\"
path = path & pathDate & "-Company-Email-Contacts-Thunderbird.csv"

Set fso = CreateObject("Scripting.FileSystemObject")
Set oFile = fso.CreateTextFile(path)
oFile.WriteLine "Vorname,Nachname,Anzeigename,Spitzname,Primäre E-Mail-Adresse,Sekundäre E-Mail-Adresse,Messenger-Name,Tel. dienstlich,Tel. privat,Fax-Nummer,Pager-Nummer,Mobil-Tel.-Nr.,Privat: Adresse,Privat: Adresse 2,Privat: Ort,Privat: Bundesland,Privat: PLZ,Privat: Land,Dienstlich: Adresse,Dienstlich: Adresse 2,Dienstlich: Ort,Dienstlich: Bundesland,Dienstlich: PLZ,Dienstlich: Land,Arbeitstitel,Abteilung,Organisation,Webseite 1,Webseite 2,Geburtsjahr,Geburtsmonat,Geburtstag,Benutzerdef. 1,Benutzerdef. 2,Benutzerdef. 3,Benutzerdef. 4,Notizen,"

rs.MoveFirst
Do While Not rs.EOF                        
    Email = rs![Email]         
    Name = Split(rs![Name], ",")    
    If rs![Telefon_int] <> "" Then
        tel = rs![Telefon_int]
    End If
    If rs![Telefon_mobint] <> "" Then
        telMobi = rs![Telefon_mobint]
    End If
    currentLine = Name(1) & "," & Name(0) & "," & Name(1) & " " & Name(0) & ",," & Email & ",,," & tel & ",,,," & telMobi & bigDelim
    oFile.WriteLine (currentLine)
    tel = ""
    telMobi = ""
    rs.MoveNext
Loop
oFile.Close
rs.Close
Set fso = Nothing
Set oFile = Nothing
MsgBox ("File successfully generated at path:  " & path)
End Sub

As far as I can tell, it works, with any subset or superset of the database records. I tested it against a employee table with 4000 entries for performance, and didn't notice a slowdown. Our actual production table is about 400 entries big. People whose names are starting with "zz_" have left the company (I didn't make that design choice and it's not changeable by me).

\$\endgroup\$
4
\$\begingroup\$

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:

  1. Prompt the user for confirmation
  2. Validate the provided input (namely the output path)
  3. Run a SQL-Query against a database
  4. Write the Query results into a file
  5. (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 :)

\$\endgroup\$
  • \$\begingroup\$ Interesting, I've seen Set <object> = Nothing as a recommended step in clean-up many places, though I don't recall exactly where, nor do I recall how old that advice is. As I understand your comment, it's not detrimental, just more typing, correct? \$\endgroup\$ – FreeMan Feb 7 '17 at 14:52
  • \$\begingroup\$ @FreeMan I'm not sure exactly, I've also seen such code, but never had problems when not doing it in a high availability asp-classic app. I concluded it's not necessary to do that \$\endgroup\$ – Vogel612 Feb 7 '17 at 16:31
  • \$\begingroup\$ From chat, this makes for some good reading and supports your assertion. \$\endgroup\$ – FreeMan Feb 7 '17 at 16:53

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.