# Simplify ASP.Net StreamWriter for appendtext to file

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

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
)

End Sub


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 ("
• ") 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}",

• String.Format reminds me of Ruby. Shiny. – RubberDuck Jun 2 '14 at 9:20

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

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

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