Nie tylko refaktoring, cz. II
W poprzednim numerze JAVA exPress zrobiliśmy pierwszy krok w kierunku uporządkowania pierwotnego kodu.
Zróbmy zatem kolejny. Przyjrzyjmy się bliżej następującemu fragmentowi:
public class ExtendedBitSet extends BitSet {
int length;
Pole klasy length ma widoczność pakietową. Prawdopodobnie nie to było zamierzeniem autora. Być może zapomniał w sposób jawny napisać odpowiedni modyfikator dostępu. W każdym razie warto znać jedną z podstawowych konsekwencji tego rozwiązania: pole to będzie dostępne dla wszystkich klas w pakiecie, co ogranicza jego enkapsulację informacji. Przypadek ten można uogólnić do stwierdzenia:
Nadawaj najbardziej restrykcyjny z możliwych modyfikatorów dostępu
W tym przypadku byłby to oczywiście modyfikator prywatny:
private int length;
W przypadku, gdy tworzona klasa będzie klasą bazową innych klas oraz będzie potrzeba udostępnienia tego pola, należy użyć modyfikatora dostępu protected:
protected int length;
Dobrą praktyką jest jednak wybieranie modyfikatora private, aż do momentu, gdy nie zaistnieje potrzeba użycia tego pola w klasie podrzędnej (czyli do momentu wyprowadzania nowych klas). Jest to analogia do typowej dla administratorów sieciowych strategii bezpieczeństwa: domyślnie blokuj.
Poza powyższymi uwagami, chciałbym zaproponować jeszcze jedno udoskonalenie. Jestem zwolennikiem jawnego inicjalizowania zmiennych, gdyż jest to bezpośrednie ujawnienie intencji autora.
Jeśli tworzę pole obiektowe, to jawnie przypisuję mu wartość początkową na null, kiedy tworzę liczbę całkowitą inicjuję ją zerem. W ten sposób oszczędzam potencjalnemu czytelnikowi mojego kodu domyślania się, czy rzeczywiście chodziło mi o wartość domyślną, czy być może nie do końca znając szczegóły języka, przyjąłem błędne założenie. W przypadku wyszukiwania przyczyn błędów może to mieć duże znaczenie.
Zatem podsumowując:
Jawnie inicjuj pola i zmienne
W efekcie uzyskamy takie rozwiązanie:
private int length = 0;
W powyżej przytoczonym wierszu ujawnił się jeszcze jeden nawyk związany ze sposobem programowania
Jak najwięcej przestrzeni dla Twoich oczu
Wzrok lubi przestrzeń. Nie lubi zbitych zdań, ogromnych ilości wyrażeń na niewielkiej przestrzeni. Zróbcie prosty eksperyment. Weźcie niewielki fragment swojego kodu i dodajcie kilka spacji (pomiędzy operatorami, pomiędzy wierszami) i porównajcie, który zapis jest czytelniejszy.
Oto przykład:
public byte[] toByteArray() {
int bytesNumber;
if (length % 8 == 0)
bytesNumber = length / 8;
else
bytesNumber = length / 8 + 1;
byte[] arr = new byte[bytesNumber];
for (int j = bytesNumber - 1, k = 0;
j >= 0; j--, k++) {
for (int i = j * 8; i < (j + 1) * 8; i++) {
if (i == length)
break;
if (get(i))
arr[k] += (byte) Math.pow(2, i % 8);
}
}
return arr;
}
oraz wersja przestrzenna:
public byte [] toByteArray() {
int bytesNumber = 0;
if ( length % 8 == 0 ) {
bytesNumber = length / 8;
} else {
bytesNumber = length / 8 + 1;
}
byte [] arr = new byte[ bytesNumber ];
for ( int j = bytesNumber - 1, k = 0;
j >= 0; j--, k++ ) {
for ( int i = j * 8; i < ( j + 1 ) * 8; i++ ) {
if ( i == length ) {
break;
}
if ( get( i ) ) {
arr [ k ] += (byte) Math.pow( 2, i % 8 );
}
}
}
return arr;
}
Modyfikacja ta zajęła mi około dwóch minut. Jednak umiejętność tworzenia kodu zgodnego ze stylem kodowania, która jest moim nawykiem, umożliwia mi pisanie takiego kodu bez żadnego dodatkowego nakładu czasu. Za to jaka przyjemność z czytania! A to dopiero początek. Kiedy mówimy o stylu kodowania przychodzi mi do głowy jeszcze jedna reguła:
Używaj konsekwentnie przyjętego standardu kodowania
Obecnie nie wyobrażam sobie tworzenia kodu, który nie podlega z góry ustalonym zasadom. Jeśli chodzi o pracę w zespole, jest to wręcz warunek konieczny pracy grupowej. A mamy ogromne wsparcie, gdyż istnieje wiele gotowych do wykorzystania standardów, np. Code Conventions for the Java Programming Language ( http://java.sun.com/docs/codeconv/), oraz narzędzi, które pomogą go, szczególnie w początkowym okresie, sumiennie przestrzegać (Checkstyle http://checkstyle.sourceforge.net/).
Poniżej znajduje się przykład omawianej klasy sformatowany wg standardu opartego na standardzie zaproponowanym przez Suna.
public class ExtendedBitSet extends BitSet {
private int length = 0;
public ExtendedBitSet(int size, String str) {
super(size);
length = size;
int strLength = str.length();
for (int i = 0; i < strLength; ++i) {
if (str.charAt(strLength - 1 - i) == '1') {
set(i);
}
}
}
public ExtendedBitSet(String str) {
super(str.length());
int strLength = str.length();
length = strLength;
for (int i = 0; i < strLength; ++i) {
if (str.charAt(strLength - 1 - i) == '1') {
set(i);
}
}
}
public void merge(ExtendedBitSet extendedBitSet) {
for (int i = extendedBitSet.nextSetBit(0);
i >= 0; i = extendedBitSet.nextSetBit(i + 1)) {
this.set(this.length + i);
}
this.length = this.length
+ extendedBitSet.length;
}
public int boolMultiply(
ExtendedBitSet extendedBitSet) {
int sum = 0;
int len = 0;
if (this.length < extendedBitSet.length) {
len = this.length;
} else {
len = extendedBitSet.length;
}
for (int i = 0; i < len; i++) {
if (this.get(i) && extendedBitSet.get(i)) {
sum++;
}
}
return sum % 2;
}
public byte[] toByteArray() {
int bytesNumber = 0;
if (length % 8 == 0) {
bytesNumber = length / 8;
} else {
bytesNumber = length / 8 + 1;
}
byte[] arr = new byte[bytesNumber];
for (int j = bytesNumber - 1, k = 0;
j >= 0; j--, k++) {
for (int i = j * 8; i < (j + 1) * 8; i++) {
if (i == length) {
break;
}
if (get(i)) {
arr[k] += (byte) Math.pow(2, i % 8);
}
}
}
return arr;
}
public String convertToBitString(int size) {
char[] resultArray = new char[size];
for (int i = 0; i < size; ++i) {
resultArray[i] = '0';
}
for (int i = this.nextSetBit(0); i >= 0;
i = this.nextSetBit(i + 1)) {
resultArray[size - 1 - i] = '1';
}
return new String(resultArray);
}
public String convertToBitString() {
return convertToBitString(this.length);
}
}
Wróćmy do naszego pola length. Tak naprawdę posiada ono jeszcze jeden mankament – jego nazwa jest dokładnie taka sama jak nazwa metody z klasy bazowej. Jest to sytuacja niekorzystna z dwóch powodów:
- dwa różne byty nie powinny mieć tej samej nazwy (metoda i pole), gdyż może to prowadzić do pomyłek,
- nazwa zmiennej nie odzwierciedla sensu właściwości. Pole to przechowuje wartość, która określa ustaloną długość wektora bitowego. Dużo większy sens miałaby na przykład nazwa fixedLength.
Zmieńmy zatem nazwe pola length na fixedLength.
Podsumowując powyższe rozważania:
Nie używaj jednej nazwy do różnych celów
oraz
Nadawaj polom, metodom i klasom nazwy, które jednoznacznie odzwierciedlają ich znaczenie
Analizując dalej przykład, spójrzmy na oba konstruktory, wyraźnie zauważymy pewną właściwość - jest tam mnóstwo powtarzającego się kodu. W ten sposób docieramy do zasady będącej esencją refaktoringu:
Eliminuj wszelkie powtórzenia
Powtórzenie to zło, które towarzyszy programistom na każdym kroku. Kuszące kopiuj-wklej, zazwyczaj ostatecznie prowadzi do kilkunastominutowych lub co gorsza wielogodzinnych poszukiwań błędów, wynikających z rozsynchronizowania się podobnych fragmentów kodu. Powtórzenia na dłuższą metę są nie do utrzymania, stąd ich eliminowanie jest podstawowym celem wszelkich refaktoringów. Przykładowy kod możemy zmienić do następującej postaci:
public ExtendedBitSet(int size, String str) {
super(size);
fixedLength = size;
initializeBitSet(str);
}
public ExtendedBitSet(String str) {
this(str.length(), str);
initializeBitSet(str);
}
private void initializeBitSet(String str) {
int strLength = str.length();
for (int i = 0; i < strLength; ++i) {
if (str.charAt(strLength - 1 - i) == '1') {
set(i);
}
}
}
Kod nam się powoli porządkuje i wygląda coraz lepiej. Wprowadziliśmy zmiany związane z wyglądem (standard kodowania i przestrzeń), wyeliminowaliśmy kilka powtórzeń i niejednoznaczności. Oczywiście zawsze należy wyważyć stopień refaktorowania lub upiększania kodu, tak aby nie stać się ofiarą perfekcjonizmu. Warto wesprzeć się pomocą innych programistów, najlepiej takich, którzy sami posługują się pewnymi zasadami oraz posiadają duże doświadczenie, i poprosić o opinię. Z pewnością wiele można się będzie dowiedzieć na temat swojego programowania.
Przyjrzyjmy się teraz metodzie boolMultipy oraz zmiennej lokalnej o nazwie sum. Zmienna ta jest deklarowana na samym początku metody, używana jest dużo później. Jest to nawyk, który pozostał jeszcze po językach proceduralnych (takich jak wczesne C przy PL/SQL), gdzie wymaga się zadeklarowania wszystkich zmiennych używanych w metodzie na samym początku. Na szczęście większość współczesnych języków programowania (w szczególności języków obiektów) nie ma tego ograniczenia. Zamiast tego podejścia sugeruję realizację zasady leniwej deklaracji zmiennych, którą można wyrazić za pomocą zdania.
Deklaruj zmienne najpóźniej jak to tylko możliwe
Warto to robić z bardzo prostego powodu – łatwiej będzie czytać kod, jeśli w zasięgu wzroku będziemy mieli operacje wykonywane na danej zmiennej. Przy bardziej złożonych metodach umieszczenie deklaracji na samym początku, może spowodować, że analizując dalszą część metody nie będziemy w stanie zorientować się czy zmienna była do tej pory przetwarzana, czy nie i co wpłynęło na jej wartość.
Załóżmy, że przewinęliśmy ekran tak, że widzimy następujący kod:
for (int i = 0; i < len; i++) {
ExtendedBitSet extendedBitSet;
if (this.get(i) && extendedBitSet.get(i)) {
sum++;
}
}
return sum % 2;
Rodzi się we mnie natychmiast pytanie – a co to za suma? Czy działo się z nią coś wcześniej? Czy może jej wartość została pobrana z zewnątrz?
W przypadku jednej zmiennej jeszcze to nie jest duży problem, ale wyobraźmy sobie sytuację, kiedy takich zmiennych jest pięć. Śledzenie logiki takiej metody będzie bardzo trudne.
Bardzo prosta operacja przeniesienia deklaracji nieco niżej spowoduje, że kod będzie dużo bardziej czytelny:
public int boolMultiply(
ExtendedBitSet extendedBitSet) {
int len = 0;
if (this.fixedLength < extendedBitSet.fixedLength)
{
len = this.fixedLength;
} else {
len = extendedBitSet.fixedLength;
}
int sum = 0;
for (int i = 0; i < len; i++) {
if (this.get(i) && extendedBitSet.get(i)) {
sum++;
}
}
return sum % 2;
}
Nieco bardziej złożona sytuacja jest w metodzie toByteArray. Jest ona niesamowicie trudna w analizie. Mi zajęło około 20 minut zrozumienie zasady jej działania. Teraz sobie wyobraźmy projekt złożony z tysiąca klas, z których każda zawiera tego typu metody. Zapanowanie nad takim kodem będzie graniczyło z cudem. Spójrzmy na kod tej metody:
public byte[] toByteArray() {
int bytesNumber = 0;
if (fixedLength % 8 == 0) {
bytesNumber = fixedLength / 8;
} else {
bytesNumber = fixedLength / 8 + 1;
}
byte[] arr = new byte[bytesNumber];
for (int j = bytesNumber - 1, k = 0; j >= 0;
j--, k++) {
for (int i = j * 8; i < (j + 1) * 8; i++) {
if (i == fixedLength) {
break;
}
if (get(i)) {
arr[k] += (byte) Math.pow(2, i % 8);
}
}
}
return arr;
}
Metoda toByteArray zwraca bajtową reprezentacje ciągów bitowych – czyli ciąg bitowy o długości 14 bitów można zaprezentować za pomocą 2 bajtów, zaś ciąg bitowy o długości 23 bity można zaprezentować za pomocą 3 bajtów.
Algorytm można opisać w następujących krokach:
- określ liczbę bajtów,
- dla każdej ósemki bitów (lub kilku bitów w przypadku niepełnych bajtów) wykonaj następującą operację:
- sprawdź wartość każdego bitu
- jeśli bit ma wartość 1, dodaj odpowiednią potęgę dwójki (wynikającą z pozycji bitu w bajcie) do wyniku danego bajtu.
Dlaczegóżby nie wyrazić tego algorytmu w formie programistycznej? Często o tym się zapomina. Jako programiści próbujemy w zwięzły sposób wyrazić nasze pomysły, co zazwyczaj prowadzi do bardzo nieczytelnych rozwiązań, których nie tylko inni ale i my sami nie jesteśmy w stanie zrozumieć w krótkim czasie. Ideałem jest dążenie do tego, aby jedno spojrzenie wystarczyło do odszyfrowania intencji twórcy. Nie potrzebujemy zagłębiać się w szczegóły realizacji algorytmu, ważne byśmy wychwycili jego główną myśl. Spójrzmy na inną implementację metody toByteArray:
public byte[] toByteArray() {
int bytesCount = computeBytesCount();
byte[] byteArray = new byte[bytesCount];
for (int i = 0; i < bytesCount; ++i) {
int byteNumber = bytesCount - i - 1;
byteArray[byteNumber] = computeByteValue(i);
}
return byteArray;
}
Najważniejsza zmiana, polega na bezpośrednim wyrażeniu algorytmu na ogólnym poziomie. Dzięki czemu jesteśmy w stanie w krótkim czasie zrozumieć intencję twórcy. Pomocnicza metoda computeBytesCount znajduje ilość bajtów nowej reprezentacji. W pętli dla każdego bajtu wykonywana jest operacja obliczania wartości bajtu i zapamiętywania wyniku. Czyż taki zapis nie jest dużo prostszy? Co z tego, że nie widać wszystkich elementów realizacji algorytmu. Bardziej dociekliwi zawsze mogą zajrzeć do metod computeBytesCount i computeByteValue.
Mogą one wyglądać następująco:
private byte computeByteValue(int byteNumber) {
int firstBitPosition = byteNumber * 8;
int lastBitPosition = (byteNumber + 1) * 8 - 1;
byte byteValue = 0;
for (int i = this.nextSetBit(firstBitPosition);
i >= firstBitPosition
&& i <= lastBitPosition; i = this
.nextSetBit(i + 1)) {
int currentBitPosition = i - firstBitPosition;
if (get(i) == true) {
byteValue += (byte) Math.pow(2,
currentBitPosition % 8);
}
}
return byteValue;
}
private int computeBytesCount() {
int bytesCount = 0;
if (fixedLength % 8 == 0) {
bytesCount = fixedLength / 8;
} else {
bytesCount = fixedLength / 8 + 1;
}
return bytesCount;
}
Warto zauważyć, że kod realizujący zadanie zbytnio się nie uprościł, ale dużo łatwiej jest go teraz przeanalizować i zrozumieć. Teoria refaktoringu nazywa tego typu działania wyłuskiwaniem metody (ang. extract method). Prawdę mówiąc tutaj poszliśmy nieco dalej, gdyż zmodyfikowaliśmy nieco implementację algorytmu, tak aby stała się bardziej czytelna. Zwróćmy uwagę na wiersze:
int firstBitPosition = byteNumber * 8;
int lastBitPosition = (byteNumber + 1) * 8 - 1;
Tak naprawdę są one wyodrębnieniem bardzo enigmatycznych wyrażeń z wiersza:
for (int i = j * 8; i < (j + 1) * 8; i++) {
Czyż intencja w takim przypadku nie staje się oczywista? Moje wieloletnie doświadczenia doprowadziły mnie do wniosku:
Jawnie nazywaj złożone elementy kodu
Zamiast
j * 8
napisz
int firstBitPosition = byteNumber * 8;
Zasadę te rozszerzam do instrukcji warunkowych. Często w kodzie możemy spotkać wyrażenia, za którymi kryje się pewna logika. Warto bezpośrednio wyrazić naszą intencję. Czytelność niesamowicie wzrasta.
Zamiast
if (index >= 0)
napisz
boolean isIndexInRange = (index >= 0);
if (isIndexInRange) {
Kod zaczyna się czytać jak książkę! Przecież programowanie jest dla ludzi. Ułatwiajmy zatem sobie życie.
A oto najważniejsza zasada, będąca kwintesencją powyższych rozważań:
Pisz kod w taki sposób, aby czytało się go jak powieść. Używaj jednoznacznych i jednocześnie prostych nazw. Realizowane operacje dziel na logiczne części i każdą implementuj w osobnych metodach.
Myślę, że jak na jeden raz, wystarczy. Wystarczy, żeby zaostrzyć apetyty i wzbudzić ochotę na więcej. Żeby oprowadzić nieco po ogrodzie refaktoringu i jego przyległościach. Zapewniam, że to niesamowite miejsce i daje niesamowicie wiele radości i satysfakcji.
Poniżej zamieszczam ostateczną wersję kodu, który przechodzi załączone testy (oczywiście ze zmianą uwzględniającą niestatyczność metod merge i boolMultiply).
Końcowa postać przykładowej klasy (warto ją porównać z postacią początkową):
public class ExtendedBitSet extends BitSet {
private int fixedLength = 0;
public ExtendedBitSet(int size, String str) {
super(size);
fixedLength = size;
initializeBitSet(str);
}
public ExtendedBitSet(String str) {
this(str.length(), str);
initializeBitSet(str);
}
private void initializeBitSet(String str) {
int strLength = str.length();
for (int i = 0; i < strLength; ++i) {
if (str.charAt(strLength - 1 - i) == '1') {
set(i);
}
}
}
public void merge(ExtendedBitSet extendedBitSet) {
for (int i = extendedBitSet.nextSetBit(0);
i >= 0; i = extendedBitSet
.nextSetBit(i + 1)) {
this.set(this.fixedLength + i);
}
this.fixedLength = this.fixedLength
+ extendedBitSet.fixedLength;
}
public int boolMultiply(
ExtendedBitSet extendedBitSet) {
int len = 0;
if (this.fixedLength <
extendedBitSet.fixedLength) {
len = this.fixedLength;
} else {
len = extendedBitSet.fixedLength;
}
int sum = 0;
for (int i = 0; i < len; i++) {
if (this.get(i) && extendedBitSet.get(i)) {
sum++;
}
}
return sum % 2;
}
public byte[] toByteArray() {
int bytesCount = computeBytesCount();
byte[] byteArray = new byte[bytesCount];
for (int i = 0; i < bytesCount; ++i) {
int byteNumber = bytesCount - i - 1;
byteArray[byteNumber] = computeByteValue(i);
}
return byteArray;
}
private byte computeByteValue(int byteNumber) {
int firstBitPosition = byteNumber * 8;
int lastBitPosition = (byteNumber + 1) * 8 - 1;
byte byteValue = 0;
for (int i = this.nextSetBit(firstBitPosition);
i >= firstBitPosition
&& i <= lastBitPosition; i = this
.nextSetBit(i + 1)) {
int currentBitPosition = i - firstBitPosition;
if (get(i) == true) {
byteValue += (byte) Math.pow(2,
currentBitPosition % 8);
}
}
return byteValue;
}
private int computeBytesCount() {
int bytesCount = 0;
if (fixedLength % 8 == 0) {
bytesCount = fixedLength / 8;
} else {
bytesCount = fixedLength / 8 + 1;
}
return bytesCount;
}
public String convertToBitString(int size) {
char[] resultArray = new char[size];
for (int i = 0; i < size; ++i) {
resultArray[i] = '0';
}
for (int i = this.nextSetBit(0); i >= 0;
i = this.nextSetBit(i + 1)) {
resultArray[size - 1 - i] = '1';
}
return new String(resultArray);
}
public String convertToBitString() {
return convertToBitString(this.fixedLength);
}
}
Dodatek. Test przykładowej klasy.
import junit.framework.TestCase;
public class ExtendedBitSetTest extends TestCase {
public void testConstructorSizeAndString()
throws Exception {
assertEquals("{0, 2}", new ExtendedBitSet(10,
"101").toString());
assertEquals("0000000101", new ExtendedBitSet(
10, "101").convertToBitString());
assertEquals("0000001011", new ExtendedBitSet(
10, "1011").convertToBitString());
assertEquals("0010001011", new ExtendedBitSet(
10, "10001011").convertToBitString());
assertEquals("{0, 1, 3, 7}",
new ExtendedBitSet(10, "10001011")
.toString());
assertEquals("{0}", new ExtendedBitSet(10,
"001").toString());
assertEquals("0000000001", new ExtendedBitSet(
10, "001").convertToBitString());
assertEquals("00000000001", new ExtendedBitSet(
10, "001").convertToBitString(11));
}
public void testMerge() throws Exception {
ExtendedBitSet extendedBitSet1 =
new ExtendedBitSet(10, "1");
ExtendedBitSet extendedBitSet2 =
new ExtendedBitSet(10, "1");
assertEquals("0000000001", extendedBitSet1
.convertToBitString());
assertEquals("0000000001", extendedBitSet2
.convertToBitString());
ExtendedBitSet mergedBitSet = ExtendedBitSet
.merge(extendedBitSet1, extendedBitSet2);
String mergedString = mergedBitSet
.convertToBitString();
assertEquals("00000000010000000001",
mergedString);
assertEquals("{0, 10}",
mergedBitSet.toString());
assertTrue(mergedBitSet.get(0) == true);
}
public void testBoolMultiple() throws Exception {
ExtendedBitSet extendedBitSet1 =
new ExtendedBitSet(3, "1");
ExtendedBitSet extendedBitSet2 =
new ExtendedBitSet(10, "1");
assertEquals(1, ExtendedBitSet.boolMultiply(
extendedBitSet1, extendedBitSet2));
extendedBitSet1 = new ExtendedBitSet(1000, "1");
extendedBitSet2 = new ExtendedBitSet(2, "1");
assertEquals(1, ExtendedBitSet.boolMultiply(
extendedBitSet1, extendedBitSet2));
extendedBitSet1 = new ExtendedBitSet(10, "1");
extendedBitSet2 = new ExtendedBitSet(10, "1");
assertEquals(1, ExtendedBitSet.boolMultiply(
extendedBitSet1, extendedBitSet2));
extendedBitSet1 = new ExtendedBitSet(10, "1");
extendedBitSet2 = new ExtendedBitSet(10, "10");
assertEquals(0, ExtendedBitSet.boolMultiply(
extendedBitSet1, extendedBitSet2));
extendedBitSet1 = new ExtendedBitSet(10, "10");
extendedBitSet2 = new ExtendedBitSet(10, "10");
assertEquals(1, ExtendedBitSet.boolMultiply(
extendedBitSet1, extendedBitSet2));
extendedBitSet1 = new ExtendedBitSet(10, "110");
extendedBitSet2 = new ExtendedBitSet(10, "110");
assertEquals(0, ExtendedBitSet.boolMultiply(
extendedBitSet1, extendedBitSet2));
}
public void testToByteArray() throws Exception {
ExtendedBitSet extendedBitSet =
new ExtendedBitSet("100000110");
byte[] toByteArray = extendedBitSet
.toByteArray();
assertEquals(1, toByteArray[0]);
assertEquals(6, toByteArray[1]);
extendedBitSet = new ExtendedBitSet(
"10111111111");
toByteArray = extendedBitSet.toByteArray();
assertEquals(5, toByteArray[0]);
assertEquals(-1, toByteArray[1]);
}
}
Nobody has commented it yet.