symptoms: a method contains too many lines of code.
treatment:
if you feel the need to comment on something inside a method, you should take this code and put it into a new method
1.extract method with a good name
(Extract Method refactoring has the following limitations:
Refactoring does not work with multiple output values in automatic mode. You have to change your code before applying the refactoring.
Refactoring does not work for a code fragment which conditionally returns from the containing method and is not placed at the end of it.即 function 裡面有提前return)
how to extract
find the local variables or parameters first
non modified variable in the segment pass as a parameter
modified variables in the segment initialized as local variables in the new method , in the end return the variables
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 |
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + getName() + "\n"; while (rentals.hasMoreElements()) { double thisAmount = 0; Rental each = (Rental) rentals.nextElement(); //determine amounts for each line switch (each.getMovie().getPriceCode()) { case Movie.REGULAR: thisAmount += 2; if (each.getDaysRented() > 2) thisAmount += (each.getDaysRented() - 2) * 1.5; break; case Movie.NEW_RELEASE: thisAmount += each.getDaysRented() * 3; break; case Movie.CHILDRENS: thisAmount += 1.5; if (each.getDaysRented() > 3) thisAmount += (each.getDaysRented() - 3) * 1.5; break; } // add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) && each.getDaysRented() >1) frequentRenterPoints ++; //show figures for this rental result += "\t" + each.getMovie().getTitle()+ "\t" + String.valueOf(thisAmount) + "\n"; totalAmount += thisAmount; } //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } ==================================================> public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + getName() + "\n"; while (rentals.hasMoreElements()) { double thisAmount = 0; Rental each = (Rental) rentals.nextElement(); thisAmount = <span style="color: #ff0000;">amountFor</span>(each); // add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) && each.getDaysRented() > 1) frequentRenterPoints ++; //show figures for this rental result += "\t" + each.getMovie().getTitle()+ "\t" + String.valueOf(thisAmount) + "\n"; totalAmount += thisAmount; } //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } private double amountFor(Rental aRental) { double result = 0; switch (aRental.getMovie().getPriceCode()) { case Movie.REGULAR: result += 2; if (aRental.getDaysRented() > 2) result += (aRental.getDaysRented() - 2) * 1.5; break; case Movie.NEW_RELEASE: result += aRental.getDaysRented() * 3; break; case Movie.CHILDRENS: result += 1.5; if (aRental.getDaysRented() > 3) result += (aRental.getDaysRented() - 3) * 1.5; break; } return result ; } |
2.replace temp with query (remove temp)
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 |
public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + getName() + "\n"; while (rentals.hasMoreElements()) { double thisAmount = 0; Rental each = (Rental) rentals.nextElement(); thisAmount = amountFor(each); // add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) && each.getDaysRented() > 1) frequentRenterPoints ++; //show figures for this rental result += "\t" + each.getMovie().getTitle()+ "\t" + String.valueOf(thisAmount) + "\n"; totalAmount += thisAmount; } //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } ======================================> public String statement() { double totalAmount = 0; int frequentRenterPoints = 0; Enumeration rentals = _rentals.elements(); String result = "Rental Record for " + getName() + "\n"; while (rentals.hasMoreElements()) { Rental each = (Rental) rentals.nextElement(); // add frequent renter points frequentRenterPoints ++; // add bonus for a two day new release rental if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) && each.getDaysRented() > 1) frequentRenterPoints ++; //show figures for this rental result += "\t" + each.getMovie().getTitle()+ "\t" + String.valueOf(amountFor(each)) + "\n"; totalAmount += amountFor(each); } //add footer lines result += "Amount owed is " + String.valueOf(totalAmount) + "\n"; result += "You earned " + String.valueOf(frequentRenterPoints) + " frequent renter points"; return result; } |
3.introduce parameter object (remove parameters)
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 |
class Account { private Vector _entries = new Vector(); double getFlowBetween (Date start, Date end) { double result = 0; Enumeration e = _entries.elements(); while (e.hasMoreElements()) { Entry each = (Entry) e.nextElement(); if (each.getDate().equals(start) || each.getDate().equals(end) || (each.getDate().after(start) && each.getDate().before(end))) { result += each.getValue(); } } return result; } ... } client code... double flow = anAccount.getFlowBetween(startDate, endDate); =========introduce parameter object========> class DateRange { DateRange (Date start, Date end) { _start = start; _end = end; } Date getStart() { return _start; } Date getEnd() { return _end; } private final Date _start; private final Date _end; } class Account { double getFlowBetween (<span style="color: #ff0000;">DateRange range</span>) { double result = 0; Enumeration e = _entries.elements(); while (e.hasMoreElements()) { Entry each = (Entry) e.nextElement(); if (each.getDate().equals(range.getStart()) || each.getDate().equals(range.getEnd()) || (each.getDate().after(range.getStart()) && each.getDate().before(range.getEnd()))) { result += each.getValue(); } } return result; } ... } client code... double flow = anAccount.getFlowBetween(<span style="color: #ff0000;">new DateRange(startDate, endDate)</span>); ======move method into our parameter object=======> class Account { double getFlowBetween (DateRange range) { double result = 0; Enumeration e = _entries.elements(); while (e.hasMoreElements()) { Entry each = (Entry) e.nextElement(); if (range.<span style="color: #ff0000;">includes</span>(each.getDate())) { result += each.getValue(); } } return result; } ... } class DateRange { boolean includes (Date arg) { return (arg.equals(_start) || arg.equals(_end) || (arg.after(_start) && arg.before(_end))); } ... } |
4.preserve whole object
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 |
class Room { boolean withinPlan(HeatingPlan plan) { int low = daysTempRange().getLow(); int high = daysTempRange().getHigh(); return plan.withinRange(low, high); } ... } class HeatingPlan { private TempRange _range; boolean withinRange (int low, int high) { return (low >= _range.getLow() && high <= _range.getHigh()); } ... } ============preserve whole object==================> class HeatingPlan { private TempRange _range; boolean withinRange (TempRange roomRange) { return (roomRange.getLow() >= _range.getLow() && roomRange.getHigh() <= _range.getHigh()); } ... } class Room { boolean withinPlan(HeatingPlan plan) { return plan.withinRange(<span style="color: #ff0000;">daysTempRange</span>()); } ... } ==============move method into object=============================> class HeatingPlan { private TempRange _range; boolean withinRange (TempRange roomRange) { return <span style="color: #000000;">_range</span>.<span style="color: #ff0000;">includes</span>(roomRange); } ... } class TempRange { boolean <span style="color: #ff0000;">includes</span> (TempRange arg) { return arg.getLow() >= this.getLow() && arg.getHigh() <= this.getHigh(); } ... } class Room { boolean withinPlan(HeatingPlan plan) { return plan.withinRange(daysTempRange()); } ... } |
5.replace method with method object
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 |
class Order { // ... public function price() { $primaryBasePrice = 10; $secondaryBasePrice = 20; $tertiaryBasePrice = 30; // Perform long computation. } } ==================================> class Order { // ... public function price() { return (new PriceCalculator($this))->compute(); } } class PriceCalculator { private $primaryBasePrice; private $secondaryBasePrice; private $tertiaryBasePrice; public function __construct(Order $order) { // Copy relevant information from the // order object. } public function compute() { // Perform long computation. } } |
6.decompose conditional
|
1 2 3 4 5 6 7 8 9 10 11 12 13 |
if ($date->before(SUMMER_START) || $date->after(SUMMER_END)) { $charge = $quantity * $winterRate + $winterServiceCharge; } else { $charge = $quantity * $summerRate; } =======================================> if (isSummer($date)) { $charge = summerCharge($quantity); } else { $charge = winterCharge($quantity); } |