Tutorial :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  


Solution: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.


Solution: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.


Solution:3

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.


Solution:4

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.


Solution:5

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.


Solution:6

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


Note:If u also have question or solution just comment us below or mail us on toontricks1994@gmail.com
Previous
Next Post »