Blog-Archiv

Sonntag, 28. November 2021

Refactoring the Code Duplication Example

This article refers to the code duplication example I presented in my preceding Blog.
I would propose two steps for getting rid of the code duplications in the example:

  1. Move the if-conditions away from AuthorExtractor into the caller by using the Java 8 functional interface Predicate

  2. Wrap the three different document-classes into just one class that provides a unified interface to them

Here are the methods in AuthorExtractor from which code duplications are to be removed: 

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
    public List<String> withNamepartFromDocumentsYoungerThan(
            String namePart,
            Date date,
            List<CsvDocument> csvDocuments,
            List<ExcelDocument> excelDocuments,
            List<PdfDocument> pdfDocuments)
    {
        final List<String> authors = new ArrayList<String>();
        
        for (CsvDocument csvDocument : csvDocuments)    {
            final String authorFullName = csvDocument.getAuthorFullName();
            final Date createdAt = csvDocument.getCreatedAt();
            // START code duplication 1
            if (authorFullName.contains(namePart) && createdAt.after(date))    {
                authors.add(authorFullName);
            }
            // END code duplication 1
        }
        
        for (ExcelDocument excelDocument : excelDocuments)    {
            final String author = excelDocument.getAuthor();
            final Date creationDate = excelDocument.getCreationDate();
            // START code duplication 1
            if (author.contains(namePart) && creationDate.after(date))    {
                authors.add(author);
            }
            // END code duplication 1
        }
        
        for (PdfDocument pdfDocument : pdfDocuments)    {
            final String authorFirstName = pdfDocument.authorFirstName();
            final String authorLastName = pdfDocument.authorLastName();
            final String authorFirstAndLastName = authorFirstName+" "+authorLastName;
            final Date createdAt = pdfDocument.createdAt();
            // START code duplication 1
            if (authorFirstAndLastName.contains(namePart) && createdAt.after(date))    {
                authors.add(authorFirstAndLastName);
            }
            // END code duplication 1
        }
        
        return authors;
    }
    
    // START code duplication 2
    public List<String> withNamestartFromDocumentsOlderEqual(
            String nameStart,
            Date date,
            List<CsvDocument> csvDocuments,
            List<ExcelDocument> excelDocuments,
            List<PdfDocument> pdfDocuments)
    {
        final List<String> authors = new ArrayList<String>();
        
        for (CsvDocument csvDocument : csvDocuments)    {
            final String authorFullName = csvDocument.getAuthorFullName();
            final Date createdAt = csvDocument.getCreatedAt();
            // START code duplication 3
            if (authorFullName.startsWith(nameStart) && (createdAt.equals(date) || createdAt.before(date)))    {
                authors.add(authorFullName);
            }
            // END code duplication 3
        }
        
        for (ExcelDocument excelDocument : excelDocuments)    {
            final String author = excelDocument.getAuthor();
            final Date creationDate = excelDocument.getCreationDate();
            // START code duplication 3
            if (author.startsWith(nameStart) && (creationDate.equals(date) || creationDate.before(date)))    {
                authors.add(author);
            }
            // END code duplication 3
        }
        
        for (PdfDocument pdfDocument : pdfDocuments)    {
            final String authorFirstName = pdfDocument.authorFirstName();
            final String authorLastName = pdfDocument.authorLastName();
            final String authorFirstAndLastName = authorFirstName+" "+authorLastName;
            final Date createdAt = pdfDocument.createdAt();
            // START code duplication 3
            if (authorFirstAndLastName.startsWith(nameStart) && (createdAt.equals(date) || createdAt.before(date)))    {
                authors.add(authorFirstAndLastName);
            }
            // END code duplication 3
        }
        
        return authors;
    }
    // END code duplication 2

Move Search Condition to Caller

When comparing the withNamepartFromDocumentsYoungerThan() method with withNamestartFromDocumentsOlderEqual() it is obvious that the if-conditions (line 14, 24, 36 - 59, 69, 81) are the only difference between them. When this can be externalized, we should end up with just one method.

This refactored method can't have the condition in its name any more, because the caller will determine the condition, so we call it findAll() now: 

import java.util.function.Predicate;

public class AuthorExtractor
{
    /** Predicates passed to find() will receive objects of this class. */
    public static class ConditionParameter
    {
        public final String author;
        public final Date creationDate;
        
        public ConditionParameter(String author, Date creationDate)    {
            this.author = author;
            this.creationDate = creationDate;
        }
    }
    
    public List<String> findAll(
            Predicate<ConditionParameter> condition,
            List<CsvDocument> csvDocuments,
            List<ExcelDocument> excelDocuments,
            List<PdfDocument> pdfDocuments)    {

        .....
    }
}

Externalizing the condition means that we need to provide a parameter type to the Predicate class. This has been implemented by the immutable class ConditionParameter. Mind that the public members do not violate encapsulation as they are final.

