Sunday, October 08, 2006
Decompose Conditional

A lot of complexity in programs comes from complex if-then-else statements. If it takes you a long time to read and understand an if-then-else statement you should take a look at the Decompose Conditional refactoring.

To show you how powerful this can be, from the example below choose which code you think is easier to understand:

Public Sub ProcessPayment()
            If _view.PaymentType = PaymentType.Cash Then
                'payment type is cash
                    Try
                        _task.SaveCashPayment(_view.Amount)
                    Catch ex As Exception
                        _view.ShowMessage("There was a problem saving the payment.", False)
                    End Try
                    _view.CloseView()
                End If
            Else
                'payment type is credit card
                If PreAuthorized(_view.Amount)) Then
                    Try
                        _task.SaveCreditCardPayment(_view.Amount)
                    Catch ex As Exception
                        _view.ShowMessage("There was a problem saving the payment.", False)
                    End Try
                    _view.CloseView()
                End If
            End If
End Sub

Or this code:

Public Sub ProcessPayment()
            If PaymentTypeIsCash() Then
                SaveCashPayment()
            Else
                SaveCreditCardPayment()
            End If
End Sub

In the first block of code, notice how it takes you a long time to figure out what the code is trying to accomplish while the second block of code reads like a couple of lines of comments.

Also, notice how the first block has a lot of code duplication. Once the SaveCashPayment and SaveCreditCardPayment are moved into their own methods it is a lot easier to recognize this duplication and refactor it. 

If there are other ways you can think of to refactor this code please post in the comments, I am always looking for ways to make code better.

Update: Check out Jean-Paul Boodhoo's response on how to further  refactor this code.

 

[ Currently Playing : Reign On - The Brian Jonestown Massacre - Bringing It All Back Home Again (4:31) ]

Programming | Technical
Sunday, October 08, 2006 5:10:59 AM (GMT Standard Time, UTC+00:00)  #    Comments [3]  |  Tracked by:
"Refactoring Ahoy!!" (Jean-Paul S. Boodhoo's Blog) [Trackback]
http://www.jpboodhoo.com/blog/PermaLink,guid,ca05b6e6-4cee-41fc-b0ef-e598308b835... [Pingback]
Tuesday, October 10, 2006 6:25:38 PM (GMT Standard Time, UTC+00:00)
Two things I noticed when reading this.

First, although the _view.PaymentType is comparing against an enum (suggesting multiple values), the 'else' block doesn't check for a value and just assumes that if PaymentType != PaymentType.Cash then it's a CC payment. Perhaps not a huge deal, but a good thing to check with an 'else-if' block and then have an 'else' block at the end that throws and exceptions saying 'Unknown Payment Type.'

The second thing, and the far cooler one, is that this style of coding has a name! Now I can sound all important like those AAE devs by throwing around a bunch of techno-mumbo-jumbo in my conversations.

I love this style of coding. It hides the complexity, makes the code easier to read and, depending on how it's written, the code can be far easier to test too.
Donald Belcham
Tuesday, October 10, 2006 7:12:16 PM (GMT Standard Time, UTC+00:00)
"_view.PaymentType is comparing against an enum (suggesting multiple values)"

Good call. CC payment shouldn't be assumed.

The other thing I noticed after posting was that the first block of code shouldn't be catching all exceptions, it should only catch specific ones.
Steven R
Tuesday, October 10, 2006 10:01:57 PM (GMT Standard Time, UTC+00:00)
Hey Steven,

I made a comment on some other refactorings you could try here:

http://www.jpboodhoo.com/blog/PermaLink,guid,ca05b6e6-4cee-41fc-b0ef-e598308b835f.aspx

JP
Jean-Paul S. Boodhoo
Comments are closed.

Theme design by Jelle Druyts

Pick a theme: