Simplify ASP.Net StreamWriter for appendtext to file

Question

Is there anyway to simplify this code so there aren't so many redundant w = File.AppendText(filePath) and w.flush() and w.close() in each if statement?

Anything else I have tried ends up locking the file, and crashing the site because to text file is in use by another process.

Public filePath As String = "c:\inetpub\logs\login.txt"
Public w As StreamWriter

Protected Sub LoginUser_LoginError(ByVal sender As Object, ByVal e As System.EventArgs) Handles LoginUser.LoginError

    Dim UserName As TextBox = DirectCast(LoginUser.FindControl("UserName"), TextBox)

    Dim CurrentUser As MembershipUser = Membership.GetUser(LoginUser.UserName)

    Dim logLoginError As New Thread(
        Sub()
            If (CurrentUser IsNot Nothing) Then

                If (CurrentUser.IsLockedOut = True) Then
                    w = File.AppendText(filePath)
                    w.WriteLine(CurrentUser.ToString & "'s (" & IPAddress & ") account is locked out - " & DateAndTime.Now)
                    w.Flush()
                    w.Close()
                ElseIf (CurrentUser.IsApproved = False) Then
                    w = File.AppendText(filePath)
                    w.WriteLine(CurrentUser.ToString & " (" & IPAddress & ") is attempting to login, but the account is disabled - " & DateAndTime.Now)
                    w.Flush()
                    w.Close()
                Else
                    w = File.AppendText(filePath)
                    w.WriteLine(CurrentUser.ToString & "'s (" & IPAddress & ") login credentials failed - " & DateAndTime.Now)
                    w.Flush()
                    w.Close()
                End If

            Else
                w = File.AppendText(filePath)
                w.WriteLine("Unknown user " & UserName.Text & " (" & IPAddress & ") attempting to login - " & DateAndTime.Now)
                w.Flush()
                w.Close()
            End If
        End Sub
    )
    logLoginError.Start()

End Sub

Answer 1

Careful with naming.

UserName is an identifier for a TextBox, but it's named like a String (which contains a user's name).

I'd declare and assign UserName like this:

Dim UserName As String = DirectCast(LoginUser.FindControl("UserName"), TextBox).Text

And now the rest of the code can refer to a string that represents a user's name without any further conversions.

---

logLoginError is an identifier for a Thread, but it's named like a Task (which runs on a thread).

loggerThread would be a more accurate name for it.

---

This code:

w.WriteLine(CurrentUser.ToString & "'s (" & IPAddress & ") account is locked out - " & DateAndTime.Now)

Is using way more String instances than you'd think:

• CurrentUser.ToString()
• "'s ("
• IPAddress
• ") account is locked out - "
• DateAndTime.Now[.ToString]

And also:

• The string resulting from CurrentUser.ToString() & "'s ("
• The string resulting from CurrentUser.ToString() & "'s (" & IPAdress
• The string resulting from CurrentUser.ToString() & "'s (" & IPAdress & ") account is locked out - "
• The string resulting from CurrentUser.ToString() & "'s (" & IPAdress & ") account is locked out - " & DateAndTime.Now

Are you sure DateAndTime isn't supposed to be DateTime? Also there's a typo in IPAdress, should be IPAddress.

To fix the number of String objects being created, you can use String.Format instead of inline concatenations - remember that String is an immutable class, "appending to it" doesn't really append anything, it just creates a new instance.

This would be cleaner and more efficient (multiline to avoid horizontal scrolling):

w.WriteLine(String.Format("{0}'s ({1}) account is locked out - {2}", 
                          CurrentUser.ToString, IPAddress, DateAndTime.Now))

Answer 2

What you could do is just move that outside of the if statement like this

Dim logLoginError As New Thread(
    Sub()
        w = File.AppendText(filePath)
        If (CurrentUser IsNot Nothing) Then
            If (CurrentUser.IsLockedOut = True) Then
                w.WriteLine(CurrentUser.ToString & "'s (" & IPAddress & ") account is locked out - " & DateAndTime.Now)
            ElseIf (CurrentUser.IsApproved = False) Then
                w.WriteLine(CurrentUser.ToString & " (" & IPAddress & ") is attempting to login, but the account is disabled - " & DateAndTime.Now)
            Else
                w.WriteLine(CurrentUser.ToString & "'s (" & IPAddress & ") login credentials failed - " & DateAndTime.Now)
            End If
        Else
            w.WriteLine("Unknown user " & UserName.Text & " (" & IPAddress & ") attempting to login - " & DateAndTime.Now)
        End If
        w.Flush()
        w.Close()
    End Sub
)

No matter what you do inside the if blocks you want w flushed and closed so do it outside of the if statement

---

The next thing that you should do is change the Conditionals to look like this:

If (CurrentUser.IsLockedOut) Then

And

ElseIf Not(CurrentUser.IsApproved) Then

And you might want to flip flop around the outside if Statement so that it is based on a positive conditional as well

Dim logLoginError As New Thread(
    Sub()
        w = File.AppendText(filePath)
        If (CurrentUser Is Nothing) Then
            w.WriteLine("Unknown user " & UserName.Text & " (" & IPAddress & ") attempting to login - " & DateAndTime.Now)
        Else
            If (CurrentUser.IsLockedOut) Then
                w.WriteLine(CurrentUser.ToString() & "'s (" & IPAddress & ") account is locked out - " & DateAndTime.Now)
            ElseIf Not(CurrentUser.IsApproved) Then
                w.WriteLine(CurrentUser.ToString() & " (" & IPAddress & ") is attempting to login, but the account is disabled - " & DateAndTime.Now)
            Else
                w.WriteLine(CurrentUser.ToString() & "'s (" & IPAddress & ") login credentials failed - " & DateAndTime.Now)
            End If
        End If
        w.Flush()
        w.Close()
    End Sub
)

This makes the code a whole lot cleaner and less redundant.

---

You call CurrentUser.ToString and it should be CurrentUser.ToString()

---

Answer 3

A bit similar to other response but you could separate the logic into two section. One that gets the message and the other that writes the message. If you remove concatenation, you can easily put your error message in the resource file. If you change your code a bit you could even move the String.Format to a single place.

Dim message As String = ""

If (CurrentUser IsNot Nothing) Then

    If (CurrentUser.IsLockedOut = True) Then
        message = String.Format("{0}'s ({1}) account is locked out - {2}", CurrentUser.ToString, IPAddress, DateAndTime.Now)
    ElseIf (CurrentUser.IsApproved = False) Then
        message = String.Format("{0} ({1}) is attempting to login, but the account is disabled - {2}", CurrentUser.ToString, IPAddress, DateAndTime.Now)
    Else
        message = String.Format("{0}'s ({1}) login credentials failed - {2}", CurrentUser.ToString, IPAddress, DateAndTime.Now)
    End If

Else
    message = String.Format("Unknown user {0} ({1}) attempting to login - {2}", UserName.Text, IPAddress, DateAndTime.Now)
End If

w = File.AppendText(filePath)
w.WriteLine(message)
w.Flush()
w.Close()

Note: By separating the logic, you can put them in two different function GetMessage and WriteMessage.