HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Script for saving/sorting/sending emails based on their subject

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptsortingsendingsubjectforemailsbasedsavingtheir

Problem

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
r

Solution

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.

Code Snippets

Public Sub foo
    dim variable as boolean
    If variable Then
        'do something
    Else 
        'do something different
    End If
End Sub
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
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
'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

Context

StackExchange Code Review Q#55055, answer score: 7

Revisions (0)

No revisions yet.