Here is what the caller in CodeDuplication.main() must do now: 

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
public class CodeDuplication
{
    public static void main(String[] args) throws Exception {
        final List<CsvDocument> csvDocuments = new ArrayList<>();
        csvDocuments.add(new CsvDocument());
        final List<ExcelDocument> excelDocuments = new ArrayList<>();
        excelDocuments.add(new ExcelDocument());
        final List<PdfDocument> pdfDocuments = new ArrayList<>();
        pdfDocuments.add(new PdfDocument());
        
        final Date date1 = Date.from(Instant.now().minus(Period.of(0, 0, 2)));    // 2 days ago
        final List<String> authors1 = new AuthorExtractor().findAll(
                (p) -> p.author.contains("a") && p.creationDate.after(date1),
                csvDocuments,
                excelDocuments,
                pdfDocuments);
        System.out.println(authors1);
        
        final Date date2 = Date.from(Instant.now());
        final List<String> authors2 = new AuthorExtractor().findAll(
                (p) -> p.author.contains("A") && (p.creationDate.equals(date2) || p.creationDate.before(date2)),
                csvDocuments,
                excelDocuments,
                pdfDocuments);
        System.out.println(authors2);
    }
}

In line 13 and 21 are the lambdas that implement the Predicate conditions needed for the new findAll() method. And there is room for many more conditions, although restricted to dealing with author and creationDate.

Now that the condition in AuthorExtractor doesn't require code duplications any more, we can implement the new findAll() method: 

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
    public List<String> findAll(
            Predicate<ConditionParameter> condition,
            List<CsvDocument> csvDocuments,
            List<ExcelDocument> excelDocuments,
            List<PdfDocument> pdfDocuments)
    {
        final List<String> authors = new ArrayList<String>();
        
        for (CsvDocument csvDocument : csvDocuments)    {
            final String authorFullName = csvDocument.getAuthorFullName();
            final Date createdAt = csvDocument.getCreatedAt();
            if (condition.test(new ConditionParameter(authorFullName, createdAt)))    {
                authors.add(authorFullName);
            }
        }
        
        for (ExcelDocument excelDocument : excelDocuments)    {
            final String author = excelDocument.getAuthor();
            final Date creationDate = excelDocument.getCreationDate();
            if (condition.test(new ConditionParameter(author, creationDate)))    {
                authors.add(author);
            }
        }
        
        for (PdfDocument pdfDocument : pdfDocuments)    {
            final String authorFirstName = pdfDocument.authorFirstName();
            final String authorLastName = pdfDocument.authorLastName();
            final String authorFirstAndLastName = authorFirstName+" "+authorLastName;
            final Date createdAt = pdfDocument.createdAt();
            if (condition.test(new ConditionParameter(authorFirstAndLastName, createdAt)))    {
                authors.add(authorFirstAndLastName);
            }
        }
        
        return authors;
    }

Due to externalizing the search-condition we could remove the copied search-method withNamestartFromDocumentsOlderEqual(). Step one is done.

Still there are three loops, should be just one. So let's continue with step two.

Wrap Different Document Classes into just One

The idea is to write one wrapper class for the different document-types that provides all search criterions (like author and creation-date) generically. The class ConditionParameter already encapsulates these criterions, so we will reuse it in the wrapper.

In this step we will not touch the calls in CodeDuplication.main() any more. We can put all document wrappers privately into AuthorExtractor. 

    private static class Document
    {
        public final ConditionParameter authorAndCreationDate;
        
        Document(CsvDocument csvDocument) {
            this.authorAndCreationDate = new ConditionParameter(
                    csvDocument.getAuthorFullName(),
                    csvDocument.getCreatedAt());
        }
        Document(PdfDocument pdfDocument) {
            this.authorAndCreationDate = new ConditionParameter(
                    pdfDocument.authorFirstName()+" "+pdfDocument.authorLastName(),
                    pdfDocument.createdAt());
        }
        Document(ExcelDocument excelDocument) {
            this.authorAndCreationDate = new ConditionParameter(
                    excelDocument.getAuthor(),
                    excelDocument.getCreationDate());
        }
    }

This helper class will wrap exactly one document. For each of the three document-types there is a dedicated constructor (so if you add another type, you will need another constructor here). After construction, all necessary information for the search will have been fetched from the original document to the wrapper.

