3

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
Ybbest
  • 1,520
  • 3
  • 29
  • 43

6 Answers6

3

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.

Dave Jarvis
  • 30,436
  • 41
  • 178
  • 315
ChrisF
  • 134,786
  • 31
  • 255
  • 325
  • 1
    ChrisF: FWIW, I don't think the problem is with accepting that methods should 'do one thing' (I think this is obvious), the issue is helping people realise what that 'one thing' should be :) (i.e. compact but complete). – Noon Silk Jan 10 '10 at 22:07
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.

Jim Lewis
  • 43,505
  • 7
  • 82
  • 96
Asad
  • 21,468
  • 17
  • 69
  • 94
  • 1
    +1 for mentioning a useful metric that I hadn't encountered before. I hope you don't mind me editing a few links into your answer to save other readers some googling... – Jim Lewis Jan 10 '10 at 22:55
  • asdi: Uh, it's not cyclomatic complexity here. He's not adding more paths, he's just trying to abstract out things that don't need to be. – Noon Silk Jan 10 '10 at 23:02
  • Thanks Jim and silky, do appreciate, I should have taken care of that myself. – Asad Jan 11 '10 at 00:02
1

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.

Noon Silk
  • 54,084
  • 6
  • 88
  • 105
  • Thanks ,whey I need to put this function inline? – Ybbest Jan 10 '10 at 21:56
  • Ybbest: As I said, it doesn't make a lot of sense to abstract it out. I doubt there are other places in your code calling it (infact, ideally it should be done somewhere else altogether, but nevertheless) and as it stands, it's just weird to have to look elsewhere for that logic. – Noon Silk Jan 10 '10 at 22:01
  • I have re-factored the code again based on re-factoring,please check it again. – Ybbest Jan 10 '10 at 22:22
  • I guess I am working with a very large legacy code and does not have any unit tests.That's probably the best I can do so far.Thanks for all your comments. – Ybbest Jan 10 '10 at 23:25
1

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.

Larry Watanabe
  • 10,126
  • 9
  • 43
  • 46
  • More comments in a big method will create a maintenance nightmare. – gingerbreadboy Jan 10 '10 at 22:08
  • He doubled the code size - how is that more readable? The original has 3 if statements - how is that a large method? – Larry Watanabe Jan 10 '10 at 22:11
  • 1
    I agree. Private helper methods like UpdateMobile()--called only for side-effects and taking no inputs--make the second example much harder to understand. Method names should be descriptive, but clear and obvious code beats a descriptive method name. TrustMe(). – Ken Fox Jan 10 '10 at 22:15
  • 1
    BTW I disagree with the "just add more comments" idea. Adding comments to the first code will not make it better. Improving the variable names and factoring out the class dependencies would make the code better. – Ken Fox Jan 10 '10 at 22:21
  • I don't disagree that it isn't the best re factoring job in the world, but if you need to add a load of comments to make it more understandable it should prob be refactored first. Code should be self documenting. – gingerbreadboy Jan 10 '10 at 22:22
1

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.

danben
  • 80,905
  • 18
  • 123
  • 145
  • Appreciate his effort for suggesting such a good name. best in terms of readability – Asad Jan 10 '10 at 22:02
0

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

gingerbreadboy
  • 7,386
  • 5
  • 36
  • 62
  • This is inside asp.net page behind code.The class name called IPhoneProfile.cs it is the code behind for IPhoneProfile.aspx – Ybbest Jan 10 '10 at 23:22
  • 1
    I would be inclined to move this logic out of the code behind and into its own business object (maybe IPhone.cs) and call into the class IPhone.Update(...details...) and maybe return a status. See http://www.codeproject.com/KB/cs/person.aspx for a hint. – gingerbreadboy Jan 10 '10 at 23:56
  • I actually don't really like that first link but can't find a good example. This is the next best thing http://stackoverflow.com/questions/686960/datatable-wrapper-or-how-to-decouple-ui-from-business-logic or maybe http://www.c-sharpcorner.com/UploadFile/rmcochran/MVC_intro12122005162329PM/MVC_intro.aspx for some MVC goodness. – gingerbreadboy Jan 11 '10 at 00:05
  • 1
    I think there is a need for separation for presentation and logic here as well, as suggested by runrunragn. program needs to define boundaries and organisation between "Presentation Layer" and "Business Logic Layer". – Asad Jan 11 '10 at 00:11
  • This can not be done as the legacy code is doing all saving for me.It is not feasible to create such class.It would be nice to do that way. ): – Ybbest Jan 11 '10 at 00:33
  • As you can see from my code , I use the presenter inside the code behind and do all my best to use MVP and also not to break anything in the legacy code. I do understand the separation for presentation and logic ,as you can see from the method call below I delegate the message sending to the presenter and inside the presenter I got SMSSender to take care of this.But I could not do the same mobile phone number due to the legacy code. IPhoneProfilePresenter.SendTextMessage(New TextMessageInfo(SMSUserControl.TxtPhoneProperty, GetOCSInstallerLink(), DownloadLink.Text)) – Ybbest Jan 11 '10 at 00:37
  • Legacy code can be a right pain. I'd still be quite interested to look at the complete code behind if it's not too sensitive, my email is on my profile if you don't want to post it. If you're interested that is. – gingerbreadboy Jan 11 '10 at 00:43