Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Here is some VB code that is part of a major system of apps that I have been asked to maintain, one of the more fun things that I get to do is rewrite some code here and there, I have done a little bit of that here, I finally have it functioning properly and would like to see if I am still on an upward stride in my VB learning.

Module Module1

    Private Function SQL_Connection_String() As String
        Dim Connection As String
#If DEBUG Then
        Connection = {TestConnectionString}
#ElseIf CONFIG = "Release" Then
        Connection = {ProductionConnectionString}
#End If
        Return Connection
    End Function
    Private Function test_Email() As String
        Return {TestersEmail}
    End Function

    Private sLetterText As String

    Sub Main()
        Dim sSource As String
        Dim sLog As String
        Dim sEvent As String
        Dim sMachine As String

        sSource = "OLMJSubscriberNotification"
        sLog = "Application"
        sEvent = "Error"
        sMachine = "."

        If Not EventLog.SourceExists(sSource) Then
            EventLog.CreateEventSource(sSource, sLog)
        End If

        Dim ErrorLog As New EventLog(sLog, sMachine, sSource)
        Dim SentLog As New EventLog(sLog, sMachine, sSource)

        ReadSQL(SentLog, ErrorLog)
    End Sub

    Public Sub ReadSQL(ByRef ErrorLog As EventLog, ByRef SentLog As EventLog)
        Dim sText As New String("")
        Dim sRecip As New String("")
        Dim mailSent As Integer
        Dim dr As SqlDataReader
        Dim sRecipList As New String("")

        Dim cnn As New SqlConnection(SQL_Connection_String())

        Const SQLstr = "SELECT UserName, SubscriptionEnd, Email,UserID, AgencyName FROM Users " & _
            "WHERE(DateDiff(Day, GETDATE(), SubscriptionEnd) <= 10) " & _
            "And DateDiff(Day, GETDATE(), SubscriptionEnd) >= 0 " & _
            "AND (DATEDIFF(day,GETDATE(), NotificationSent) < -10 OR NotificationSent IS NULL)"

        Dim objCommand As New SqlCommand(SQLstr, cnn)

        Try
            ReadLetterText(ErrorLog)
            cnn.Open()
            dr = objCommand.ExecuteReader
            While dr.Read
                sRecip = dr(2).ToString
                sRecipList = sRecipList & sRecip & " - " & dr(4).ToString & vbCrLf
                SendMail(sLetterText, sRecip, ErrorLog, True)
                UpdateNoticeSent(dr(3), ErrorLog)
                mailSent += 1
            End While
            If mailSent > 0 Then
                sRecipList = "The following " & mailSent.ToString & " subscribers received expiration mail " & Now.ToString & ":" & vbCrLf & vbCrLf & sRecipList

#If DEBUG Then
                SendMail(sRecipList, test_Email, ErrorLog, False)
#Else
                SendMail(sRecipList, {ErrorContactEmails} , ErrorLog, False)
#End If
            End If
            SentLog.WriteEntry("Notices Sent: " & mailSent.ToString & "  " & Now.ToString)

        Catch err As System.Exception
            ErrorLog.WriteEntry(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString)
#If DEBUG Then
            SendErrorMail(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString, test_Email, ErrorLog)
#Else
            SendErrorMail(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString, "helpdesk@Address.com", ErrorLog)
#End If
        Finally
            objCommand.Dispose()
            cnn.Dispose()
        End Try
    End Sub

    Private Sub SendErrorMail(ByVal sText As String, ByVal sRecip As String, ByRef ErrorLog As EventLog)
        Try
            Dim Mailmsg As New Mail.MailMessage
            Dim obj As New Mail.SmtpClient("{SMTP.Client.Fake}")
            obj.Port = 25

#If DEBUG Then
            Mailmsg.To.Add(test_Email)
#Else
            Mailmsg.To.Add(New MailAddress("Developer@Address.com"))
            Mailmsg.CC.Add("ProjectManager@Address.com")
            Mailmsg.To.Add(sRecip)
