我非常喜欢重构,但我一直想知道重构所带来的问题。
Fowler建议重构
为此,他不喜欢方法中的临时和冗长的参数列表。他认为很多温度或参数都是代码结构中更基本问题的症状。但摆脱临时工本身也会引发其他问题。显然,如果在将一个方法输入到另一个方法之前,引入一个temp来保存它的输出,那么如果您只是在一个语句上管道化这两个方法并消除这个临时方法,那么您就失去了代码可读性。因此,如果我们想要保持代码的可读性,就不能消除所有的临时代码。
但是我们应该尽可能多的移除。消除部分临时值的最简单的方法是使用全局变量。但是使用这种方法,您只需要将实用程序对象变成类范围的字段(属性)。这将导致许多领域,其中很少有人声称与阶级本身的定义或工作本质密切相关。这很难符合良好的OO设计的精神。
我们经常看到类的输入参数被输入到该类的字段中。但是,如果输入参数只是对类至关重要的数据的容器(例如,输入文件、集合或集料),那么将这些内容放入类字段是没有意义的。构造函数从这些输入中提取基本数据并使这些对象/集合成为类字段将更有意义。同样地,如果一个类是某种类型的数据处理器--想想一个文本处理器,它有原始文本和“噪声”字的文件输入,并为‘噪音’和无噪声的单词生成数据结构--那么我认为让进程的输出数据结构也被定义为类字段是有争议的。尤其是当输出对象被用作一些stat分析或图形显示类的输入时尤其如此。在这里,我们显然需要一些方法来实现getOutputCollection1()、getOutputCollection2()等。将这些输出集合与它们的预定义的getter一起存储为类字段,可以简化这种检索,而无需重新运行处理来生成它们。其他类型的类(用于实际对象的模型类,例如HotelRoom)可能不需要对字段数据进行明智的考虑,因为属性将更加明显。
我很少看到对类字段的选择或使用的关注。在这个问题上,除了一个简短的讨论之外,很难找到任何东西。在Fowler的书或其他关于重构的书中,根本没有提到它们消除临时的潜力。
如果其他议员对这问题有意见,我希望收到。
我附上一个代码示例,它足够小,足够大,我认为。该程序从一个文件中获取大量预先格式化的消息,其中包含来自免费电子邮件服务("gPost")的大量消息块,并接收另一个噪音词文件。它将消息文本和噪声词导入到合适的数据结构中。然后,它创建一个来自每个邮箱用户的消息中所有无噪声单词的LinkedList,并输出所有用户最常用的10个最常用的无噪声单词。
public class GPostProcessor
{
private List<GPost> gPosts = new ArrayList<>(); // GPost ArrayList holding ALL contents of input file
private Set<String> noiseWords; // HashSet buffer for noise words
private Set<MyString> mailboxHandles = new TreeSet<>(); // TreeSet for holding mailbox handles alphabetically
private Map<MyString, List<WordCount>> wordUsages = new HashMap<>(); // HashMap collection for vocab list of all mailboxes
// CONSTRUCTORS ...
/** Parameterless constructor.<br>
* The system cannot operate without input file parameters.<br>
* So the parameterless constructor essentially displays an error message <br>
* before conducting an orderly shut down of the program.
*/
GPostProcessor()
{
System.out.println("ERROR at GPostProcessor - no parameters in constructor !"
+ "\nThe system will now shut down in 5 seconds.");
try
{
TimeUnit.SECONDS.sleep(5);
}
catch(Exception e)
{
e.printStackTrace();
}
System.exit(0);
System.out.println();
throw new IllegalArgumentException("No parameters for GPostProcessor instance ! "
+ "\nSystem shutdown follows.");
}
/** Parametered constructor.<br>
* Input messages and noise-words files are applied to input parameters.<br>
* @param messagesFile A String holding the name of the messages file.
* @param noiseWordsFile A String holding the name of the noise words file.
*/
GPostProcessor(String messagesFile, String noiseWordsFile)
{
if (messagesFile == null)
{
System.out.println("ERROR at GPostProcessor parametered constructor !"
+ "\nNull parameter for messages file."
+ "\nThe system will now shut down in 5 seconds . . .");
try
{
TimeUnit.SECONDS.sleep(5);
}
catch(Exception e)
{
e.printStackTrace();
}
System.exit(0);
}
else if (noiseWordsFile.equals(null))
{
System.out.println("ERROR at GPostProcessor parametered constructor !"
+ "\nNull parameter for noise-words file.\n"
+ "\nThe system will now shut down in 5 seconds . . .");
try
{
TimeUnit.SECONDS.sleep(5);
}
catch(Exception e)
{
e.printStackTrace();
}
System.exit(0);
}
else // Fill in primary data structures ...
{
this.gPosts = parseToGPosts(loadGPostMessages(messagesFile)); // Extract all gPosts from input file to one big string
this.noiseWords = loadNoiseWords(noiseWordsFile); // Load noise words from their file into a HashSet
}
}
// GETTERS & SETTERS - mainly used for the JUnit tests.
/******************************************************************
***** This is the MAIN class for the gPostProcessor package. *****
*
* @param args A String array into which arguments may be put.
*****************************************************************/
public static void main(String[] args)
{
final Instant startTime = Instant.now();
new GPostProcessor("messages3.txt", "noiseWords.txt").processGPosts();
final Instant endTime = Instant.now();
final long duration = Duration.between(startTime, endTime).toNanos();
System.out.printf("\n%-10s%12s%1s%2s", "Duration: ", duration, " ", "ns");
}
// DATA MANIPULATION METHODS ...
/** Processes the file of gPosts, <i>messagesFile</i>, excluding words in the
* noise words file, <i>noiseWordsFile</i>.
* Each gPost message is drawn from an ArrayList<String> buffer generated
* by method <i>loadGPostMessages(.)</i>, converted into GPost objects and analysed
* before adding its mailbox handle and WordCount list to a HashMap collection, <i>wordUsages</i>.
* Finally, a report is generated showing the 10 most used words for each gPost
* user whose emails are listed in <i>messagesFile</i>.
*/
void processGPosts()
{
GPost gPost; // Temp GPost object
MyString mHandle; // Temp gPost address handle
List<WordCount> messageList; // Temp WordCount list got from a GPost
for(int p = 0; p < gPosts.size(); p++) // Analyse each gPost in the input file
{
System.out.println("Processing gPost #" + (p + 1) + " ..."); // Display email # being processed
gPost = gPosts.get(p);
mHandle = getMailboxHandle(gPost); // Extract the user's mailbox handle
mailboxHandles.add(mHandle); // Add mailbox handle to TreeSet of handles
messageList = getMessageList(gPost); // Extract the wordCountList from the gPost
mergeInUserVocab(mHandle, messageList); // Merge wordCountList with existing vocabList
}
Map<MyString, List<WordCount>> wordUsagesTop10 = prepareDataForReport(); // Prepare data for report ...
generateReport(wordUsagesTop10, "report.txt"); // ... and generate report for mailbox user
}
// FILE BUFFERING METHODS ...
/** Reads in gPost items line by line from the text file, <i>messagesFile</i> and puts
* its contents immediately into a single big String object.<br>
* This ensures minimal delay at the slowest stage of the system, i.e. reading input.
* Parsing and analysing of gPost items from the input file is done by other non-I/O methods.
* @param messagesFile A String holding the name of a file holding all gPosts.
* @return A String object holding all the input file contents..
*/
String loadGPostMessages(String messagesFile)
{
final Instant startTime = Instant.now();
StringBuilder contents = new StringBuilder(),
textLine = new StringBuilder(); // Output gPosts as a single StringBuilder
if (messagesFile == null)
{ // Check for null filename
contents = null;
System.out.println("ERROR ! No or null filename for messages file."
+ "\nPlease check messages file and ensure valid name entered.");
}
else
{
try
{
BufferedReader buffReader = new BufferedReader(new FileReader(messagesFile));
while ( !textLine.append(buffReader.readLine() ).toString().equals("null") ) // While file has content in next line ...
{
contents = contents.append(textLine); // ... add line to file contents StringBuilder
textLine.setLength(0); // ... then clear textLine for next read.
}
buffReader.close();
}
catch(IOException re)
{
contents = null;
System.out.println("\nReading of " + messagesFile + " failed.");
re.printStackTrace();
}
}
final Instant endTime = Instant.now();
final long duration = Duration.between(startTime, endTime).toNanos();
System.out.printf("\n%-10s%12s%1s%2s", "Load Duration: ", duration, " ", "ns");
return contents.toString();
}
/** Reads in noise words from the text file, <i>noiseWordsFile</i>, and puts
* them into a HashSet<String>. <br>
* Noise words are assumed to be on separate lines of the text file to
* facilitate rapid reading of the input.
*
* @param noiseWordsFile A String object giving the name of the file
* that holds all the noise words.
* @return A HashSet<String> holding all the noise words.
*/
Set<String> loadNoiseWords(String noiseWordsFile)
{
Set<String> noiseWords = new HashSet<>();
String textLine;
try // Read in noise words from textfile, "noiseWordsFile"
{
BufferedReader buffReader = new BufferedReader(new FileReader(noiseWordsFile));
textLine = buffReader.readLine();
while (textLine != null)
{
noiseWords.add(textLine.toLowerCase().trim());
textLine = buffReader.readLine();
}
buffReader.close();
}
catch(IOException re)
{
System.out.println("Read of " + noiseWordsFile + " failed.\n");
re.printStackTrace();
}
return noiseWords;
}
// PROCESSING METHODS ...
// ==================
/** Check each block of the gPost string to see if it is in valid gPost format.
* @return A String value that holds the error message if the gPostsText is not validly
* formatted or a null string otherwise.
* */
String validGPost(StringBuilder gPostsText)
{
int mHandle = gPostsText.substring(0, 254).indexOf("@gPost.com"), // Find index of first email handle
start = gPostsText.indexOf("gPostBegin"), // ... next gPostBegin ...
end = gPostsText.indexOf("gPostEnd"); // ... and next gPostEnd
if (mHandle < 0)
{
return "ERROR in input text: missing sender address.";
}
else if (start < 0)
{
return "ERROR in input text: missing message preamble.";
}
else if (end < 0)
{
return "ERROR in input text: missing message epilogue.";
}
else if (start < mHandle)
{
return "ERROR in input file: message preamble before sender.";
}
else if (end < mHandle)
{
return "ERROR in input file: message epilogue before sender.";
}
else if (end < start)
{
return "ERROR in input file: message epilogue before preamble.";
}
else
{
return null;
}
}
/** Converts a large string containing many gPosts into a ArrayList<GPost> collection.
*
* @param gPostsString A String object that carries the important contents of a gPosts' file.
* @return An ArrayList<GPost> collection holding sender and message text for the input gPosts.
*/
ArrayList<GPost> parseToGPosts(String gPostsString)
{
ArrayList<GPost> result = new ArrayList<>(); // Copy input file String to a StringBuilder
StringBuilder sender = new StringBuilder(""),
message = new StringBuilder("");
String error;
int postNum = 0, // gPost index
iHandle, // Email handle index
start, // Start index of message
end = 0; // End index of message
if (!gPostsString.equals(null))
{
StringBuilder gPostsText = new StringBuilder(gPostsString.replaceAll("\\r\\n|\\r|\\n", " ")); // Turn line breaks to spaces
while (gPostsText.length() != 0)
{
if ((error = validGPost(gPostsText)) != null) // Invalid gPost formatting ?
{
System.out.println("\n Post #" + postNum + " - " + error); // => Output error
}
else // Valid gPost format ...
{
iHandle = gPostsText.indexOf("@gPost.com"); // Find index of handle
sender.append(gPostsText.substring(0, iHandle + 10).trim()); // Store sender handle
start = gPostsText.indexOf("gPostBegin") + 10; // Start of gPost message is after preamble
end = gPostsText.indexOf("gPostEnd"); // End of gPost message is before epilogue
message.append(gPostsText.substring(start, end).trim()); // Strip out gPost message
if (message.equals(null))
{
System.out.println("\n Post #" + postNum + ": Empty message."); // Empty message warning
break;
}
else
{
result.add(postNum, new GPost(sender.toString(), message.toString())); // Add to gPost ArrayList
start = end + 8; // Reset start index for remainder of gPostsString
sender.setLength(0); // Clear sender ...
message.setLength(0); // ... and message StringBuilders
}
}
gPostsText = gPostsText.delete(0, end + 8);
postNum++;
}
}
else
{
result = null;
System.out.println("No gPosts produced. \nPlease check input file");
}
return result;
}
/** Returns the mailbox handle for the sender of a gPost message.<br>
* The mailbox handle for a sender with an email address of <i>john.west@gPost.com</i>
* is simply the email with the <i>{@literal @}</i> character and the domain name removed,
* i.e. <i>john.west</i>. <br>
*
* @param gPost A GPost object holding the contents of a gPost message.
* @return Returns the handle of the email address sending a GPost message.
*/
MyString getMailboxHandle(GPost gPost)
{
return new MyString(gPost.getSender().substring(0, gPost.getSender().indexOf("@")));
}
/** Analyses the input gPost to generate the list of non-noise words in it.
* Noise words are drawn from the class attribute, <i>noiseWords</i>.<br>
* @param gPost A GPost object holding the contents of a gPost message. .
* @return Returns the unsorted WordCount list extracted from a GPost message.
*/
List<WordCount> getMessageList(GPost gPost)
{
List<WordCount> result = new LinkedList<>(); // Returned List of non-noise words
List<String> wordList; // List for words extracted from gPost message
WordCount newWordCount; // A WordCount object for additions to messageList
String word; // Any word in wordList
int i = 0, // Iteration indices
j = 0;
String message = gPost.getMessageText().toLowerCase(); // Extract message text from gPost & lowercase it
message = message.replaceAll("[\\p{P}&&[^\u0027]]", " "); // Replace punctuations bar apostrophes by space
wordList = new LinkedList<>(Arrays.asList(message.split(" +"))); // Split message into words then put these into a LinkedList
while (i < wordList.size()) // Remove noise & non-alphabetic words
{
word = wordList.get(i).trim();
if ( (noiseWords.contains(word)) || (!word.matches("[a-z]+$")) ) // Remove noisewords & non-alphabetic char words ...
{
wordList.remove(i); // ... this includes words with apostrophes
}
else // CARE! Don't increment iterator index after removal !
{
for(j = 0; j < result.size() && !result.get(j).getWord().equals(word); j++); // Check each word in the message ...
{
if (j < result.size()) // Already on messageList ?
result.get(j).setCount(result.get(j).getCount() + 1); // => increment its count
else // Not on messageList ...
{
newWordCount = new WordCount(word, 1); // => create WordCount object for that word with count = 1 ...
result.add(newWordCount); // Increment wordList iterator
}
i++;
}
}
}
return result;
}
/** Merges a WordCount list (<i>messageList</i>) got from a gPost message with the
* WordCount list for the entire message vocabulary (<i> vocabList</i>) already held
* for that same gPost user.
* To make subsequent mergings as efficient as possible, we exploit the fact that the
* word use frequency by a person follows a Zipf distribution - the discrete variable
* version of the Pareto or 80/20 distribution. Basically, the vast majority of the words
* a person uses comes from a small minority of their vocabulary. So by keeping <i>vocabList</i>
* sorted in order of frequency and the most frequent word at the head of the list, we
* can sharply reduce the average search time for words in subsequent messages.
*
* @param messageList A LinkedList<WordCount> object holding WordCount data from
* a single message.
* @return A LinkedList<WordCount> collection showing a given user's WordCount data.
*/
void mergeInUserVocab(MyString mHandle, List<WordCount> messageList)
{
int j, // Indices for list items
mark;
WordCount wordCount;
List<WordCount> vocabList = wordUsages.get(mHandle); // Temp for mailbox WordCount list
if (vocabList == null) // When existing vocabList for that handle is empty ...
{
messageList.sort(WordCount::compareByCount); // ... sort the messageList by COUNT and assign it to vocabList
wordUsages.put(mHandle, messageList);
}
else // When the existing vocabList for that handle is NOT empty ...
{ // ... check if each word in messageList exists in vocabList
for(int i = 0; i < messageList.size(); i++)
{
wordCount = messageList.get(i);
for(j = 0; j < vocabList.size(); j++) // Check vocabList for each word in messageList
{
if(wordCount.getWord().equals(vocabList.get(j).getWord())) // When a word match is found ...
{
vocabList.get(j).setCount(vocabList.get(j).getCount() + wordCount.getCount()); // .. update vocabList count accordingly
break; // ... and stop searching vocabList
}
} // NOTE: j is closing index of vocabList search loop
mark = j; // Set insertion index at either vocabList match index or list tail
while (mark > 0 && vocabList.get(mark - 1).getCount() < wordCount.getCount()) // Find correct count location for updated or new word
{
mark--;
}
if (j == vocabList.size()) // When the messageList item is not in the vocabList ...
{
vocabList.add(mark, wordCount); // .. insert the new item at the correct index ...
}
}
}
wordUsages.put(mHandle, vocabList); // wordUsages now updated & re-sorted by count.
}
/** Prepares the processed WordCount lists for each user so that the 10 most frequently used words
* are stored against email address for each mailbox owner. <br/>
* Since wordUsages contains lists of words that are SORTED BY FREQUENCY ALONE and have not yet
* been sorted alphabetically, we have to look at words on each list after the tenth word in case
* some of these have the same count as the latter.
* After all of these are identified and added to the top 10 words, the resulting list can
* be sorted by firstly count and then alphabetically for words on equal count.
* @return A HashMap collection of the most frequently used words in all gPost users hashed by
* the user's email handle.
* */
Map<MyString, List<WordCount>> prepareDataForReport()
{
Map<MyString, List<WordCount>> result = new HashMap<>(); // Temp to hold top 10 words for each user
List<WordCount> wordUsage = new LinkedList<>(); // Temp for user vocab
System.out.println("Preparing data for report ..."); // Log system status
for(MyString mHandle : mailboxHandles) // Find each user's mailbox address
{ // Locate associated user's vocab list
int i = 0;
wordUsage = wordUsages.get(mHandle);
for(i = 0; i < 10; i++) // ... fill its first 10 words
{
wordUsage.set(i, wordUsages.get(mHandle).get(i));
}
while (wordUsage.get(i).getCount() == wordUsage.get(9).getCount()) // ... get words with same count as 10th word ...
{
result.get(mHandle).set(i, wordUsage.get(i)); // ... then add any such word to the sub-list
i++;
}
wordUsage.sort(WordCount::compareByCountThenWord); // Finally re-sort wordUsageTops by count and then word ...
wordUsage.subList(10, wordUsage.size()).clear(); // Trim list to first 10 elements
result.put(mHandle, wordUsage); // ... and update
}
return result;
}
/** Generates a report showing top 10 word use data for each mailbox user found in a batch
* of messages the <i>gPostMessages.txt</i> input file.
* gPost users' data are tabulated by email address in alphabetical order.
* Within a given user's data, words are listed primarily by frequency of use, with
* words of equal frequency being ordered alphabetically.
*
* The source data for the report is a HashMap, <i>wordUsagesTop10</i>, an abridged version
* of the class field, <i>wordUsages</i>. This holds just the 10 most frequently used words
* for each gPost mailbox user. <br/>
*
* @param reportFile A String holding the name of the report file.
* listed in the input file.
*/
void generateReport(Map<MyString, List<WordCount>> wordUsagesTop10, String reportFile)
{
MyString email; // Resulting email address, e.g. john.west@gPost.com
final String headingFormat = "%-30s%-10s%n", // Strings to format report ...
emailFormat = "%n%n%-30s",
wordCountFormat = "%15s",
wordFormat = "%-5s",
countFormat = "%n%35s";
System.out.println("Generating report file ..."); // Log system status ...
try
{
BufferedWriter buffWriter = new BufferedWriter(new FileWriter(reportFile));
buffWriter.write(String.format(headingFormat, "MAILBOX", "WORD USAGE")); // Write report headings to output file
buffWriter.write(String.format(headingFormat, "=======", "=========="));
System.out.println("Report file opened ..."); // Console status notification during output writing
for (MyString mHandle : mailboxHandles) // Write all users' top words plus their frequencies
{
email = new MyString(mHandle + "@gPost.com");
buffWriter.write(String.format(emailFormat, email.getValue())); // Email address to left of output line ...
buffWriter.write(String.format(wordFormat, "WORD")); // ... then the WORD row ...
for (int i = 0; i < 10; i++) // ... then 10 most commonly used words
{
buffWriter.write(String.format(wordCountFormat, wordUsagesTop10.get(mHandle).get(i).getWord()));
}
buffWriter.write(String.format(countFormat, "COUNT")); // Skip to next line for COUNT row ...
for (int i = 0; i < 10; i++) // ... putting count for each of the 10 words below that word
{
buffWriter.write(String.format(wordCountFormat, wordUsagesTop10.get(mHandle).get(i).getCount()));
}
wordUsagesTop10.clear(); // Clear list ahead of loading next mailbox address
}
buffWriter.close();
System.out.println("Report writing completed."); // Display system status on report writing end
}
catch(IOException we)
{
System.out.println("Error during writing to file: " + reportFile);
we.printStackTrace();
}
}
}发布于 2020-01-02 19:59:03
你的问题似乎是基于误解。福勒一般不会“不喜欢临时工”。恰恰相反,他为引入这些变量提供了一种特定的重构,他称之为
当然,他也给逆运算取了一个名字:
从这一点出发,我希望重构不是关于引入或删除这些变量--而是在这些临时变量有助于可实现性时引入这些变量,以及在代码变得更加简洁而又不失去清晰性的情况下删除它们。
确实还有另一种减少临时变量的重构,称为
(我链接到了SO问题,因为福勒在线目录的版本是有问题的)。
当临时的冗余状态可能过时时,这种重构是有意义的,因此函数调用("query")而不是临时调用可以确保代码保持正确。我认为正确无疑是暂时删除的一个正当理由。
如果我没记错的话,福勒确实描述了临时人员如何使"提取法“重构更加困难,因为可能需要将这些参数作为附加参数传递给提取的方法。类似地,有必要将提取方法中的临时更改返回给调用方。这有时会导致
在这种情况下,用类成员替换临时参数可以避免这种情况,并减少长参数列表。然而,这只是一个潜在的解决方案,有时是好的,有时是丑陋的。通常会有其他选择需要考虑,比如
国际水文学组织没有一般的规则,这些决定中哪一个是最合适的一般方法,我们需要根据具体情况来决定。
发布于 2020-01-02 18:43:09
我认为减少临时变量本身的价值不大,特别是如果这些变量被用来澄清代码的话。
考虑refactoring.guru中的这两个代码示例:
void renderBanner() {
if ((platform.toUpperCase().indexOf("MAC") > -1) &&
(browser.toUpperCase().indexOf("IE") > -1) &&
wasInitialized() && resize > 0 )
{
// do something
}
}-
void renderBanner() {
final boolean isMacOs = platform.toUpperCase().indexOf("MAC") > -1;
final boolean isIE = browser.toUpperCase().indexOf("IE") > -1;
final boolean wasResized = resize > 0;
if (isMacOs && isIE && wasInitialized() && wasResized) {
// do something
}
}哪一个更容易读?
从性能角度(在大多数编程语言中),以这种方式使用临时变量基本上是免费的。
发布于 2020-01-02 22:31:58
这是一个典型的例子,就是读一些东西,却把它搞错了。
临时变量以其最简单、最不丑、最不稳定、最可维护、最有效的形式是局部变量,在需要它们的地方引入,在不再需要时立即超出范围。
您的代码系统地删除了临时变量的声明,从而降低了代码的可读性,并且容易出错。它不只是更难读,它还假装有一个意图保持这种距离,这通常是使用临时变量的其他一些不明显的意图。
盲目地听建议而不理解它,也不理解它背后的原因只会导致问题。
https://softwareengineering.stackexchange.com/questions/403209
复制相似问题