One long method or many methods but much readable

I have re-factored one of the method as shown below ,personally I think it is much readable but one of my colleague does not really like it arguing that it is too many methods.Am I wrong ,if not how can I convinced him? Original Method:

   Private Sub SendTextMessage(ByVal sender As Object, ByVal eventArgs As EventArgs)
                    If String.IsNu开发者_JAVA技巧llOrEmpty(SMSUserControl.TxtPhoneProperty) Then
                        lblError.Text = Globalization.GetTranslation("PhoneNotSet", "A mobile phone number must be specified")
                        Exit Sub
                    End If

                    Try
                        If Not String.Equals(UserHelper.CurrentUser.Mobile, SMSUserControl.TxtPhoneProperty) Then
                            UserHelper.CurrentUser.Mobile = SMSUserControl.TxtPhoneProperty
                            UserHelper.CurrentUser.Properties.Save()
                        End If
                        If IPhoneProfilePresenter Is Nothing Then
                            IPhoneProfilePresenter = New IPhoneProfilePresenter(Me, CabFileNameManagerFactory.Create(), SMSSenderFactory.Create())
                        End If
                        IPhoneProfilePresenter.SendTextMessage(New TextMessageInfo(SMSUserControl.TxtPhoneProperty, GetOCSInstallerLink(), DownloadLink.Text))

                    Catch ex As Exception
                        lblError.Text = _
                            CWebLog.LogAndDisplayError(CurrentSession.IsFullLogging, _
                                                        Globalization.GetTranslation("MobilePhoneSave", _
                                                                                      "Failed to save the mobile phone number"), _
                                                        ex)
                    End Try
                End Sub

Re-factored Method:

    Private Sub SendTextMessage(ByVal sender As Object, ByVal eventArgs As EventArgs)
            If PhoneNumberIsNotSet() Then
                lblError.Text = Globalization.GetTranslation("PhoneNotSet", "A mobile phone number must be specified")
                Exit Sub
            End If

            Try
                If User_Input_Mobile_Is_Not_The_Same_As_The_One_Stored_In_The_Database() Then
                    UpdateMobile()

                End If
                GetIPhoneProfilePresenter().SendTextMessage(New TextMessageInfo(SMSUserControl.Mobile, GetOCSInstallerLink(), DownloadLink.Text))




            Catch ex As Exception
                lblError.Text = _
                    CWebLog.LogAndDisplayError(CurrentSession.IsFullLogging, _
                                                Globalization.GetTranslation("MobilePhoneSave", _
                                                                              "Failed to save the mobile phone number"), _
                                                ex)
            End Try
        End Sub

        Private Sub UpdateMobile()

            UserHelper.CurrentUser.Mobile = SMSUserControl.Mobile
            UserHelper.CurrentUser.Properties.Save()
        End Sub

        Private Function User_Input_Mobile_Is_Not_The_Same_As_The_One_Stored_In_The_Database() As Boolean

            Return Not String.Equals(UserHelper.CurrentUser.Mobile, SMSUserControl.Mobile)
        End Function

        Private Function PhoneNumberIsNotSet() As Boolean

            Return String.IsNullOrEmpty(SMSUserControl.Mobile)
        End Function

        Private Function GetIPhoneProfilePresenter() As IPhoneProfilePresenter

            If IPhoneProfilePresenter Is Nothing Then
                IPhoneProfilePresenter = New IPhoneProfilePresenter(Me, CabFileNameManagerFactory.Create(), SMSSenderFactory.Create())
            End If
            Return IPhoneProfilePresenter
        End Function

Re factored again based on feedback

Private Sub ValidateAndSaveMobilePhoneAndSendTextMessage(ByVal sender As Object, ByVal eventArgs As EventArgs)
            If ValidateMobilePhoneNumber() Then
                SaveMobilePhoneNumber()
                SendTextMessage()
            End If

        End Sub

        Private Sub SendTextMessage()

            If iPhoneProfilePresenter Is Nothing Then
                iPhoneProfilePresenter = New iPhoneProfilePresenter(Me, CabFileNameManagerFactory.Create(), SMSSenderFactory.Create())
            End If
            iPhoneProfilePresenter.SendTextMessage(New TextMessageInfo(SMSUserControl.TxtPhoneProperty, GetOCSInstallerLink(), DownloadLink.Text))
        End Sub

        Private Sub SaveMobilePhoneNumber()

            Try
                If Not String.Equals(Globalization.CurrentUser.Mobile, SMSUserControl.TxtPhoneProperty) Then
                    Globalization.CurrentUser.Mobile = SMSUserControl.TxtPhoneProperty
                    Globalization.CurrentUser.Properties.Save()
                End If
            Catch ex As Exception
                lblError.Text = _
                    CWebLog.LogAndDisplayError(CurrentSession.IsFullLogging, _
                                               ContentHelper.GetTranslation("MobilePhoneSave", _
                                                                            "Failed to save the mobile phone number"), _
                                               ex)
            End Try
        End Sub

        Private Function ValidateMobilePhoneNumber() As Boolean
            Dim isValid As Boolean = True
            If String.IsNullOrEmpty(SMSUserControl.TxtPhoneProperty) Then
                lblError.Text = ContentHelper.GetTranslation("PhoneNotSet", "A mobile phone number must be specified")
                isValid = False
            End If
            Return isValid
        End Function

Methods should "Do Only One Thing" (see this Coding Horror blog post for more details). This is also known as the Single Responsibility Principle.

Methods should be short(ish), easily understood and have no side effects. You should be able to describe what a method does in it's name. For example, if your method is VerifyAddressAndAddToDatabase then it needs splitting into two methods - VerifyAddress and AddToDatabase.

In the past it was considered a bad idea to call lots of methods (or procedures and functions as they were then called) as there were overheads involved. But with modern compilers and faster processors this is not an issue.

What you are concerned about is probably Cyclomatic complexity, the number of different paths that code can take. This can be used a metric to identify pieces of code that are good candidates for being refactored.

Having high per-method cyclomatic complexity suggests a method is getting too complicated.

in terms of not only yourself working on it but any one else who works with it will have a tough time. It becomes difficult to

• test
• maintain and
• refactor

that piece of code later on. A better practice is to keep Cyclomatic complexity under or equal to 2; i.e your method should not take more then 2 paths while getting to a decision.

Get rid of the 'User_Input...' function; put that code inline. Otherwise, it's acceptable.

Function names like that are just ridiculous, IMHO, because it may be that you want to do slightly more than that at a later time, and then you'd need to change the name (lest it be misleading). Also, it's annoying to read, IMHO.

Ask him why he is concerned with having "too many" methods. Generally speaking, function calls in modern languages do not add significant overhead. It's usually far more economical to save time for the programmer trying to read his colleagues' code rather than saving a few clock cycles for the processor.

Personally, I think it's nice when reading a function provides a high-level overview of what it does (because it consists of calls to other methods that are informatively named), rather than me having to work out the intent myself.

That said, and as pointed out by others, User_Input_Mobile_Is_Not_The_Same_As_The_One_Stored_In_The_Database() is not the cleanest method name I've ever seen.

You've reduced the original method by 1 if statement, and added 4 more methods in exchange. I would say that this makes the code more complex.

The original is really a 3 statement method. This is short enough it doesn't need refactoring. The huge name for one of the refactored methods is a hint that it isn't a good statement to refactor.

I would just add more comments to the original, especially a header comment for the function as a whole.

NOTE: this isn't a direct answer to your question, but i couldn't fit it into a comment...

What is the class name/structure? I tend to go by the rule-of-thumb that method names should be in the context of the class name where possible.

If your class is MobileContact then MobileContact.SaveMobilePhoneNumber() can become MobileContact.Save() or MobileContact.User_Input_Mobile_Is_Not_The_Same_As_The_One_Stored_In_The_Database() could become MobileContact.IsDirty().