I'm a great fan of refactoring but I've been wondering about the issues raised by refactoring.
Fowler advises refactoring
- to make code readable to all users
- to make the code structure more sensible, e.g. methods in the class most affected by them
- to make the overall project more OO - and hence more easily maintained and extended
To this end he dislikes temps and long lists of parameters in methods. He sees a lot of temps or parameters as symptoms of more fundamental problems with the code structure. But getting rid of temps can itself raise other issues. Obviously if a temp is brought in to hold the output of one method before it's input to another method, you lose code readability if you just pipeline the two methods on a single statement and eliminate that temp. So we can't eliminate all temps if we want to maintain code readability.
But we should try to remove as many as possible. The crudest way to eliminate a share of temps is to use global variables. But with this you are making mere utility objects into class-wide fields (attributes). This would lead to a lot of fields with few of them having a claim to be germane to the definition, or working essence, of the class per se. This is hardly in the spirit of good OO design.
Often we see the input parameters to a class made into that class' fields. However, if the input parameters are simply containers (e.g. input files, collections or aggregates) for the data vital to a class, then it makes no sense to make such things into class fields. It would make more sense for the constructor to extract the essential data from these inputs and make those objects/collections the class fields. Likewise if a class is a data processor of some kind - think of a text processor which has file inputs for raw text and 'noise' words and which produces data structures for 'noise' and non-noise words - then I think it's arguable to have the output data structures of the process also defined as class fields. This especially so when the output objects will be used as inputs to, say, some stat analysis or graphic display classes. Here we clearly need some methods to getOutputCollection1(), getOutputCollection2(), etc. Having those output collections stored as class fields with their predefined getters facilitates this retrieval without re-running the processing to produce them. Other types of class (model classes for real-world objects, e.g. HotelRoom) may not require such judicious consideration of field data as the attributes would be more obvious.
I have seen very little focus on selection or use of class fields. It's hard enough to find anything but a short discussion on this matter. There's no mention at all on their potential for eliminating temps in Fowler's or other books on refactoring.
If any other SO members have views on this question then I would like to receive them.
I attach a code example that is small enough and big enough, I think. This program takes a load of pre-formatted messages from a file holding a large block of messages from a free email service ("gPost") and takes another file of noise words. It imports into suitable data structures both the message text and the noise words. It then creates a LinkedList of all non-noise words in the messages from each mailbox user and outputs all users' top 10 most frequently used non-noise words.
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>[email protected]</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. [email protected]
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();
}
}
}
temps
, do you mean something like these temps?