One more wrapper encpsulates all three lists. Again, if another document-list gets added, also this class needs maintenance: 

    private static class Documents implements Iterable<Document>
    {
        private final Iterator<CsvDocument> csvDocuments;
        private final Iterator<ExcelDocument> excelDocuments;
        private final Iterator<PdfDocument> pdfDocuments;
        
        Documents(
                List<CsvDocument> csvDocuments,
                List<ExcelDocument> excelDocuments,
                List<PdfDocument> pdfDocuments)    {
            this.csvDocuments = csvDocuments.iterator();
            this.excelDocuments = excelDocuments.iterator();
            this.pdfDocuments = pdfDocuments.iterator();
        }
        
        @Override
        public Iterator<Document> iterator() {
            return new Iterator<AuthorExtractor3.Document>()
            {
                @Override
                public boolean hasNext() {
                    return csvDocuments.hasNext() || 
                            excelDocuments.hasNext() || 
                            pdfDocuments.hasNext();
                }
                @Override
                public Document next() {
                    return csvDocuments.hasNext() ? new Document(csvDocuments.next())
                        : excelDocuments.hasNext() ? new Document(excelDocuments.next())
                            : pdfDocuments.hasNext() ? new Document(pdfDocuments.next())
                                : null;
                }
            };
        }
    }

This implements the Java interface Iterable to be used in a for-loop. The Iterable implementation is done in an anonymous class that delegates to the three lists. It returns a wrapper Document so that the caller doesn't need to care any more for the type of the document.

Now we can implement the findAll() method in a very simple way: 

    public List<String> findAll(
            Predicate<ConditionParameter> condition,
            List<CsvDocument> csvDocuments,
            List<ExcelDocument> excelDocuments,
            List<PdfDocument> pdfDocuments)
    {
        final List<String> authors = new ArrayList<String>();
        
        for (Document document : new Documents(csvDocuments, excelDocuments, pdfDocuments))
            if (condition.test(document.authorAndCreationDate))
                authors.add(document.authorAndCreationDate.author);
        
        return authors;
    }

The loop now uses the Iterable interface of the generic class Documents. The condition inside the loop calls the given Predicate parameter, and when that condition succeeds, the document is added to the list.
That's what we need. Easy to read and understand, short and concise.

Here is the full refactored source code of AuthorExtractor: 

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
import java.util.function.Predicate;

public class AuthorExtractor
{
    /** Predicate lambdas passed to find() will receive objects of this class. */
    public static class ConditionParameter
    {
        public final String author;
        public final Date creationDate;
        
        public ConditionParameter(String author, Date creationDate)    {
            this.author = author;
            this.creationDate = creationDate;
        }
    }
    
    public List<String> findAll(
            Predicate<ConditionParameter> condition,
            List<CsvDocument> csvDocuments,
            List<ExcelDocument> excelDocuments,
            List<PdfDocument> pdfDocuments)
    {
        final List<String> authors = new ArrayList<String>();
        
        for (Document document : new Documents(csvDocuments, excelDocuments, pdfDocuments))
            if (condition.test(document.authorAndCreationDate))
                authors.add(document.authorAndCreationDate.author);
        
        return authors;
    }
    
    
    private static class Document
    {
        public final ConditionParameter authorAndCreationDate;
        
        Document(CsvDocument csvDocument) {
            this.authorAndCreationDate = new ConditionParameter(
                    csvDocument.getAuthorFullName(),
                    csvDocument.getCreatedAt());
        }
        Document(PdfDocument pdfDocument) {
            this.authorAndCreationDate = new ConditionParameter(
                    pdfDocument.authorFirstName()+" "+pdfDocument.authorLastName(),
                    pdfDocument.createdAt());
        }
        Document(ExcelDocument excelDocument) {
            this.authorAndCreationDate = new ConditionParameter(
                    excelDocument.getAuthor(),
                    excelDocument.getCreationDate());
        }
    }
    
    private static class Documents implements Iterable<Document>
    {
        private final Iterator<CsvDocument> csvDocuments;
        private final Iterator<ExcelDocument> excelDocuments;
        private final Iterator<PdfDocument> pdfDocuments;
        
        Documents(
                List<CsvDocument> csvDocuments,
                List<ExcelDocument> excelDocuments,
                List<PdfDocument> pdfDocuments)    {
            this.csvDocuments = csvDocuments.iterator();
            this.excelDocuments = excelDocuments.iterator();
            this.pdfDocuments = pdfDocuments.iterator();
        }
        
        @Override
        public Iterator<Document> iterator() {
            return new Iterator<AuthorExtractor.Document>()
            {
                @Override
                public boolean hasNext() {
                    return csvDocuments.hasNext() || 
                            excelDocuments.hasNext() || 
                            pdfDocuments.hasNext();
                }
                @Override
                public Document next() {
                    return csvDocuments.hasNext() ? new Document(csvDocuments.next())
                        : excelDocuments.hasNext() ? new Document(excelDocuments.next())
                            : pdfDocuments.hasNext() ? new Document(pdfDocuments.next())
                                : null;
                }
            };
        }
    }

}

Resume

Refactoring code duplications is not an easy task. Depending on the reason for the duplication you will need to find concepts on method- and class-level. Introducing interfaces will always be a safe way, although functional expressions may be much shorter. The effort for fixing code duplications can be saved when writing professional code right from start.




Keine Kommentare:

Kommentar veröffentlichen

Abonnieren Kommentare zum Post (Atom)