patternMinor
Script for saving/sorting/sending emails based on their subject
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.
```
'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
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
-
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
-
As a matter of opinion, Sub and Function names should be PascalCase. Instead of
-
Back to over abbreviation. There are single letter parameter names. They tell me nothing about what I'm supposed to pass into them.
Functionality
-
Let's talk about this section for a minute.
This works great for now, but what happens when I want to expand it to include other document types? I'm thinking a
-
You can break out of the for loop in
There are probably a couple of other things that could be said, but I'll leave those to another reviewer.
-
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 IfThis 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.
saveFoldershould 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 FunctionThere 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 SubIf 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 IfPublic 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 FunctionContext
StackExchange Code Review Q#55055, answer score: 7
Revisions (0)
No revisions yet.