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))

• RubberDuck: String.Format reminds me of Ruby. Shiny.

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.