Refactoring else-if operators with different extensions?

Question

I want to know how we better way refctoring this part of code with else-if operators. When is performed eguals check with different extentions?

Code:

    private void findFiles(String path) {

        try {
            File root = new File(path);
            File[] list = root.listFiles();
            for (File currentFile : list) {
                if (currentFile.isDirectory()) {
                    findFiles(currentFile.getAbsolutePath());
                } else {
                    if (currentFile.getName().toLowerCase().endsWith((".txt"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".pdf"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".doc"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".docx"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".html"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".htm"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".xml"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".djvu"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".djv"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".rar"))) {
                        queue.put(currentFile);
                    } else if (currentFile.getName().toLowerCase()
                            .endsWith((".rtf"))) {
                        queue.put(currentFile);
                    } 
                }
            }
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

    }
}

Questions:

How better way to refactoring code? Make it simpler for
understanding.
Can we use some another way to check extentions variants?

Thanks,
Nazar.

You should probably "cache" `currentFile.getName().toLowerCase()`, at least. — Alvin Wong, Feb 24 '13 at 10:33
is that copy&paste or the code does the same action with every matched extension? — guido, Feb 24 '13 at 10:34
Or get the last index of `"."` and compare the substring (extension)? — Alvin Wong, Feb 24 '13 at 10:34
One, you're being terribly inefficient by calling `currentFile.getName().toLowerCase()` over and over. That results in 3 method calls every time and makes the code hard to understand. You only need to do that once. — Visionary Software Solutions, Feb 24 '13 at 10:35
If you extract the extension you don't have such a hard time comparing it. Since Java 7 you then have the ability to use a switch/case-construct to compare the extension - if you need to distinguish the extensions. Otherwise you might create a List/an Array of the valid extensions and check, if the list contains the extracted extension. — Stefan Neubert, Feb 24 '13 at 10:37

jlordo · Answer 1 · 2013-02-24T10:57:41.717

8

You can replace your whole list of checking extensions with this:

// outside the loop (or even method):
Set<String> extensions = new HashSet<>(Arrays.asList(".txt", ".pdf", ".doc",
                 ".docx", ".html", ".htm", ".xml", ".djvu", ".rar", ".rtf"));
// in the loop:
String fileName = currentFile.getName().toLowerCase();
if (extensions.contains(fileName.substring(fileName.lastIndexOf(".")))) {
    queue.put(currentFile);
}

edited Feb 24 '13 at 10:57

answered Feb 24 '13 at 10:36

jlordo

37,490
6
58
83

I have a feeling he doesn't really want to do the same action in every case, otherwise that code would be dumb. That's probably just for show. – Visionary Software Solutions Feb 24 '13 at 10:37
4

good but you need to extract the extension from the filename – guido Feb 24 '13 at 10:37
Same idea as mine. I would use a HashSet instead of a List, though. O(1) rather than O(n). – JB Nizet Feb 24 '13 at 10:42
@guido: Yes, you are right, forgot about that ;) Edited my answer... – jlordo Feb 24 '13 at 10:48
@JBNizet How do you think of the current answer? Creating a `HashSet` from a `List` doesn't seem quite efficient either. – Alvin Wong Feb 24 '13 at 10:52
@AlvinWong Considering there are 10 items in the list, we are talking about milliseconds (if not less), that will be executed once in the whole program. – assylias Feb 24 '13 at 10:54
1

@assylias and we should remind the OP to put that **outside** the for-loop. – Alvin Wong Feb 24 '13 at 10:56
@AlvinWong Of course. It should not even be in the method. More like a `private static final` member for example. – assylias Feb 24 '13 at 10:57
@jlordo Need add => `... new HashSet(...)` – catch23 Feb 24 '13 at 10:59
@nazar_art: Edited, used the Java 7 diamond operator, though. – jlordo Feb 24 '13 at 11:01
@jlordo don't find correctly? – catch23 Feb 24 '13 at 11:01
@nazar_art: Either way is correct. I am lazy and therefor prefer the diamond operator. – jlordo Feb 24 '13 at 11:02
this won't still work for `.tar.gz` – guido Feb 24 '13 at 11:05
@guido: then you have to add `".gz"` to the list... (or use an `else if`)... Or something else... – jlordo Feb 24 '13 at 11:07
what if you want to accept `.tar.gz` but not `.man.gz` – guido Feb 24 '13 at 11:09
@guido: I'm sure you'll find a solution if you encounter that problem ;) – jlordo Feb 24 '13 at 11:10

score 2 · Answer 2 · edited May 23 '17 at 11:43

2

The best solution would be to refactor this to the STRATEGY Pattern, as seen here:

edited May 23 '17 at 11:43

Community

1
1

answered Feb 24 '13 at 10:37

Visionary Software Solutions

3,825
2
29
35

You don't have to throw design patterns at every problem. Especially since the solution that jlordo proposed is both, elegant and short enough to solve the problem. – joschi Feb 24 '13 at 10:40
1