#End If
            Mailmsg.Subject = "OLMJ Subscriber Notification Error"
            Mailmsg.From = New MailAddress("webdev@Address.com", "UJS Online Money Judgment")
            Mailmsg.Priority = MailPriority.High

            Mailmsg.Body = sText
            Mailmsg.IsBodyHtml = False

            obj.Send(Mailmsg)
        Catch err As System.Exception
            ErrorLog.WriteEntry(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString)
        End Try
    End Sub

    Private Sub SendMail(ByVal sText As String, ByVal sRecip As String, ByRef ErrorLog As EventLog, ByVal sendForm As Boolean)
        Try
            Dim Mailmsg As New Mail.MailMessage
            Dim obj As New Mail.SmtpClient("SMTP.Client.fake")
            obj.Port = 25

            Mailmsg.To.Add(sRecip)
            Mailmsg.From = New MailAddress("webdev@Address.com", "UJS Online Money Judgment")
            Mailmsg.Subject = "Expiration Notice"
            Mailmsg.Body = sText
            Mailmsg.Priority = MailPriority.Normal
            Mailmsg.IsBodyHtml = True

            If sendForm Then
                Dim subAgreeForm As New Attachment("Subscriber_Agreement_And_Request_Form.pdf")
                Mailmsg.Attachments.Add(subAgreeForm)
            End If

            obj.Send(Mailmsg)
        Catch err As System.Exception
            ErrorLog.WriteEntry(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString)
#If DEBUG Then
            SendErrorMail(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString, test_Email, ErrorLog)
#Else
            SendErrorMail(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString, "helpdesk@Address.com", ErrorLog)
#End If
        End Try
    End Sub

    Private Sub UpdateNoticeSent(ByVal userID As Integer, ByRef ErrorLog As EventLog)
        Dim rowsAffected As Integer
        Const SQLstr = _
            "UPDATE Users SET NotificationSent = @NotiSent where UserID = @UserID"

        Using UpdateConn As New SqlConnection(SQL_Connection_String)
            Using objCommand As New SqlCommand(SQLstr, UpdateConn)
                Try
                    objCommand.CommandType = CommandType.Text
                    objCommand.Parameters.Add("@UserID", SqlDbType.Int)
                    objCommand.Parameters("@UserID").Value = userID
                    objCommand.Parameters.Add("@NotiSent", SqlDbType.DateTime)
                    objCommand.Parameters("@NotiSent").Value = Now
                    UpdateConn.Open()
                    rowsAffected = objCommand.ExecuteNonQuery()
                Catch err As System.Exception
                    ErrorLog.WriteEntry(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString)
#If DEBUG Then
                    SendErrorMail(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString, test_Email, ErrorLog)
#Else
            SendErrorMail(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString, "helpdesk@Address.com", ErrorLog)
#End If
                Finally
                    objCommand.Dispose()
                    UpdateConn.Close()
                End Try
            End Using
        End Using
    End Sub
    Private Sub ReadLetterText(ByRef ErrorLog As EventLog)

        Dim sFilePath As String
        Dim sTempLine As String
        Dim sLetterBody As New String("")

        Try
            sFilePath = System.AppDomain.CurrentDomain.BaseDirectory()
            sFilePath = sFilePath & "SubExp.txt"
            Dim myStreamReader As New IO.StreamReader(sFilePath)
            Do Until myStreamReader.Peek() = -1
                sTempLine = myStreamReader.ReadLine()
                sLetterBody = sLetterBody & vbCrLf & sTempLine
            Loop
            sLetterText = sLetterBody
        Catch err As System.Exception
            ErrorLog.WriteEntry(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString)
            SendErrorMail(System.Reflection.MethodBase.GetCurrentMethod().Name().ToString & " " & err.ToString, "helpdesk@Address.com", ErrorLog)
        End Try
    End Sub
End Module

let me know what you think.

share|improve this question

3 Answers 3

up vote 6 down vote accepted

This query uses DATEDIFF() inefficiently:

Const SQLstr = "SELECT UserName, SubscriptionEnd, Email,UserID, AgencyName FROM Users " & _
    "WHERE(DateDiff(Day, GETDATE(), SubscriptionEnd) <= 10) " & _
    "And DateDiff(Day, GETDATE(), SubscriptionEnd) >= 0 " & _
    "AND (DATEDIFF(day,GETDATE(), NotificationSent) < -10 OR NotificationSent IS NULL)"

