[RESOLVED] Long methods

[chris128]

I understand that it is good practice to break up your application's code into separate blocks - so for example you would not just have your entire code in a button click event, you would break up the parts of the procedure into methods that call each other. Normally I follow this practice but in my current little project (creating an SMTP server) I am finding that difficult. I'll explain...

I have a method that is called each time a remote server connects to my SMTP server and tries to pass a message to it, this method needs to read the data that the remote server has sent and then respond to the remote server appropriately and store certain parts of the data the remote server sent (ie the sender address of the email message, the recipient address, the body of the email etc). Now you would think that this is quite easy to separate into different methods as you would just have one function that reads the data, then a quick IF statement to see what the command was that the remote server sent, and then a separate procedure for each of the possible commands that takes an appropriate action.

However, because the actions of each of these parts all need to access so many things that are declared in the original method, it seems pointless just passing these items around through parameters in functions/subs just for the sake of the code looking a bit neater. Its kind of hard to explain but it seems to me that without either declaring all these things at class level (which looks pretty naff to me) or passing loads of object references around between each method, then there isnt really any obvious way of separating this code.

Maybe its just me - maybe this method is no where near what you would consider "too long" and maybe the amount of objects I would need to pass to each method if I separated it all would not be considered anything out of the ordinary but just have a look at the code below and see what you think... As you can see, I have defined separate procedures for some things, such as the WriteToClientStream and SMTPSendMessage but I'm struggling to find any more things that are self contained enough to split out