agree in case he has different logic for each case – Ahmad Y. Saleh Feb 24 '13 at 10:40
Clearly if he really is trying to do the same logic in every block, a pattern is overkill. I was going for the more general "what's a succinct way of refactoring away if-else chains" type of question, which seemed to be what was implied. – Visionary Software Solutions Feb 24 '13 at 10:50

assylias · Accepted Answer · 2013-02-24T11:35:18.127

2

You could use a regex:

String s = currentFile.getName().toLowerCase();
if (s.matches("^.+?\\.(txt|pdf|doc|docx|html|htm|xml|djvu|rar|rtf)$")) {
    queue.put(currentFile);
}

That assumes that the action to be taken is the same for all extensions.

In details:

^         beginning of string
.+        one or more characters
?         non greedy -> don't consume characters that match the rest of the regex
\\.       a period
(pdf|doc) match pdf or doc
$         the end of the string

edited Feb 24 '13 at 11:35

answered Feb 24 '13 at 10:39

assylias

321,522
82
660
783

+1 for me, you may want to flag for case insensitive match. – guido Feb 24 '13 at 10:46
@guido the string is in lower case already so it should be fine (unless I misunderstand your comment). – assylias Feb 24 '13 at 10:48
1

I meant to not call toLowerCase() and match case insensitive; in general it means worse execution time but you might want to capture the file name or part of it, without loosing case in the same match. anyway I was going offtopic probably. – guido Feb 24 '13 at 10:52
@assylias `The left-hand side of an assignment must be a variable` - warnings – catch23 Feb 24 '13 at 11:08
@nazar_art On what line? I don't get that warning so it is probably due to some other code (maybe you already use `s` somewhere else). – assylias Feb 24 '13 at 11:10
@assylias All work perfect, it was my mistake. – catch23 Feb 24 '13 at 11:23
@assylias Can you recoment some good tutorial about `regex`? – catch23 Feb 24 '13 at 11:24
@nazar_art A good start is the [regex tag wiki](http://stackoverflow.com/tags/regex/info). – assylias Feb 24 '13 at 11:26
@nazar_art In particular this link: http://www.regular-expressions.info/java.html – assylias Feb 24 '13 at 11:31

score 2 · Answer 4 · answered Feb 24 '13 at 10:40

I would create a getExtension() method, which returns the extension of the file, and a final set of accepted extensions:

private static final Set<String> ACCEPTED_EXTENSIONS = 
    Collections.unmodifiableSet(new HashSet<String>(Arrays.asList(".txt", ".doc", ...));

private String getExtension(File f) {
    // TODO return the extension of the file
}

The code would then be redueced to:

private void findFiles(String path) {

    try {
        File root = new File(path);
        File[] list = root.listFiles();
        for (File currentFile : list) {
            if (currentFile.isDirectory()) {
                findFiles(currentFile.getAbsolutePath());
            } 
            else if (ACCEPTED_EXTENSIONS.contains(getExtension(currentFile))) {
                queue.put(currentFile);
            }
        }
    } 
    catch (InterruptedException e) {
        e.printStackTrace();
    }

Or even better, I would create a FileFilter which only accepts directories and files with one of the accepted extensions (using the same set and getExtension() method), and would use root.listFiles(fileFilter).

I understand, but getExtension() could be tricky for something like `.tar.gz` — guido, Feb 24 '13 at 11:02
how better return extention from `private String getExtension(File f) { }` — catch23, Feb 24 '13 at 11:13
Find the first dot in the String, and return everything from this index to the end of the String. Without caring about file without an extension, it's as simple as `file.getName().substring(file.getName().indexOf('.')).toLowerCase()`. — JB Nizet, Feb 24 '13 at 14:48

score 1 · Answer 5 · answered Feb 24 '13 at 10:41

Create a method

public boolean isPermissibleFileType(String fileName){
    String[] fileTypes = {".pdf",".doc",".docx",".html",".htm",".xml",".djvu",".djv",".rar",".rtf"};
    return Arrays.asList(fileTypes).contains(fileName.substring(fileName.lastIndexOf('.')).toLowerCase());
}

Use Method in Loop

private void findFiles(String path) {

        try {
            File root = new File(path);
            File[] list = root.listFiles();
            for (File currentFile : list) {
                if (currentFile.isDirectory()) {
                    findFiles(currentFile.getAbsolutePath());
                } else {
                    if(isPermissibleFileType(currentFile.getName()){
                       queue.put(currentFile);
                    }
                }
            }
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

    }
}

you need to extract the extensions from currentFile – guido Feb 24 '13 at 10:42 — guido, Feb 24 '13 at 10:42
@guido Good catch. I updated. – Kevin Bowersox Feb 24 '13 at 10:45 — Kevin Bowersox, Feb 24 '13 at 10:45
@Kevin Bowersox A little bit long run but work correctly. – catch23 Feb 24 '13 at 11:18 — catch23, Feb 24 '13 at 11:18

score 0 · Answer 6 · answered Feb 24 '13 at 10:43

0

You can extract the extension checks into some helper method using class FileNameFilter. And then for recursion you can use your original finder method.

answered Feb 24 '13 at 10:43

Jiri Kremser

12,471
7
45
72

Refactoring else-if operators with different extensions?

6 Answers6