To accomplish that, the server would have to run DATEDIFF() on every single row in the Users table. Instead, you should calculate the cutoff dates just once:

SELECT UserName, SubscriptionEnd, Email,UserID, AgencyName
    FROM Users
    WHERE
        SubscriptionEnd <= CAST(DATEADD(Day, 10, GETDATE()) AS DATE)
        AND SubscriptionEnd >= CAST(GETDATE() AS DATE)
        AND (NotificationSent IS NULL
             OR NotificationSent < CAST(DATEADD(Day, -10, GETDATE()) AS DATE));

(The casts truncate the results of DATEADD() to midnight.)

share|improve this answer
    
I like that. thank you it makes sense. maybe I should tag this as SQL as well?? –  Malachi Apr 15 at 17:39

when I posted this I thought that I caught everything that I could think of, but looking at it again I noticed that I could use at least 2 more Using statements here:

    Dim objCommand As New SqlCommand(SQLstr, cnn)

    Try
        ReadLetterText(ErrorLog)
        cnn.Open()
        dr = objCommand.ExecuteReader
        While dr.Read
            sRecip = dr(2).ToString
            sRecipList = sRecipList & sRecip & " - " & dr(4).ToString & vbCrLf
            SendMail(sLetterText, sRecip, ErrorLog, True)
            UpdateNoticeSent(dr(3), ErrorLog)
            mailSent += 1
        End While
        If mailSent > 0 Then

now I have

     Using objCommand As New SqlCommand(SQLstr, cnn)
        Try
            ReadLetterText(ErrorLog)
            cnn.Open()
            dr = objCommand.ExecuteReader
            Using dr
                While dr.Read
                    sRecip = dr(2).ToString
                    sRecipList = sRecipList & sRecip & " - " & dr(4).ToString & vbCrLf
                    SendMail(sLetterText, sRecip, ErrorLog, True)
                    UpdateNoticeSent(dr(3), ErrorLog)
                    mailSent += 1
                End While
            End Using
            If mailSent > 0 Then

The DataReader couldn't be initialized inside the using statement like the connection could be, something about it being a Friend and I couldn't make it angry enough to denounce this "Friend Status" so I left the declaration and assignment but put it into a using statement.

I could probably get rid of the Finally as well because all of the disposable objects are inside of Using statements now and should be disposed of in the case of an error no matter what, but I am paranoid so I leave them in there, and as far as I know it doesn't hurt, much.

share|improve this answer
    
+1 but I'm not sure I get the part about not being able to go Using dr = objCommand.ExecuteReader - Somehow I'm sure I could make this work in C# (no clue about correct VB syntax though)... –  Mat's Mug Apr 15 at 17:34
    
In VB I don't always understand the terms they use there. it should have been Using dr As New SqlDataReader(objCommand.ExecuteReader) or something like that. but it didn't like it –  Malachi Apr 15 at 17:36

I think a lot of this code could be greatly simplified if you were using a logging framework, such as NLog.

The logging configuration would be in a XML configuration file, which you can change during of after deployment, without changing the code.

You can configure one or more targets - a text file in the file system, a database table, the Windows Event Log, an email notification, ..or all of 'em.

Then you can configure rules and loggers so as to have log messages for a specific logger, of a given level (say, ERROR and up), emailed to support with a stack trace, for example.

This would remove most of the logging clutter and precompiler instructions - then all you need to do is deploy the nlog.config file along with your project, and overwrite it with a "production" copy when deployed in production (that could also be automated).

share|improve this answer
    
that sounds wonderful, what is the learning curve? lol but seriously. –  Malachi Apr 15 at 18:27
1  
It's pretty easy actually - check it out! (I've only used it with C# though) –  Mat's Mug Apr 15 at 18:28
    
I will check it out, I think that is just what I need for another one of my projects here at work, I will put it on the list of things for that project –  Malachi Apr 15 at 18:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.