-2

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&lt;String&gt; 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&lt;String&gt;. <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&lt;String&gt; 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&lt;WordCount&gt; object holding WordCount data from
     *  a single message.
     *  @return A LinkedList&lt;WordCount&gt; 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();
    }
}

}
13
  • 3
    Can you provide a citation for the phrase "he dislikes temps and long lists of parameters in methods" so that we have some context? Jan 2, 2020 at 18:34
  • Also, I believe the question would very much benefit from an example, even a trivial one, to better explain what you mean. I think some of the points you make are a a tad too abstract to answer. Jan 2, 2020 at 18:37
  • Refactoring, Fowler, p.21, 120- .
    – Trunk
    Jan 2, 2020 at 18:38
  • 2
    By temps, do you mean something like these temps? Jan 2, 2020 at 18:44
  • 3
    According to this answer, the reason Fowler "dislikes temps" is that they are "often are so plentiful that they make (method) extraction very awkward." If that is the reason, replacing temps with fields would make extraction even more awkward, not less.
    – John Wu
    Jan 2, 2020 at 19:22

4 Answers 4

8

Your question seems to be based on a misunderstanding. Fowler does not "dislike temps" in general. Quite the opposite, he provided a specific refactoring for introducing such variables, he called it

Of course, he also gave the inverse operation a name:

From this I hope it gets clear refactoring is neither about introducing or removing such variables - it is about introducing such temporary variables when they contribute to readibility, and removing them when the code becomes more concise without loosing clarity.

There is indeed another refactoring to reduce temporary variables, it is called

(I linked to the SO question, since the version from Fowler's online catalog is buggy).

This refactoring makes sense when the redundant state of a temporary might become outdated, so a function call ("query") instead of the temporary makes sure the code stays correct. And I think correctness is surely a valid reason for removing a temporary.

If I remember correctly, Fowler indeed described how temporaries can make the "Extract Method" refactoring harder, because one may need to pass these as additional parameters to the extracted method. Similarly, it can become necessary to return any changes to temporaries inside the extracted method back to the caller. This can sometimes lead to

  • functions with long parameter lists, and

  • (by repeated extraction) chains of methods where the same parameter is passed over and over again.

For this case, replacing the temporary by a class member can avoid this and reduce the longish parameter lists. However, this is only one potential solution which is sometimes fine, and sometimes ugly. Often there are alternatives to consider, like

  • using the "Replace Temp with Query" refactoring, or

  • extracting a little bit more or a little bit less code to the new function, or

  • changing the logic slightly in a different manner, or

  • grouping several parameters into a new kind of object.

There is IMHO no general rule which of these decisions is the most suitable approach in general, one needs to decide this on a by-case basis.

3
  • You're reprising Robert Harvey's answer. I'm not knocking temps where they are needed for clarity, cf. paragraph 2 of my initial question. I am just exploring the scope for making certain data class fields where its vital role across the methods of its own class or even to other class methods point strongly to its "right" to be a class attribute. Of course this process will result in much reduction in temps and method parameters. But it is not the primary reason for doing so. I'm refactoring into a class' natural components - not atomizing the class using a loads of spurious attributes !
    – Trunk
    Jan 2, 2020 at 22:04
  • @Trunk: maybe my answer goes in the same direction as Robert's, but it surely does not "reprise it". It appears to me you don't like that direction.
    – Doc Brown
    Jan 3, 2020 at 7:49
  • @Trunk: I added some paragraphs about class members and the "Extract method" refactoring.
    – Doc Brown
    Jan 6, 2020 at 16:14
6

I don't see much value in reduction of temporary variables as a goal unto itself, especially if those variables are being used to clarify the code.

Consider these two code examples from 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
  }
}

Which one is easier to read?

Using temporary variables this way is essentially free, from a performance perspective (in most programming languages).

16
  • 5
    I suppose this has to be said: Fowler is not the be-all and end-all of good design. He has some good ideas, but that's all they are: ideas. They are not laws, mandates or absolutes. Like any other idea, you evaluate it based on your own best judgement. One of the biggest problems in our industry is blindly following precepts like this without questioning them or understanding how they work. Jan 2, 2020 at 19:38
  • 4
    I don't know if Fowler would split the code of the example into more classes, but I am pretty sure Bob Martin would find a way ;-)
    – Doc Brown
    Jan 2, 2020 at 20:14
  • 2
    @Trunk: That's a bit unfair, isn't it? Most of these types of issues are ultimately subjective anyway, and at the end of the day people are going to make up their own damned mind about them, regardless of our opinions. Jan 2, 2020 at 23:27
  • 1
    Maybe so. I'm only in this IT thing myself for the dough, no crusade is in course here. But I still venture an opinion (often wrong, of course, but how else do we learn?) on most stuff I have any experience of. We will make up our mind regardless of opinions - I'm doing that now with @gnasher729 if he can't substantiate his dismissal of my idea - but it is fair to say that we are all better informed when we get other folks' views on a subject - I mean THEIR OWN VIEWS, not their recounting of other people's views. .
    – Trunk
    Jan 2, 2020 at 23:36
  • 2
    @Trunk: I have an opinion about this, but I hate holy wars, that's why I don't comment on this. And I think if someone tends to attack other people on a personal level (like questioning their professionality without even knowing them or overinterpreting their statements), then chances are way pretty high they get trouble not just in their job.
    – Doc Brown
    Jan 3, 2020 at 7:41
2

It's a typical case of reading something and getting it completely wrong.

Temporary variables in their simplest, least ugly, most understable, most maintainable, most efficient form are local variables, introduced exactly at the point where they are needed, going out of scope immediately when they are not needed anymore.

Your code systematically removes the declarations of temporary variables away from their use, making the code less readable and error prone. It's not just harder to read, it also pretends that there is an intent to keep that distance, which would usually be using the temporary variable for some other non-obvious intent.

Blindly listening to advice without understanding it and without understanding the reason behind it only leads to problems.

1
  • "Your code systematically removes the declarations of temporary variables away from their use . . . " Please elaborate ! Are you saying that you consider the class attributes that I declared to be incorrect and better as temps? Or are you saying that the temps that I retained in these methods are localizable ? Where indices are declared outside a loop, it's because I'm using the final value of the index outside it.
    – Trunk
    Jan 2, 2020 at 23:12
1

A fairly good rule of thumb is that variables should exist in the smallest scope necessary. This is because the more variables you have in scope at any time, the harder the code can be to understand, and the greater the chance of weird side-effects and interactions.

For this reason, defining a variable at class-level when it only needs to exist at method-level is something that would be hard to justify.

1
  • As a rule of thumb that's okay though smallest scope necessary demands its own interpretation for each project. For one thing, this phrase can't mean the same as smallest scope possible.
    – Trunk
    Jan 8, 2020 at 0:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.