patternMinor
Simplify ASP.Net StreamWriter for appendtext to file
Viewed 0 times
streamwriterfileappendtextnetsimplifyforasp
Problem
Is there anyway to simplify this code so there aren't so many redundant
Anything else I have tried ends up locking the file, and crashing the site because to text file is in use by another process.
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 SubSolution
Careful with naming.
I'd declare and assign
And now the rest of the code can refer to a string that represents a user's name without any further conversions.
This code:
Is using way more
And also:
Are you sure
To fix the number of
This would be cleaner and more efficient (multiline to avoid horizontal scrolling):
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).TextAnd 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))Code Snippets
Dim UserName As String = DirectCast(LoginUser.FindControl("UserName"), TextBox).Textw.WriteLine(CurrentUser.ToString & "'s (" & IPAddress & ") account is locked out - " & DateAndTime.Now)w.WriteLine(String.Format("{0}'s ({1}) account is locked out - {2}",
CurrentUser.ToString, IPAddress, DateAndTime.Now))Context
StackExchange Code Review Q#51838, answer score: 4
Revisions (0)
No revisions yet.