''' <summary>
''' Handles the session and command processing for each connected client
''' </summary>
''' <param name="AcceptedTcpClient">The TcpClient object that represents the connected client</param>
Private Sub ManageSession(ByVal AcceptedTcpClient As Object)
 
        Dim RemoteClient As TcpClient = DirectCast(AcceptedTcpClient, TcpClient)
        Dim RemoteIP As String = RemoteClient.Client.RemoteEndPoint.ToString
        Dim ClientStream As NetworkStream = RemoteClient.GetStream
 
        'Respond to the connection request and raise the ClientConnected event
        WriteToClientStream(ClientStream, "220 Prototype SMTP Server , ready at " & Now.ToString)
        RaiseEvent ClientConnected(New SMTPSessionEventArgs(RemoteIP, String.Empty, String.Empty))
 
 
        'Flags that are used to track the current progress of the session
        Dim QuitCommandReceived As Boolean = False
        Dim InDataCommand As Boolean = False
        Dim MessageSent As Boolean = False
 
        'Email attributes populated by received SMTP commands
        Dim SenderAddress As String = String.Empty
        Dim RecipientAddresses As New List(Of String)
        Dim Subject As String = String.Empty
        Dim MessageBody As String = String.Empty
 
        'Will contain the string of data received from the remote client
        Dim ReceivedStringBuilder As New StringBuilder
 
        Do While QuitCommandReceived = False
            Do
                Dim size As Integer = 0
                Dim DataAsBytes(1024) As Byte
                Do While ClientStream.DataAvailable
                    size = ClientStream.Read(DataAsBytes, 0, DataAsBytes.Length)
                    Dim ShortDataAsString As String = System.Text.Encoding.ASCII.GetString(DataAsBytes, 0, size)
                    ReceivedStringBuilder.Append(ShortDataAsString)
                Loop
 
                If InDataCommand Then
                    If ReceivedStringBuilder.ToString.EndsWith(vbCrLf & "." & vbCrLf) Then
                        WriteToClientStream(ClientStream, "250 Message queued for delivery.")
                        MessageBody = ReceivedStringBuilder.ToString.Remove(ReceivedStringBuilder.Length - 3, 3)
                        SMTPSendMessage(SenderAddress, RecipientAddresses, Subject, MessageBody, RemoteIP)
                        InDataCommand = False
                        SenderAddress = String.Empty
                        RecipientAddresses.Clear()
                        Subject = String.Empty
                        MessageBody = String.Empty
                        MessageSent = True
                        Exit Do
                    End If
                ElseIf InDataCommand = False Then
                    If ReceivedStringBuilder.ToString.EndsWith(vbCrLf) Then
                        Exit Do
                    End If
                End If
            Loop 'Start listening for new data on the stream again
 
            If MessageSent Then
                MessageSent = False
            Else
                Dim CommandEntered As String = String.Empty
                CommandEntered = ReceivedStringBuilder.ToString
                RaiseEvent CommandReceived(New SMTPSessionEventArgs(RemoteIP, CommandEntered, String.Empty))
 
                '-------- HELO Command ---------
                If String.Compare(CommandEntered, "HELO" & vbCrLf, True) = 0 Then
                    WriteToClientStream(ClientStream, "250 HELO From " & RemoteClient.Client.LocalEndPoint.ToString)
 
                    '--- EHLO Command ---
                ElseIf CommandEntered.StartsWith("EHLO ", StringComparison.CurrentCultureIgnoreCase) Or String.Compare(CommandEntered, "EHLO" & vbCrLf, True) = 0 Then
                    WriteToClientStream(ClientStream, "250 OK")
 
                    '--- MAIL FROM Command ---
                ElseIf CommandEntered.StartsWith("MAIL FROM:", StringComparison.CurrentCultureIgnoreCase) Then
                    If Not SenderAddress = String.Empty Then
                        WriteToClientStream(ClientStream, "503 5.5.2 Sender already specified")
                    Else
                        Dim RequestedSenderAddress As String = CommandEntered.Substring(CommandEntered.IndexOf(":") + 1).Trim
                        If RequestedSenderAddress.Contains("@") = False Then
                            WriteToClientStream(ClientStream, "501 5.5.4 Invalid Address")
                        Else
                            SenderAddress = RequestedSenderAddress
                            WriteToClientStream(ClientStream, "250 2.1.0 " & SenderAddress & "....Sender OK")
                        End If
                    End If
 
                    '--- RCPT TO Command ---
                ElseIf CommandEntered.StartsWith("RCPT TO:", StringComparison.CurrentCultureIgnoreCase) Then
                    If SenderAddress = String.Empty Then
                        WriteToClientStream(ClientStream, "503 5.5.2 Need Mail From: first")
                    Else
                        Dim RequestedRecipientAddress As String = CommandEntered.Substring(CommandEntered.IndexOf(":") + 1).Trim
                        If RequestedRecipientAddress.Contains("@") = False Then
                            WriteToClientStream(ClientStream, "501 5.5.4 Invalid Address")
                        Else
                            RecipientAddresses.Add(RequestedRecipientAddress)
                            WriteToClientStream(ClientStream, "250 2.1.5 " & RequestedRecipientAddress)
                        End If
                    End If
 
                    '--- DATA Command ---
                ElseIf String.Compare(CommandEntered, "DATA" & vbCrLf, True) = 0 Then
                    If SenderAddress = String.Empty Then
                        WriteToClientStream(ClientStream, "503 5.5.2 Need mail command.")
                    ElseIf RecipientAddresses.Count < 1 Then
                        WriteToClientStream(ClientStream, "503 5.5.2 Need Rcpt command.")
                    Else
                        WriteToClientStream(ClientStream, "354 Start mail input; end with <CRLF>.<CRLF>")
                        InDataCommand = True
                    End If
 
                    '--- AUTH LOGIN Command ---
                    ElseIf String.Compare(CommandEntered, "AUTH LOGIN" & vbCrLf, True) = 0 Then
                    'Havent got this far yet...
 
                    '--- RSET Command ---
                ElseIf String.Compare(CommandEntered, "RSET" & vbCrLf, True) = 0 Then
                    SenderAddress = String.Empty
                    RecipientAddresses.Clear()
                    Subject = String.Empty
                    MessageBody = String.Empty
                    InDataCommand = False
                    WriteToClientStream(ClientStream, "250 2.0.0 Resetting")
                    '--- QUIT Command ---
                ElseIf String.Compare(CommandEntered, "QUIT" & vbCrLf, True) = 0 Then
                    QuitCommandReceived = True
                    RemoteClient.Close()
                    '--- Unknown Command ---
                Else
                    WriteToClientStream(ClientStream, "500 5.3.3 Unknown Command: " & CommandEntered)
                End If
            End If
 
            'Clear out the command string ready for next command
            ReceivedStringBuilder.Remove(0, ReceivedStringBuilder.Length)
 
        Loop 'Start processing commands that are sent from the client again (unless QUIT has been sent)
