HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Simplify ASP.Net StreamWriter for appendtext to file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
streamwriterfileappendtextnetsimplifyforasp

Problem

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

Solution

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

Code Snippets

Dim UserName As String = DirectCast(LoginUser.FindControl("UserName"), TextBox).Text
w.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.