Arthur Dick, Peter Chwiecko
September 18, 2004
Assignment Specifications
The original code:Note: To compile, you will need the tio package, which can be found in the folder: /home/profs/becker/Courses/233/tio |
Why is this code bad?
|
Refactoring ProcessThe First Step:The first step I took was to build a reliable set of test cases for the code using JUnit. This allows us to make changes to the code while avoiding introducing bugs. If you can make small changes iteratively, testing after every change to be sure the integrity of the code remains intact, you can quickly and efficiently improve the quality of your existing code. If you try to attempt refactoring without solid tests, chances are you are going to introduce bugs and possibly not even realize it. I had to add some additional set and get methods to the Account class to even write a test suite, this made it obvious to me that one of the major problems with this code is that the user interface and data manipulation are not separated at all. Download my test suite here. OK, what next?The first thing I tried to do was to extract a class from Account to create a Display class. This separates the calculations done on the account in the Account class from the User Interface in the Display class. This proved to be quite difficult to do after the fact without changing the overall operation of the program. I found after doing this (I didn't do a great job... it could still be a lot better) the code did seem to be a lot more readable, even for a trivial program such as this. After this refactoring, the Account class is no longer responsible for printing to the screen/getting input from the user. Rather, it just receives a valid value for the account and can then do it's own calculations. And then...Next, I decided to do something about the redundant code checking if an account was open in many of the functions inside the Account class. To do this, I used the "Extract Method" refactoring to create an isAccountActive method. This way, if in the future the way an active account is determined changes, the code only needs to be modified in one spot, rather than having to find all the spots where the code was essentially cut-and-pasted. Similarly, more redundant code was removed by replacing it with the set and get methods written for the test cases. This increased the readability of the code by reducing the number of redundant "if" statements lying around. The set and get methods are also more intuitive for a reader, when compared to actually modifying the data. Finally,The final refactoring I applied was a substitute algorithm to the selectionScreen method in the Display class (used to be in the Account class). This part of the code just had an unnecessarily long switch statement (if you notice I left it out before), which was easily replaced by a little character arithmetic and an if statement. |
The refactored code:You will still need the tio package. |
Why is this code better?After going through all of these steps, the code is now easier to understand by someone who may read it in the future. Try reading both versions of Account.java side by side, and you should see the difference. Also, the amount of redundant code has been greatly reduced. By eliminating any duplicate code, the design is more solid, and therefore future changes would be easier to make to the refactored code. The refactoring process also helped me to see a rather large bug. In the original code, accounts are never actually closed once they are opened, I saw this easily and fixed it during the refactoring process. It's too bad I couldn't find a slightly more interesting piece of code to work with... but all my (surviving) code beyond cpsc233 is C code. Hopefully this helped a few people who bothered to read it anyway. |