One long method or many methods but much readable

Question

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.IsNullOrEmpty(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

Answer 1

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.

Answer 2

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.