End Sub

[chris128]

Oh and if you are wondering why the AcceptedTcpClient parameter of the method is just of type Object, its because this is how the method is called:

'When a new client connection is accepted, start the ManageSession method on a new ThreadPool thread
'and then loop back to get ready for new client to connect
Do While DoListen
      Threading.ThreadPool.QueueUserWorkItem(AddressOf ManageSession, TListener.AcceptTcpClient())
Loop

I would have just added this as an edit to the original post but apparently that makes the post too long

Oh and one random question - is a Sub/Function that is Private still called a Method or is Method just a term for public Subs and Functions?

[jmcilhinney]

I haven't read all your code but the first place I can see that could be refactored would be lines 35 to 37:

ReceivedStringBuilder.Append(ReadDataString(ClientStream))

Private Function ReadDataString(ByVal stream As Stream) As String
    Dim data(1023) As Byte
 
    stream.Read(data, 0, data.Length)
 
    Return System.Text.Encoding.ASCII.GetString(data)
End Function

Notice that I use 1023 as the upper bound of the array, so the Length is 1024.

That's a private method.

You could pass in a Byte array to avoid recreating an array each call but, unless that's going to make a significant difference to the performance then I'd suggest not. You should strive to write code that is as efficient as possible but you should also strive to write code that is as maintainable as possible too, so you need to balance the two.

[Shaggy Hiker]

I haven't read through the code, either, but it happens. Heck, the bulk of it is just an ElseIf ladder with relatively small blocks. Nothing surprising about that.

My point is that you make it sound like the code is less right because it is a long method. As long as the code works, it is only less right if there is an alternative way to write the same thing such that it is more efficient. When it comes to Select Case and ElseIf ladders, that generally doesn't happen.

[chris128]

I haven't read through the code, either, but it happens. Heck, the bulk of it is just an ElseIf ladder with relatively small blocks. Nothing surprising about that.

My point is that you make it sound like the code is less right because it is a long method. As long as the code works, it is only less right if there is an alternative way to write the same thing such that it is more efficient. When it comes to Select Case and ElseIf ladders, that generally doesn't happen.

Thanks thats kind of why I posted it, because I thought it might just be me thinking it is bad when its not really. I appreciate the code is a bit long for everyone to go through each line so thanks for the comments anyway.

JMC - thanks for that suggestion, I did consider doing that before but I wasnt sure if passing the network stream object around a lot was perhaps going to be detrimental to performance. Guess I should give it a go and see what happens through.
That brings me on to another question about how passing reference types by value works, but I think that belongs in a new thread..

So anyway, the general concencus then is that I could probably refactor a few little bits but in general it doesnt look like the method is really unnecessarily long?

[jmcilhinney]

Passing any variable by value always works the same way: it copies the value. If that variable is a reference type then you're simply copying a reference, which is exactly why reference types exist. This allows you to pass large objects around without making multiple copies of the object.

[Shaggy Hiker]

I'd say that the answer is Yes.

[techgnome]

So anyway, the general concencus then is that I could probably refactor a few little bits but in general it doesn't look like the method is really unnecessarily long?

I'd agree with that too. I've seen worse, and when you mentioned that it was long, I was surprised to find it weighing at 140 lines. That's typical. I've seen routines that weigh in at 500+ (and sadly with no way to break it out further)... I didn't read all of the code, but scanned through it... and it seems like it's already about as tight as it is going to get.

-tg

[chris128]

Cool, well thanks for the comments guys just my lack of real world developer experience showing through I guess :P