10
\$\begingroup\$

I've spent a couple days making this script in Outlook that checks my emails and saves/sorts/sends a reply based on the subject. This also happens to be my first experience in working with VBA. Now that I have it working, I was wondering how I could simplify it or "make it prettier" (I always got docked points in my C++ class for having "ugly code").

DOC and PDF are unrelated to .doc and .pdf. They're just my stupid renaming of what are actually two different types of names for .pdf files.

Public Sub saveattachtodisk(itm As Outlook.MailItem)
Dim objAtt As Outlook.Attachment
Dim saveFolder As String
Dim saveName As String
Dim whatIs As String
Dim emailAttach As String
Dim attName As String
saveName = itm.Subject
whatIs = retArray(saveName)
saveFolder = "C:\Users\USER\Desktop\Staging"
For Each objAtt In itm.Attachments
    If whatIs = "PDF" Then
        objAtt.SaveAsFile saveFolder & "\" & UCase(saveName) & ".pdf"
ElseIf whatIs = "DOC" Then
    objAtt.SaveAsFile saveFolder & "\" & "DOC 14-" & stripNums(saveName) & ".pdf"
    emailAttach = saveFolder + "\" + "DOC 14-" + stripNums(saveName) + ".pdf"
    attName = stripNums(saveName)
    sendDOC emailAttach, attName
ElseIf whatIs = "other" Then
    objAtt.SaveAsFile saveFolder & "\" & saveName & ".pdf"
End If
objAtt = Nothing
Next
End Sub

'Pulls numbers from DOC type document
Function stripNums(s As String) As String
Dim saveName As String
Dim i As Integer
For i = 1 To Len(s)
    If Mid(s, i, 1) >= "0" And Mid(s, i, 1) <= "9" Then
        saveName = saveName + Mid(s, i, 1)
    End If
Next
stripNums = saveName
End Function

'Checks to see if document is PDF type docement
Function isPDF(s As String) As Boolean
Dim retVal As Boolean
retVal = False
For i = 1 To Len(s)
If Mid(s, i, 2) = "1f" Then
    retVal = True
End If
Next
isPDF = retVal
End Function

'Checks to see if document is of type PDF, DOC, or other
Function retArray(s As String) As String
Dim retVal As String
Dim isItPDF As Boolean
retVal = "other"
arr = Array("t 1", "t 2", "t 3", "t 4", "t 5", "t 6", "t 7", "t 8", "t 9", "t 0")
isItPDF = isPDF(s)
If isItPDF = False Then
For j = 0 To 9
    If InStr(1, s, arr(j), 1) = 1 Then
        retVal = "DOC"
    End If
Next
ElseIf isItPDF = True Then
    retVal = "PDF"
End If
    retArray = retVal
End Function

'Automatically creates and sends an email for DOC type documents
Function sendDOC(s As String, n As String)
Dim objMsg As MailItem
Set objMsg = Application.CreateItem(olMailItem)
objMsg.Recipients.Add ("SEND.TO@EMAIL.COM")
objMsg.Recipients.Add ("DOUBLE.CHECK@EMAIL.COM")
objMsg.Subject = "DOC 14-" & n
objMsg.Body = "Please see attached." & vbCrLf & vbCrLf & vbCrLf & "SIGNATURE BLOCK"
objMsg.Attachments.Add (s)
objMsg.Send
End Function
\$\endgroup\$
  • \$\begingroup\$ Your professor should probably step outside of academia into a real code base sometime.... \$\endgroup\$ – RubberDuck Jun 25 '14 at 17:33
  • \$\begingroup\$ I realize now that she was a terrible professor (long story) and I'm working to un-learn the bad habits I got from that course \$\endgroup\$ – slow_excellence Jun 25 '14 at 17:38
7
\$\begingroup\$

Readability

  • Proper indentation is important for readability. Everything inside of Sub...End Sub should be indented one tab (or four spaces). Else If statements should line up with their corresponding If statements. For example:

    Public Sub foo
        dim variable as boolean
        If variable Then
            'do something
        Else 
            'do something different
        End If
    End Sub
    
  • Variable and routine names are overly abbreviated. We're no longer in the bad ol' days when the length of variable names were limited. Instead of attName, try attachmentName. Apply this to all of your variable names and Mr. Maintainer will thank you.

  • As a matter of opinion, Sub and Function names should be PascalCase. Instead of saveattachtodisk, try SaveAttachmentToDisk. I do like that you're following the verb-noun naming schema for them though. You did a good job there.

  • Back to over abbreviation. There are single letter parameter names. They tell me nothing about what I'm supposed to pass into them.

