eeymoo/Clean-Code-zh
1
0
Fork 0
mirror of https://github.com/glen9527/Clean-Code-zh.git synced 2025-12-17 18:54:23 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

标题 & 代码高亮

Browse Source
This commit is contained in:
gdut-yy
2019-12-28 22:27:33 +08:00
parent 67f3f67fac
commit 893fb65d8c
17 changed files with 684 additions and 682 deletions
Download Patch File Download Diff File Expand all files Collapse all files

112
docs/ch16.md
View File

@@ -1,4 +1,4 @@
Refactoring SerialDate
# 第 16 章 Refactoring SerialDate
Image
If you go to http://www.jfree.org/jcommon/index.php, you will find the JCommon library. Deep within that library there is a package named org.jfree.date. Within that package there is a class named SerialDate. We are going to explore that class.
@@ -29,7 +29,7 @@ The first few commented-out tests (lines 23-63) were a bit of conceit on my part
I left the tests at line 32 and line 45 commented out because it’s not clear to me that the “tues” and “thurs” abbreviations ought to be supported.
The tests on line 153 and line 154 don’t pass. Clearly, they should [G2]. We can easily fix this, and the tests on line 163 through line 213, by making the following changes to the stringToMonthCode function.
```java
457 if ((result < 1) || (result > 12)) {
result = -1;
458 for (int i = 0; i < monthNames.length; i++) {
@@ -43,11 +43,11 @@ The tests on line 153 and line 154 don’t pass. Clearly, they should [G2]. We c
466 }
467 }
468 }
```
The commented test on line 318 exposes a bug in the getFollowingDayOfWeek method (line 672). December 25th, 2004, was a Saturday. The following Saturday was January 1st, 2005. However, when we run the test, we see that getFollowingDayOfWeek returns December 25th as the Saturday that follows December 25th. Clearly, this is wrong [G3],[T1]. We see the problem in line 685. It is a typical boundary condition error [T5]. It should read as follows:
```java
685 if (baseDOW >= targetWeekday) {
```
It is interesting to note that this function was the target of an earlier repair. The change history (line 43) shows that “bugs” were fixed in getPreviousDayOfWeek, getFollowingDayOfWeek, and getNearestDayOfWeek [T6].
The testGetNearestDayOfWeek unit test (line 329), which tests the getNearestDayOfWeek method (line 705), did not start out as long and exhaustive as it currently is. I added a lot of test cases to it because my initial test cases did not all pass [T6]. You can see the pattern of failure by looking at which test cases are commented out. That pattern is revealing [T7]. It shows that the algorithm fails if the nearest day is in the future. Clearly there is some kind of boundary condition error [T5].
@@ -55,7 +55,7 @@ The testGetNearestDayOfWeek unit test (line 329), which tests the getNearestDayO
The pattern of test coverage reported by Clover is also interesting [T8]. Line 719 never gets executed! This means that the if statement in line 718 is always false. Sure enough, a look at the code shows that this must be true. The adjust variable is always negative and so cannot be greater or equal to 4. So this algorithm is just wrong.
The right algorithm is shown below:
```java
int delta = targetDOW - base.getDayOfWeek();
int positiveDelta = delta + 7;
int adjust = positiveDelta % 7;
@@ -63,7 +63,7 @@ The right algorithm is shown below:
adjust -= 7;
return SerialDate.addDays(adjust, base);
```
Finally, the tests at line 417 and line 429 can be made to pass simply by throwing an IllegalArgumentException instead of returning an error string from weekInMonthToString and relativeToString.
With these changes all the unit tests pass, and I believe SerialDate now works. So now it’s time to make it “right.”
@@ -75,7 +75,7 @@ Starting at line 1, we see a ream of comments with license information, copyrigh
The import list starting at line 61 could be shortened by using java.text.* and java.util.*. [J1]
I wince at the HTML formatting in the Javadoc (line 67). Having a source file with more than one language in it troubles me. This comment has four languages in it: Java, English, Javadoc, and html [G1]. With that many languages in use, it’s hard to keep things straight. For example, the nice positioning of line 71 and line 72 are lost when the Javadoc is generated, and yet who wants to see <ul> and <li> in the source code? A better strategy might be to just surround the whole comment with <pre> so that the formatting that is apparent in the source code is preserved within the Javadoc.1
I wince at the HTML formatting in the Javadoc (line 67). Having a source file with more than one language in it troubles me. This comment has four languages in it: Java, English, Javadoc, and html [G1]. With that many languages in use, it’s hard to keep things straight. For example, the nice positioning of line 71 and line 72 are lost when the Javadoc is generated, and yet who wants to see `<ul>` and `<li>` in the source code? A better strategy might be to just surround the whole comment with `<pre>` so that the formatting that is apparent in the source code is preserved within the Javadoc.1
1. An even better solution would have been for Javadoc to present all comments as preformatted, so that comments appear the same in both code and document.
@@ -92,7 +92,7 @@ Unfortunately, there are already too many classes in the Java library named Date
From now on in this discussion I will use the term DayDate. I leave it to you to remember that the listings you are looking at still use SerialDate.
I understand why DayDate inherits from Comparable and Serializable. But why does it inherit from MonthConstants? The class MonthConstants (Listing B-3, page 372) is just a bunch of static final constants that define the months. Inheriting from classes with constants is an old trick that Java programmers used so that they could avoid using expressions like MonthConstants.January, but it’s a bad idea [J2]. MonthConstants should really be an enum.
```java
public abstract class DayDate implements Comparable,
Serializable {
public static enum Month {
@@ -122,7 +122,7 @@ I understand why DayDate inherits from Comparable and Serializable. But why does
}
public final int index;
}
```
Changing MonthConstants to this enum forces quite a few changes to the DayDate class and all it’s users. It took me an hour to make all the changes. However, any function that used to take an int for a month, now takes a Month enumerator. This means we can get rid of the isValidMonthCode method (line 326), and all the month code error checking such as that in monthCodeToQuarter (line 356) [G5].
Next, we have line 91, serialVersionUID. This variable is used to control the serializer. If we change it, then any DayDate written with an older version of the software won’t be readable anymore and will result in an InvalidClassException. If you don’t declare the serialVersionUID variable, then the compiler automatically generates one for you, and it will be different every time you make a change to the module. I know that all the documents recommend manual control of this variable, but it seems to me that automatic control of serialization is a lot safer [G4]. After all, I’d much rather debug an InvalidClassException than the odd behavior that would ensue if I forgot to change the serialVersionUID. So I’m going to delete the variable—at least for the time being.2
@@ -132,10 +132,10 @@ Next, we have line 91, serialVersionUID. This variable is used to control the se
I find the comment on line 93 redundant. Redundant comments are just places to collect lies and misinformation [C2]. So I’m going to get rid of it and its ilk.
The comments at line 97 and line 100 talk about serial numbers, which I discussed earlier [C1]. The variables they describe are the earliest and latest possible dates that DayDate can describe. This can be made a bit clearer [N1].
```java
public static final int EARLIEST_DATE_ORDINAL = 2; // 1/1/1900
public static final int LATEST_DATE_ORDINAL = 2958465; // 12/31/9999
```
It’s not clear to me why EARLIEST_DATE_ORDINAL is 2 instead of 0. There is a hint in the comment on line 829 that suggests that this has something to do with the way dates are represented in Microsoft Excel. There is a much deeper insight provided in a derivative of DayDate called SpreadsheetDate (Listing B-5, page 382). The comment on line 71 describes the issue nicely.
The problem I have with this is that the issue seems to be related to the implementation of SpreadsheetDate and has nothing to do with DayDate. I conclude from this that EARLIEST_DATE_ORDINAL and LATEST_DATE_ORDINAL do not really belong in DayDate and should be moved to SpreadsheetDate [G6].
@@ -149,7 +149,7 @@ What we need to do is provide this information without polluting DayDate itself.
It’s generally a bad idea for base classes to know about their derivatives. To fix this, we should use the ABSTRACT FACTORY3 pattern and create a DayDateFactory. This factory will create the instances of DayDate that we need and can also answer questions about the implementation, such as the maximum and minimum dates.
3. [GOF].
```java
public abstract class DayDateFactory {
private static DayDateFactory factory = new SpreadsheetDateFactory();
public static void setInstance(DayDateFactory factory) {
@@ -186,7 +186,7 @@ It’s generally a bad idea for base classes to know about their derivatives. To
return factory._getMaximumYear();
}
}
```
This factory class replaces the createInstance methods with makeDate methods, which improves the names quite a bit [N1]. It defaults to a SpreadsheetDateFactory but can be changed at any time to use a different factory. The static methods that delegate to abstract methods use a combination of the SINGLETON,4 DECORATOR,5 and ABSTRACT FACTORY patterns that I have found to be useful.
4. Ibid.
@@ -194,7 +194,7 @@ This factory class replaces the createInstance methods with makeDate methods, wh
5. Ibid.
The SpreadsheetDateFactory looks like this.
```java
public class SpreadsheetDateFactory extends DayDateFactory {
public DayDate _makeDate(int ordinal) {
return new SpreadsheetDate(ordinal);
@@ -225,7 +225,7 @@ The SpreadsheetDateFactory looks like this.
return SpreadsheetDate.MAXIMUM_YEAR_SUPPORTED;
}
}
```
As you can see, I have already moved the MINIMUM_YEAR_SUPPORTED and MAXIMUM_YEAR_SUPPORTED variables into SpreadsheetDate, where they belong [G6].
The next issue in DayDate are the day constants beginning at line 109. These should really be another enum [J3]. We’ve seen this pattern before, so I won’t repeat it here. You’ll see it in the final listings.
@@ -245,7 +245,7 @@ What settles the argument for me is that to be consistent [G11], we should make
The same goes for the table, LEAP_YEAR_AGGREGATE_DAYS_TO_END_OF_MONTH.
Next, we see three sets of constants that can be turned into enums (lines 162–205). The first of the three selects a week within a month. I changed it into an enum named WeekInMonth.
```java
public enum WeekInMonth {
FIRST(1), SECOND(2), THIRD(3), FOURTH(4), LAST(0);
public final int index;
@@ -254,7 +254,7 @@ Next, we see three sets of constants that can be turned into enums (lines 162–
this.index = index;
}
}
```
The second set of constants (lines 177–187) is a bit more obscure. The INCLUDE_NONE, INCLUDE_FIRST, INCLUDE_SECOND, and INCLUDE_BOTH constants are used to describe whether the defining end-point dates of a range should be included in that range. Mathematically, this is described using the terms “open interval,” “half-open interval,” and “closed interval.” I think it is clearer using the mathematical nomenclature [N3], so I changed it to an enum named DateInterval with CLOSED, CLOSED_LEFT, CLOSED_RIGHT, and OPEN enumerators.
The third set of constants (lines 189–205) describe whether a search for a particular day of the week should result in the last, next, or nearest instance. Deciding what to call this is difficult at best. In the end, I settled for WeekdayRange with LAST, NEXT, and NEAREST enumerators.
@@ -278,7 +278,7 @@ I didn’t care for the duplicate if statements [G5] inside the for loop (line 2
It occurred to me that this method does not really belong in DayDate. It’s really the parse function of Day. So I moved it into the Day enumeration. However, that made the Day enumeration pretty large. Because the concept of Day does not depend on DayDate, I moved the Day enumeration outside of the DayDate class into its own source file [G13].
I also moved the next function, weekdayCodeToString (lines 272–286) into the Day enumeration and called it toString.
```java
public enum Day {
MONDAY(Calendar.MONDAY),
TUESDAY(Calendar.TUESDAY),
@@ -324,27 +324,27 @@ I also moved the next function, weekdayCodeToString (lines 272–286) into the D
return dateSymbols.getWeekdays()[index];
}
}
```
There are two getMonths functions (lines 288–316). The first calls the second. The second is never called by anyone but the first. Therefore, I collapsed the two into one and vastly simplified them [G9],[G12],[F4]. Finally, I changed the name to be a bit more self-descriptive [N1].
```java
public static String[] getMonthNames() {
return dateFormatSymbols.getMonths();
}
```
The isValidMonthCode function (lines 326–346) was made irrelevant by the Month enum, so I deleted it [G9].
The monthCodeToQuarter function (lines 356–375) smells of FEATURE ENVY7 [G14] and probably belongs in the Month enum as a method named quarter. So I replaced it.
7. [Refactoring].
```java
public int quarter() {
return 1 + (index-1)/3;
}
```
This made the Month enum big enough to be in its own class. So I moved it out of DayDate to be consistent with the Day enum [G11],[G13].
The next two methods are named monthCodeToString (lines 377–426). Again, we see the pattern of one method calling its twin with a flag. It is usually a bad idea to pass a flag as an argument to a function, especially when that flag simply selects the format of the output [G15]. I renamed, simplified, and restructured these functions and moved them into the Month enum [N1],[N3],[C3],[G14].
```java
public String toString() {
return dateFormatSymbols.getMonths()[index - 1];
}
@@ -352,9 +352,9 @@ The next two methods are named monthCodeToString (lines 377–426). Again, we se
public String toShortString() {
return dateFormatSymbols.getShortMonths()[index - 1];
}
```
The next method is stringToMonthCode (lines 428–472). I renamed it, moved it into the Month enum, and simplified it [N1],[N3],[C3],[G14],[G12].
```java
public static Month parse(String s) {
s = s.trim();
for (Month m : Month.values())
@@ -372,37 +372,37 @@ The next method is stringToMonthCode (lines 428–472). I renamed it, moved it i
return s.equalsIgnoreCase(toString()) ||
s.equalsIgnoreCase(toShortString());
}
```
The isLeapYear method (lines 495–517) can be made a bit more expressive [G16].
```java
public static boolean isLeapYear(int year) {
boolean fourth = year % 4 == 0;
boolean hundredth = year % 100 == 0;
boolean fourHundredth = year % 400 == 0;
return fourth && (!hundredth || fourHundredth);
}
```
The next function, leapYearCount (lines 519–536) doesn’t really belong in DayDate. Nobody calls it except for two methods in SpreadsheetDate. So I pushed it down [G6].
The lastDayOfMonth function (lines 538–560) makes use of the LAST_DAY_OF_MONTH array. This array really belongs in the Month enum [G17], so I moved it there. I also simplified the function and made it a bit more expressive [G16].
```java
public static int lastDayOfMonth(Month month, int year) {
if (month == Month.FEBRUARY && isLeapYear(year))
return month.lastDay() + 1;
else
return month.lastDay();
}
```
Now things start to get a bit more interesting. The next function is addDays (lines 562–576). First of all, because this function operates on the variables of DayDate, it should not be static [G18]. So I changed it to an instance method. Second, it calls the function toSerial. This function should be renamed toOrdinal [N1]. Finally, the method can be simplified.
```java
public DayDate addDays(int days) {
return DayDateFactory.makeDate(toOrdinal() + days);
}
```
The same goes for addMonths (lines 578–602). It should be an instance method [G18]. The algorithm is a bit complicated, so I used EXPLAINING TEMPORARY VARIABLES8 [G19] to make it more transparent. I also renamed the method getYYY to getYear [N1].
8. [Beck97].
```java
public DayDate addMonths(int months) {
int thisMonthAsOrdinal = 12 * getYear() + getMonth().index - 1;
int resultMonthAsOrdinal = thisMonthAsOrdinal + months;
@@ -413,40 +413,40 @@ The same goes for addMonths (lines 578–602). It should be an instance method [
int resultDay = Math.min(getDayOfMonth(), lastDayOfResultMonth);
return DayDateFactory.makeDate(resultDay, resultMonth, resultYear);
}
```
The addYears function (lines 604–626) provides no surprises over the others.
```java
public DayDate plusYears(int years) {
int resultYear = getYear() + years;
int lastDayOfMonthInResultYear = lastDayOfMonth(getMonth(), resultYear);
int resultDay = Math.min(getDayOfMonth(), lastDayOfMonthInResultYear);
return DayDateFactory.makeDate(resultDay, getMonth(), resultYear);
}
```
There is a little itch at the back of my mind that is bothering me about changing these methods from static to instance. Does the expression date.addDays(5) make it clear that the date object does not change and that a new instance of DayDate is returned? Or does it erroneously imply that we are adding five days to the date object? You might not think that is a big problem, but a bit of code that looks like the following can be very deceiving [G20].
```java
DayDate date = DateFactory.makeDate(5, Month.DECEMBER, 1952);
date.addDays(7); // bump date by one week.
```
Someone reading this code would very likely just accept that addDays is changing the date object. So we need a name that breaks this ambiguity [N4]. So I changed the names to plusDays and plusMonths. It seems to me that the intent of the method is captured nicely by
```java
DayDate date = oldDate.plusDays(5);
```
whereas the following doesn’t read fluidly enough for a reader to simply accept that the date object is changed:
```java
date.plusDays(5);
```
The algorithms continue to get more interesting. getPreviousDayOfWeek (lines 628–660) works but is a bit complicated. After some thought about what was really going on [G21], I was able to simplify it and use EXPLAINING TEMPORARY VARIABLES [G19] to make it clearer. I also changed it from a static method to an instance method [G18], and got rid of the duplicate instance method [G5] (lines 997–1008).
```java
public DayDate getPreviousDayOfWeek(Day targetDayOfWeek) {
int offsetToTarget = targetDayOfWeek.index - getDayOfWeek().index;
if (offsetToTarget >= 0)
offsetToTarget -= 7;
return plusDays(offsetToTarget);
}
```
The exact same analysis and result occurred for getFollowingDayOfWeek (lines 662–693).
```java
public DayDate getFollowingDayOfWeek(Day targetDayOfWeek) {
int offsetToTarget = targetDayOfWeek.index - getDayOfWeek().index;
if (offsetToTarget <= 0)
@@ -454,9 +454,9 @@ The exact same analysis and result occurred for getFollowingDayOfWeek (lines 662
offsetToTarget += 7;
return plusDays(offsetToTarget);
}
```
The next function is getNearestDayOfWeek (lines 695–726), which we corrected back on page 270. But the changes I made back then aren’t consistent with the current pattern in the last two functions [G11]. So I made it consistent and used some EXPLAINING TEMPORARY VARIABLES [G19] to clarify the algorithm.
```java
public DayDate getNearestDayOfWeek(final Day targetDay) {
int offsetToThisWeeksTarget = targetDay.index - getDayOfWeek().index;
int offsetToFutureTarget = (offsetToThisWeeksTarget + 7) % 7;
@@ -467,16 +467,16 @@ The next function is getNearestDayOfWeek (lines 695–726), which we corrected b
else
return plusDays(offsetToFutureTarget);
}
```
The getEndOfCurrentMonth method (lines 728–740) is a little strange because it is an instance method that envies [G14] its own class by taking a DayDate argument. I made it a true instance method and clarified a few names.
```java
public DayDate getEndOfMonth() {
Month month = getMonth();
int year = getYear();
int lastDay = lastDayOfMonth(month, year);
return DayDateFactory.makeDate(lastDay, month, year);
}
```
Refactoring weekInMonthToString (lines 742–761) turned out to be very interesting indeed. Using the refactoring tools of my IDE, I first moved the method to the WeekInMonth enum that I created back on page 275. Then I renamed the method to toString. Next, I changed it from a static method to an instance method. All the tests still passed. (Can you guess where I am going?)
Next, I deleted the method entirely! Five asserts failed (lines 411–415, Listing B-4, page 374). I changed these lines to use the names of the enumerators (FIRST, SECOND, …). All the tests passed. Can you see why? Can you also see why each of these steps was necessary? The refactoring tool made sure that all previous callers of weekInMonthToString now called toString on the weekInMonth enumerator because all enumerators implement toString to simply return their names.…
@@ -496,13 +496,13 @@ If you look carefully (line 247, Listing B-5, page 382), you’ll see that the a
Logical dependencies like this bother me [G22]. If something logical depends on the implementation, then something physical should too. Also, it seems to me that the algorithm itself could be generic with a much smaller portion of it dependent on the implementation [G6].
So I created an abstract method in DayDate named getDayOfWeekForOrdinalZero and implemented it in SpreadsheetDate to return Day.SATURDAY. Then I moved the getDayOfWeek method up to DayDate and changed it to call getOrdinalDay and getDayOfWeekForOrdinal-Zero.
```java
public Day getDayOfWeek() {
Day startingDay = getDayOfWeekForOrdinalZero();
int startingOffset = startingDay.index - Day.SUNDAY.index;
return Day.make((getOrdinalDay() + startingOffset) % 7 + 1);
}
```
As a side note, look carefully at the comment on line 895 through line 899. Was this repetition really necessary? As usual, I deleted this comment along with all the others.
The next method is compare (lines 902–913). Again, this method is inappropriately abstract [G6], so I pulled the implementation up into DayDate. Also, the name does not communicate enough [N1]. This method actually returns the difference in days since the argument. So I changed the name to daysSince. Also, I noted that there weren’t any tests for this method, so I wrote them.
@@ -510,7 +510,7 @@ The next method is compare (lines 902–913). Again, this method is inappropriat
The next six functions (lines 915–980) are all abstract methods that should be implemented in DayDate. So I pulled them all up from SpreadsheetDate.
The last function, isInRange (lines 982–995) also needs to be pulled up and refactored. The switch statement is a bit ugly [G23] and can be replaced by moving the cases into the DateInterval enum.
```java
public enum DateInterval {
OPEN {
public boolean isIn(int d, int left, int right) {
@@ -541,7 +541,7 @@ The last function, isInRange (lines 982–995) also needs to be pulled up and re
int right = Math.max(d1.getOrdinalDay(), d2.getOrdinalDay());
return interval.isIn(getOrdinalDay(), left, right);
}
```
That brings us to the end of DayDate. So now we’ll make one more pass over the whole class to see how well it flows.
First, the opening comment is long out of date, so I shortened and improved it [C2].