Máme v programu ošklivý, ale bezvadně fungující, kus kódu. Zrefaktorizujeme ho?
Otázka | Pro | Proti |
---|---|---|
Máme testy? | ano | ne |
Bude kód často upravován? | ano | ne |
Velká cena chyb? | ne | ano |
Refaktorizací musíme získat víc, než vynaložíme.
Některé metodiky vývoje software, jako extrémní programování a programování řízené testy, považují refaktorizaci za samozřejmou součást života kódu. Doporučují refaktorizovat kdykoliv je k tomu příležitost s tím, že veškerý kód je pokryt testy, takže se není třeba příliš obávat zanesení chyb.
Refaktorizace má i psychologický efekt: Programátory většinou potěší, že se jim podařilo z programu dostat pryč kus entropie. A s výsledným čistším kódem se jim lépe pracuje. To má i "manažerské" dopady jako nižší burn-out rate a tedy menší fluktuaci zaměstnanců, což zvyšuje produktivitu. Může být tedy vhodné nechat programátory refaktorizovat i ve chvíli, kdy refaktorizace sama o sobě nic podstatného nepřinese.
Příliš dlouhá funkce: > 1–2 obrazovky textu, cca 50 řádků
Příliš velká třída: velké množství atributů, typicky následované duplicitním kódem.
switch
Úpravy různých metod v důsledku změny: typicky pomůže rozdělit třídu tak, aby jeden typ změn ovlivňoval pouze jednu z nových tříd.
Změna vyžadující mnoho úprav ve více třídách: měněné chování je nutné dát dohromady pomocí přesunů metod a atributů.
Paralelní hierarchie: shodné prefixy názvů v obou hierarchiích, většinou pomůže přesun metod a atributů do jedné hierarchie.
Metoda se příliš zajímá o jinou třídu: Bývá signálem k přesunu metody do jiné třídy.
Příkazy switch
: Často je lepší využít
polymorfismus.
Shluky dat: Skupina atributů používaných současně. Typicky pomůže uzavření do vlastního objektu.
Přílišné používání primitivních typů: Správně bychom si měli definovat vlastní datové typy odpovídající sémantice kódu. Bohužel, zdaleka ne ve všech jazycích je to jednoduše možné. Např. v Javě je jedinou možností, jak definovat vlastní datový typ, vytvořit novou třídu. S tou se pak ale nedá pohodlně pracovat (mj. kvůli absenci přetěžování operátorů). Obecně platí, že prakticky všechny typové systémy v běžné používaných jazycích jsou poměrně slabé a umožňují toho vyjádřit málo.
Pomocné atributy třídy: atributy nastavonány jen někdy v některých metodách. Typicky pomůže zapouzdření takových atributů do samostatné třídy.
Podtřída se nehlásí k "odkazu" svého předka: Nejspíš je na místě kompozice místo dědičnosti.
Komentáře používané na vysvětlení obtížně pochopitelného kódu: Je-li to možné, je dobré se držet zásady "Don't document bad code – rewrite it" (Kernighan a Plauger, 1978).
Uvedené "code smells" byly převzaty z knih Steva McConnela (Code Complete) a Martina Fowlera (Refactoring). Na jiných místech můžete najít mnohé další, někdy i s návody na to, kterou refaktorizaci použít k nápravě:
break
nebo return
místo řídící
proměnné cyklu
if
-then
-else
switch
)
polymorfismem
null
Sloučení duplicitních fragmentů kódu v různých částech podmínky:
Máte-li například stejný kus kódu na konci if
-větve a
else
-větve podmínky, měli byste ho přesunout až za
podmínku. Samozřejmě můžou nastat i podstatně složitější případy,
kdy bude nutná extrakce do metody.
Zapouzdření přetypování směrem dolů (downcastingu): Pokud například
píšete třídu EmployeeList
zapouzdřující seznam objektů
typu Employee
, měly by jeho metody vracet tento typ, i
když třeba nějaká podkladová třída pracuje s obecným
Object
. Přetypování by mělo být třídou zapouzdřeno.
Náhrada virtuálních metod inicializací dat: Pokud se podtřídy neliší kódem, ale jen hodnotou některých svých atributů, stačí v nich tyto atributy nainicializovat a společný kód ponechat v bázové třídě.
Skrytí delegáta: Představte si, že třída A volá třídu B a pak třídu C. Nebylo by lepší, kdyby volala jen třídu B a ta delegovala na třídu C? Někdy ano, někdy ne – záleží na abstrakci a na tom, kdo by měl být zodpovědný za volání třídy C.
Přidání cizí funkce: Někdy byste potřebovali přidat k nějaké třídě novou metodu, ale nemůžete to z nějakého důvodu udělat. V tom případě musíte metodu přidat do jiné třídy, kde bude "cizí".
Přidání rozšiřující třídy: V předchozím případě (cizí funkce) se můžete rozhodnout vytvořit pro nově přidané metody novou třídu – rozšiřující třídu. Lze použít jak dědičnost (rozšiřující třída bude potomkem rozšiřované třídy), tak kompozici (rozšiřující třída bude obsahovat instanci rozšiřované třídy).
public class ThreadLocal {
private ThreadLocal () { /* non-instantiable */ }
// Sets current thread's value for named variable.
public static void set (String key, Object value);
// Returns current thread's value for named variable.
public static void get (String key);
}
public class ThreadLocal { private ThreadLocal () { /* non-instantiable */ } // Sets current thread's value for named variable. public static void set (String key, Object value); // Returns current thread's value for named variable. public static void get (String key); }
public class ThreadLocal {
private ThreadLocal () { /* non-instantiable */ }
public static class Key { Key() { } };
// Generates unique, unforgeable key
public static Key getKey () { return new Key (); }
public static void set (Key key, Object value);
public static void get (Key key);
}
public class ThreadLocal { private ThreadLocal () { /* non-instantiable */ } public static class Key { Key() { } }; // Generates unique, unforgeable key public static Key getKey () { return new Key (); } public static void set (Key key, Object value); public static void get (Key key); }
static ThreadLocal.Key serialNumberKey = ThreadLocal.getKey (); ThreadLocal.set (serialNumberKey, nextSerialNumber ()); System.out.println (ThreadLocal.get (serialNumberKey));
public class ThreadLocal { public ThreadLocal () { } public void set (Object value); public void get (); }
public class ThreadLocal { public ThreadLocal () { } public void set (Object value); public void get (); }
static ThreadLocal serialNumber = new ThreadLocal (); serialNumber.set (nextSerialNumber ()); System.out.println (serialNumber.get ());
Vytvoření jednoznačného zdroje pro data, která nemáte pod kontrolou: Některá data mohou být například skrytá v třídách GUI. Spodní vrstvy programu by neměly o existenci GUI nic vědět, je tedy lepší vytvořit nějakého prostředníka, který jim data bude zprostředkovávat.
grep
, sed
, Perl, Ruby, Python,...
Tvorba refaktorizačních nástrojů pro statické jazyky je jednodušší, jelikož je v compile-time známo mnohem víc informací o struktuře kódu, než u dynamických jazyků. V dynamických jazycích si místo refaktorizačních nástrojů typicky musíme vystačit s tradičními Unixovými nástroji a skriptováním.
Ale i pokud pracujeme se statickým jazykem, zdaleka ještě nemáme vyhráno – například u C++ a C# se plete do cesty podmíněná kompilace, která tvorbu nástrojů také komplikuje. Je to pravděpodobně jeden z důvodů, proč jsou dnes asi nejdál refaktorizační nástroje pro Javu, která podobné vlastnosti nemá.
class Movie { static final int REGULAR = 0; static final int NEW_RELEASE = 1; static final int CHILDRENS = 2; private String __title; private int __priceCode; Movie (final String title, final int priceCode) { __title = title; __priceCode = priceCode; } int getPriceCode () { return __priceCode; } void setPriceCode (final int priceCode) { __priceCode = priceCode; } String getTitle () { return __title; } }
class Rental { private Movie __movie; private int __daysRented; Rental (final Movie movie, final int daysRented) { __movie = movie; __daysRented = daysRented; } int getDaysRented () { return __daysRented; } Movie getMovie () { return __movie; } }
class Customer { private String __name; private List__rentals = new ArrayList (); Customer (final String name) { __name = name; } String getName () { return __name; } void addRental (final Rental rental) { __rentals.add (rental); } String statement () { double amountTotal = 0; int frequentRenterPoints = 0; String result = "Rental Record for "+ getName () + "\n"; for (final Rental rental : __rentals) { double rentalAmount = 0; // determine amount for each rental switch (rental.getMovie ().getPriceCode ()) { case Movie.REGULAR: rentalAmount += 2; if (rental.getDaysRented () > 2) { rentalAmount += (rental.getDaysRented () - 2) * 1.5; } break; case Movie.NEW_RELEASE: rentalAmount += rental.getDaysRented () * 3; break; case Movie.CHILDRENS: rentalAmount += 1.5; if (rental.getDaysRented () > 3) { rentalAmount += (rental.getDaysRented () - 3) * 1.5; } break; }
// add frequent renter points frequentRenterPoints++; // add bonus for a two day new release rental if ( (rental.getMovie ().getPriceCode () == Movie.NEW_RELEASE) && (rental.getDaysRented () > 1) ) { frequentRenterPoints++; } // show figures for this rental result += "\t"+ rental.getMovie ().getTitle () +"\t"+ rentalAmount +"\n"; amountTotal += rentalAmount; } // add footer lines result += "Amount owed is "+ amountTotal +"\n"; result += "You earned "+ frequentRenterPoints +" frequent renter points\n"; return result; } }
Customer.statement()
statement()
String statement () {
double amountTotal = 0;
int frequentRenterPoints = 0;
String result = "Rental Record for "+ getName () + "\n";
for (final Rental rental : __rentals) {
final double rentalAmount = __amountFor (rental);
// add frequent renter points
frequentRenterPoints++;
// add bonus for a two day new release rental
if (
(rental.getMovie ().getPriceCode () == Movie.NEW_RELEASE) &&
(rental.getDaysRented () > 1)
) {
frequentRenterPoints++;
}
// show figures for this rental
result += "\t"+ rental.getMovie ().getTitle () +"\t"+
rentalAmount +"\n";
amountTotal += rentalAmount;
}
// add footer lines
result += "Amount owed is "+ amountTotal +"\n";
result += "You earned "+ frequentRenterPoints +
" frequent renter points\n";
return result;
}
private double __amountFor (final Rental rental) {
double amount = 0;
// determine amount for each rental
switch (rental.getMovie ().getPriceCode ()) {
case Movie.REGULAR:
amount += 2;
if (rental.getDaysRented () > 2) {
amount += (rental.getDaysRented () - 2) * 1.5;
}
break;
case Movie.NEW_RELEASE:
amount += rental.getDaysRented () * 3;
break;
case Movie.CHILDRENS:
amount += 1.5;
if (rental.getDaysRented () > 3) {
amount += (rental.getDaysRented () - 3) * 1.5;
}
break;
}
return amount;
}
Customer.__amountFor ()
do Rental.getCharge ()
class Rental {
private Movie __movie;
private int __daysRented;
Rental (final Movie movie, final int daysRented) {
__movie = movie;
__daysRented = daysRented;
}
int getDaysRented () {
return __daysRented;
}
Movie getMovie () {
return __movie;
}
double getCharge () {
double result = 0;
// determine amount for each rental
switch (getMovie ().getPriceCode ()) {
case Movie.REGULAR:
result += 2;
if (getDaysRented () > 2) {
result += (getDaysRented () - 2) * 1.5;
}
break;
case Movie.NEW_RELEASE:
result += getDaysRented () * 3;
break;
case Movie.CHILDRENS:
result += 1.5;
if (getDaysRented () > 3) {
result += (getDaysRented () - 3) * 1.5;
}
break;
}
return result;
}
}
String statement () {
double amountTotal = 0;
int frequentRenterPoints = 0;
String result = "Rental Record for "+ getName () + "\n";
for (final Rental rental : __rentals) {
final double rentalAmount = rental.getCharge ();
// add frequent renter points
frequentRenterPoints++;
// add bonus for a two day new release rental
if (
(rental.getMovie ().getPriceCode () == Movie.NEW_RELEASE) &&
(rental.getDaysRented () > 1)
) {
frequentRenterPoints++;
}
// show figures for this rental
result += "\t"+ rental.getMovie ().getTitle () +"\t"+ rentalAmount +"\n";
amountTotal += rentalAmount;
}
// add footer lines
result += "Amount owed is "+ amountTotal +"\n";
result += "You earned "+ frequentRenterPoints +" frequent renter points\n";
return result;
}
statement ()
String statement () {
double amountTotal = 0;
int frequentRenterPoints = 0;
String result = "Rental Record for "+ getName () + "\n";
for (final Rental rental : __rentals) {
// add frequent renter points
frequentRenterPoints++;
// add bonus for a two day new release rental
if (
(rental.getMovie ().getPriceCode () == Movie.NEW_RELEASE) &&
(rental.getDaysRented () > 1)
) {
frequentRenterPoints++;
}
// show figures for this rental
result += "\t"+ rental.getMovie ().getTitle () +"\t"+ rental.getCharge () +"\n";
amountTotal += rental.getCharge ();
}
// add footer lines
result += "Amount owed is "+ amountTotal +"\n";
result += "You earned "+ frequentRenterPoints +" frequent renter points\n";
return result;
}
getFrequentRenterPoints ()
a přesun do třídy Rental
String statement () {
double amountTotal = 0;
int frequentRenterPoints = 0;
String result = "Rental Record for "+ getName () +"\n";
for (final Rental rental : __rentals) {
frequentRenterPoints += rental.getFrequentRenterPoints ();
// show figures for this rental
result += "\t"+ rental.getMovie ().getTitle () +"\t"+ rental.getCharge () +"\n";
amountTotal += rental.getCharge ();
}
// add footer lines
result += "Amount owed is "+ amountTotal +"\n";
result += "You earned "+ frequentRenterPoints + " frequent renter points\n";
return result;
}
int getFrequentRenterPoints () {
// add bonus for a two day new release rental
if (
(getMovie ().getPriceCode () == Movie.NEW_RELEASE) &&
(getDaysRented () > 1)
) {
return 2;
} else {
return 1;
}
}
statement ()
String statement () {
String result = "Rental Record for "+ getName () + "\n";
for (final Rental rental : __rentals) {
// show figures for this rental
result += "\t"+ rental.getMovie ().getTitle () +"\t"+ rental.getCharge () +"\n";
}
// add footer lines
result += "Amount owed is "+ __getTotalCharge () +"\n";
result += "You earned "+ __getTotalFrequentRenterPoints () + " frequent renter points\n";
return result;
}
private double __getTotalCharge () {
double result = 0;
for (final Rental rental : __rentals) {
result += rental.getCharge ();
}
return result;
}
private int __getTotalFrequentRenterPoints () {
int result = 0;
for (final Rental rental : __rentals) {
result += rental.getFrequentRenterPoints ();
}
return result;
}
Customer.statement ()
htmlStatement ()
String htmlstatement () {
String result = "<h1>Rental Record for <em>"+ getName () + "</em></h1><p>\n";
for (final Rental rental : __rentals) {
// show figures for this rental
result += rental.getMovie ().getTitle () +": "+ rental.getCharge () +"<br/>\n";
}
// add footer lines
result += "<p>you owe <em>"+ __getTotalCharge () +"</em><p>\n";
result += "On this rental you earned <em>"+
__getTotalFrequentRenterPoints ()+"</em> frequent renter points<p>\n";
return result;
}
Rental.getCharge ()
do třídy Movie
double getCharge () {
return __movie.getCharge (__daysRented);
}
double getCharge (final int daysRented) {
double result = 0;
// determine amount for each rental
switch (getPriceCode ()) {
case Movie.REGULAR:
result += 2;
if (daysRented > 2) {
result += (daysRented - 2) * 1.5;
}
break;
case Movie.NEW_RELEASE:
result += daysRented * 3;
break;
case Movie.CHILDRENS:
result += 1.5;
if (daysRented > 3) {
result += (daysRented - 3) * 1.5;
}
break;
}
return result;
}
Rental.getFrequentRenterPoints()
do třídy Movie
int getFrequentRenterPoints () {
return __movie.getFrequentRenterPoints (__daysRented);
}
int getFrequentRenterPoints (final int daysRented) {
// add bonus for a two day new release rental
if ((getPriceCode () == Movie.NEW_RELEASE) && (daysRented > 1)) {
return 2;
} else {
return 1;
}
}
abstract class Price {
abstract int getPriceCode ();
}
class RegularPrice extends Price {
@Override
int getPriceCode () {
return Movie.REGULAR;
}
}
class NewReleasePrice extends Price {
@Override
int getPriceCode () {
return Movie.NEW_RELEASE;
}
}
class ChildrensPrice extends Price {
@Override
int getPriceCode () {
return Movie.CHILDRENS;
}
}
abstract class Movie {
private Price __price;
Movie (final String title, final int priceCode) {
__title = title;
setPriceCode (priceCode);
}
void setPriceCode (final int priceCode) {
switch (priceCode) {
case REGULAR:
__price = new RegularPrice ();
break;
case NEW_RELEASE:
__price = new NewReleasePrice ();
break;
case CHILDRENS:
__price = new ChildrensPrice ();
break;
default:
throw new IllegalArgumentException ("invalid price code" + priceCode);
}
}
}
Movie.getCharge ()
do třídy Price
double getCharge (final int daysRented) {
return __price.getCharge (daysRented);
}
abstract class Price {
double getCharge (final int daysRented) {
double result = 0;
// determine amount for each rental
switch (getPriceCode ()) {
case Movie.REGULAR:
result += 2;
if (daysRented > 2) {
result += (daysRented - 2) * 1.5;
}
break;
case Movie.NEW_RELEASE:
result += daysRented * 3;
break;
case Movie.CHILDRENS:
result += 1.5;
if (daysRented > 3) {
result += (daysRented - 3) * 1.5;
}
break;
}
return result;
}
}
getCharge( )
polymorfizmemabstract class Price {
abstract int getPriceCode ();
abstract double getCharge (final int daysRented);
}
class ChildrensPrice extends Price {
@Override
int getPriceCode () {
return Movie.CHILDRENS;
}
@Override
double getCharge (final int daysRented) {
double result = 1.5;
if (daysRented > 3) {
result += (daysRented - 3) * 1.5;
}
return result;
}
}
class RegularPrice extends Price {
@Override
int getPriceCode () {
return Movie.REGULAR;
}
@Override
double getCharge (final int daysRented) {
double result = 2;
if (daysRented > 2) {
result += (daysRented - 2) * 1.5;
}
return result;
}
}
class NewReleasePrice extends Price {
@Override
int getPriceCode () {
return Movie.NEW_RELEASE;
}
@Override
double getCharge (final int daysRented) {
return daysRented * 3;
}
}
Movie.getFrequentRenterPoints()
int getFrequentRenterPoints (final int daysRented) {
return __price.getFrequentRenterPoints (daysRented);
}
abstract class Price {
abstract int getPriceCode ();
abstract double getCharge (final int daysRented);
int getFrequentRenterPoints (int daysRented) {
return 1;
}
}
class NewReleasePrice extends Price {
@Override
int getPriceCode () {
return Movie.NEW_RELEASE;
}
@Override
double getCharge (final int daysRented) {
return daysRented * 3;
}
@Override
int getFrequentRenterPoints (int daysRented) {
// add bonus for a two day new release rental
return (daysRented > 1) ? 2 : 1;
}
}
Customer.statement()
class TextStatement extends Statement {
@Override
String value (final Customer customer) {
String result = "Rental Record for "+ customer.getName () + "\n";
for (final Rental rental : customer.getRentals ()) {
// show figures for this rental
result += "\t"+ rental.getMovie ().getTitle () +"\t"+ rental.getCharge () +"\n";
}
// add footer lines
result += "Amount owed is "+ customer.getTotalCharge () +"\n";
result += "You earned "+ customer.getTotalFrequentRenterPoints ()+" frequent renter points\n";
return result;
}
}
class HtmlStatement extends Statement {
@Override
String value (final Customer customer) {
String result = "<h1>Rental Record for <em>"+ customer.getName () + "</em></h1><p>\n";
for (final Rental rental : customer.getRentals ()) {
// show figures for this rental
result += rental.getMovie ().getTitle () +": "+ rental.getCharge () +"<br/>\n";
}
// add footer lines
result += "<p>you owe <em>"+ customer.getTotalCharge () +"</em><p>\n";
result += "On this rental you earned <em>"+
customer.getTotalFrequentRenterPoints ()+"</em> frequent renter points<p>\n";
return result;
}
}
abstract class Statement {
abstract String value (final Customer customer);
}
class Customer {
String statement () {
return new TextStatement ().value (this);
}
String htmlStatement () {
return new HtmlStatement ().value (this);
}
}
abstract class Statement {
String value (final Customer customer) {
String result = headerString (customer);
for (final Rental rental : customer.getRentals ()) {
// show figures for this rental
result += eachRentalString (rental);
}
// add footer lines
result += footerString (customer);
return result;
}
protected abstract String headerString (Customer customer);
protected abstract String eachRentalString (Rental rental);
protected abstract String footerString (Customer customer);
}
class TextStatement extends Statement {
@Override
String headerString (final Customer customer) {
return "Rental Record for "+ customer.getName () + "\n";
}
@Override
String eachRentalString (final Rental rental) {
return "\t"+ rental.getMovie ().getTitle () +"\t"+ rental.getCharge () +"\n";
}
@Override
String footerString (final Customer customer) {
return
"Amount owed is "+ customer.getTotalCharge () +"\n" +
"You earned "+ customer.getTotalFrequentRenterPoints ()+" frequent renter points\n";
}
}
class HtmlStatement extends Statement {
@Override
String headerString (final Customer customer) {
return "<h1>Rental Record for <em>"+ customer.getName () + "</em></h1><p>\n";
}
@Override
String eachRentalString (final Rental rental) {
return rental.getMovie ().getTitle () +": "+ rental.getCharge () +"<br/>\n";
}
@Override
String footerString (final Customer customer) {
return
"<p>You owe <em>"+ customer.getTotalCharge () +"</em>\n" +
"<p>On this rental you earned <em>"+ customer.getTotalFrequentRenterPoints () +
"</em> frequent renter points<p>\n";
}
}