Functionality

  • What happens if "C:\Users\USER\Desktop\Staging" doesn't exist on the client machine? The code crashes. Adding some error handling in will help, but the routine should also either let the user pick a folder, or build the directory itself. It's looks like it's a temporary structure, so I recommend the latter.
  • Let's talk about this section for a minute.

    If whatIs = "PDF" Then
        objAtt.SaveAsFile saveFolder & "\" & UCase(saveName) & ".pdf"
    ElseIf whatIs = "DOC" Then
        objAtt.SaveAsFile saveFolder & "\" & "DOC 14-" & stripNums(saveName) & ".pdf"
        emailAttach = saveFolder + "\" + "DOC 14-" + stripNums(saveName) + ".pdf"
        attName = stripNums(saveName)
        sendDOC emailAttach, attName
    ElseIf whatIs = "other" Then
        objAtt.SaveAsFile saveFolder & "\" & saveName & ".pdf"
    End If
    

This works great for now, but what happens when I want to expand it to include other document types? I'm thinking a DocType enum could go a long way toward making it easier to extend, as well as improve readability.

Public Enum DocType
    PDF
    DOC
End Enum

Select Case whatIs
    Case PDF
        objAtt.SaveAsFile saveFolder & "\" & UCase(saveName) & ".pdf"
    Case DOC
        objAtt.SaveAsFile saveFolder & "\" & "DOC 14-" & stripNums(saveName) & ".pdf"
        emailAttach = saveFolder + "\" + "DOC 14-" + stripNums(saveName) + ".pdf"
        attName = stripNums(saveName)
        sendDOC emailAttach, attName
    Case Else
        objAtt.SaveAsFile saveFolder & "\" & saveName & ".pdf"
End Case
  • The "Doc 14-" string show up a lot. Store it in a constant at the module scope.
  • saveFolder should be a constant at the procedure scope. If it doesn't change, declare it as a constant so Mr. Maintainer doesn't try to assign it a new value at runtime in the future.
  • You can break out of the for loop in isPDF() as soon as it finds the value it's looking for. There's also no reason for the returnValue variable to exist. Just assign the function value directly. Booleans are implicitly false upon creation, so isPDF() could look more like this.

    'Checks to see if document is PDF type docement
    Function isPDF(s As String) As Boolean
        For i = 1 To Len(s)
            If Mid(s, i, 2) = "1f" Then
                isPDF = True
                Exit For
            End If
        Next
    End Function
    

There are probably a couple of other things that could be said, but I'll leave those to another reviewer.

\$\endgroup\$
  • 2
    \$\begingroup\$ Thanks for your input! I'll be sure to keep these in mind for future projects. Also in regards to the doc vs pdf issue, they are both pdf files but have different data which require a separate naming convention. I use this code in my workplace and had to change some of the names to be safe (doc and pdf aren't the best idea now that I think about it). Thanks for the enum idea though, I really like how that works but I didn't think to use it as this is my first foray into vba. \$\endgroup\$ – slow_excellence Jun 24 '14 at 13:22
  • \$\begingroup\$ You're welcome. I hope to see you around. We could use more vba questions. \$\endgroup\$ – RubberDuck Jun 24 '14 at 13:24
  • \$\begingroup\$ I just realized a huge flaw with my script: It will overwrite any files with the same name. I've spent the past hour or so trying to make a function to search the folder for documents with the same name and add a number to the end of the new file, but I'm completely stumped. Having to add the file size to the end of the name as a temporary measure so I don't lose any important data. \$\endgroup\$ – slow_excellence Jun 25 '14 at 15:24
  • 1
    \$\begingroup\$ @slow_excellence how about adding the date/time of the email to the file's name? \$\endgroup\$ – Mathieu Guindon Jun 25 '14 at 15:54
4
\$\begingroup\$

@ckuhn203 made a very good job at pointing out most of what I had seen in your code. I'll just add a few more remarks.

In this snippet:

'Checks to see if document is of type PDF, DOC, or other
Function retArray(s As String) As String
Dim retVal As String
Dim isItPDF As Boolean
retVal = "other"
arr = Array("t 1", "t 2", "t 3", "t 4", "t 5", "t 6", "t 7", "t 8", "t 9", "t 0")
isItPDF = isPDF(s)
If isItPDF = False Then
For j = 0 To 9
    If InStr(1, s, arr(j), 1) = 1 Then
        retVal = "DOC"
    End If
Next
ElseIf isItPDF = True Then
    retVal = "PDF"
End If
    retArray = retVal
End Function

It's very, very unclear what you're doing, and why. If you're going to be supporting document types, you'll want to have a DocumentType enum, and return an enum value instead of a magic string:

Public Enum DocumentType
    UnknownFileType
    PdfDocument
    WordDocument
'    ExcelWorkbook
'    PowerPointPresentation
End Enum

Oops just noticed, that was also in @ckuhn203's answer.

The function's name, retArray, makes no sense. Consider a signature like this:

Function InferDocumentType(mailSubject As String) As DocumentType

All of a sudden, without even looking at the function's body, you know just by reading the signature, that the function will try to infer a known document type from a string representing the subject of an email. That's what descriptive names do. It makes the calling code much easier to read, too.

I'm not sure I understand why or how you're able to infer that an attachment is a .doc (.doc right?) when the subject of the email contains "t 1" or "t 6". A comment is in order here, but I think the code is making a bad assumption, inferring the type of attachment from the subject line rather than from the attached file's filename seems like a long shot.

I'd probably write it something like this:

Function InferDocumentType(attachedFileName As String) As DocumentType

    Dim dotSplit() As String
    dotSplit = Split(attachedFileName, ".")

    Dim fileExtension As String
    fileExtension = Left(dotSplit(UBound(dotSplit)), 3) '.docx becomes .doc

    Select Case LCase(fileExtension)
        Case "pdf":
            InferDocumentType = PdfDocument
        Case "doc":
            InferDocumentType = WordDocument
'        Case "xls":
'            InferDocumentType = ExcelWorkbook
'        Case "ppt":
'            InferDocumentType = PowerPointPresentation
        Case Else:
            InferDocumentType = UnknownFileType
    End Select

End Function

Then, if you want to base the file name of the copy you're saving to disk on the email's subject, by all means go ahead! But I wouldn't trust the subject of an email to contain any information whatsoever about the type of file(s) attached.

With a function that takes a file name and returns an enum that tells you what the type of file is, you can get rid of isPDF.


Don't do this:

If isItPDF = False Then
    ...
ElseIf isItPDF = True Then
    ...
End If

isItPDF is a Boolean, it can either be True or False. The above is much cleaner like this:

If isItPDF Then 'note the condition is reversed: you have it as "If Not isItPDF Then"
    ...
Else
    ...
End If

There's no need to do this:

For Each objAtt In itm.Attachments
    ...
    objAtt = Nothing 'this line does nothing
Next

The objAtt variable, or more precisely each instance of it, only lives inside an iteration of the loop, and will be Nothing after the loop; there's no need to explicitly set it to Nothing just before it's automatically set to a new reference!


Above everything else (IMO), you really need to work on your naming. I know naming is hard, but you need to try harder than that!

Public Sub saveattachtodisk(itm As Outlook.MailItem)

alllowercase is a PITA to read, and disemvoweling is a sin (akin to Hungarian Notation). Compare:

Public Sub SaveAttachmentsToDisk(emailItem As Outlook.MailItem)

@ckuhn203 already mentioned that (although in a little bit of a weaker way); use PascalCase as a naming convention (capitalize first letter of every word, no underscores), and thank yourself later (like, 6 months down the road later, when you or someone else return into that code to add or fix something)!

Also, obj is never a relevant, useful or meaningful prefix or suffix to add to any identifier in VB.


Last little nitpick:

objMsg.Body = "Please see attached." & vbCrLf & vbCrLf & vbCrLf & "SIGNATURE BLOCK"

vbCrLf is tying your macro to Windows - it probably doesn't matter, but it's good to know that if you used the environment-aware vbNewLine (and clearer IMO) instead of vbCrLf, that part of your macro running on a Linux or MacOS box would produce the expected output.

Note that String(3, vbNewLine) would produce a string with 3 new lines, so:

mailMessage.Body = "Please see attached." & String(3, vbNewLine) & "SIGNATURE BLOCK"
\$\endgroup\$
  • \$\begingroup\$ Thanks for your input mat. In regards to the DOC and PDF situation everything being processed is a pdf file, just different types of files . I (idiotically) didn't think how the way I renamed it would cause confusion. For some more clarity, I'm using this script to save files that I am scanning and sending to myself (outlook rule filters out other senders) so I'm controlling the name of the subject (I could just name the attachment when I scan it but it takes an extra 30-60s and that REALLY adds up when I'm scanning 30 years worth of info)... When I went into this project, I had zero knowled \$\endgroup\$ – slow_excellence Jun 25 '14 at 16:59
  • \$\begingroup\$ I added some clarification in the description since I can't post short enough replies. I do, however, have a question about the enum. I've been fiddling around with it and can't seem to get it right. Is it declared before the sub, inside of it, or after it? I also tried replacing the main body of code with the select case example and it refused to process any emails at that point. I also tried to add the date to the name as you suggested but it won't save because windows doesn't like ":" and /or "/". When using a replace function (I added the date/time to the end of the string) I got an error. \$\endgroup\$ – slow_excellence Jun 25 '14 at 17:33
  • \$\begingroup\$ You can declare an enum in the declarations section of any module, before Option Explicit if you have it - basically it goes at the top of the module. \$\endgroup\$ – Mathieu Guindon Jun 25 '14 at 17:34
  • \$\begingroup\$ Actually best practices recommend putting Option Explicit at the very very top of a module, so the enum would be declared just after it. Sorry for the confusion (I always put Option Explicit as the last thing in my declarations section, for some reason). \$\endgroup\$ – Mathieu Guindon Jun 25 '14 at 17:44

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.