0
\$\begingroup\$

I have been learning about refactoring and I have this method that I'm wondering if I should split into two for code cleanness. First, the method as one, next the method split in two:

Method not split:

public static ArrayList<String> getFilePathOrName(String pathOrName) {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    if(pathOrName.equalsIgnoreCase("path")) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.toString());
            }
        }
    } else if(pathOrName.equalsIgnoreCase("name")) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.getName());
            }
        }
    }
    return files;
}

Method split:

public static ArrayList<String> getAllUndistributedFilesNames() {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.getName());
        }
    }
    return files;
}

public static ArrayList<String> getAllUndistributedFilesPaths() {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    for (File file : listOfFiles) {
        if (file.isFile()) {
            files.add(file.toString());
        }
    }
    return files;
}

I feel like the method names when they are split is more intuitive. However, I'm not sure if that is enough justification to have this much duplicate code.

\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB May 13 at 9:17
  • \$\begingroup\$ There is another alternative: create methods to use with java.nio.file.Files.walk()/walkTree(). \$\endgroup\$ – greybeard May 13 at 10:56
3
\$\begingroup\$

When we refactor the aim is to reduce code duplication. but you introduced even more duplication.

The way to go is to look out for code that is similar and make it the same.

When looking at your original method the duplication is in the if/else cascade. This means you should deal with that. The obvious differences are:

  • the string literal used
  • the else before the second if
  • the method called at the object file

Lets convert that in thee steps:

  1. the else is not needed for business reasons. It is introduced for performance optimization. You can safely remove this else and verify by measurement it that has really an impact. My guess is it does not. so lets let rid of it:

    if(pathOrName.equalsIgnoreCase("path")) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.toString());
            }
        }
    }
    if(pathOrName.equalsIgnoreCase("name")) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.getName());
            }
        }
    }
    
  2. now you can extract the first literal string into a variable:

    String requestedPathName = "path";
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.toString());
            }
        }
    }
    if(pathOrName.equalsIgnoreCase("name")) {
      // ...
    

    Then, after the first if block we can resuse this variable:

    String requestedPathName = "path";
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(file.toString());
            }
        }
    }
    requestedPathName = "name";
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
      // ...
    
  3. The last change requires to use an interface. This interface should declare a method that takes a file object as a parameter and returns a String object. One option is to define such interface yourself:

    @FunctionalInterface
    interface FileToStringConverter{
         String convert(File file);
    }
    

    Or we use the Function interface provides by the Java standard Lib. I choose the latter.

    The principle is the same as with the string literal before:

    replace the original code with the interface usage:

    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(new Function<File,String>(){ 
          @Override public String apply(File t){
               t.toString();
             }
          }.apply(file));
            }
        }
    }
    

    extract the interface implementation to a local variable:

    // interface implemented as Lambda and method reference.
    Function<File,String> converter = File::toString; // interface implemented as Lambda and method reference.
     if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(converter.apply(file)); 
            }
        }
    }
    

    repeat this in the other `if block:

    String requestedPathName = "path";
    Function<File,String> converter = File::toString;
      if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(converter.apply(file));
            }
        }
    }
    requestedPathName = "name";
    converter = File::getName;
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(converter.apply(file));
            }
        }
    }
    

Now both if blocks look exactly the same. You can select one of them and apply your IDEs extract method automated refactoring feature:

public static ArrayList<String> getFilePathOrName(String pathOrName) {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

        String requestedPathName = "path";
        Function<File,String> converter = File::getName;
        // of cause you should choose a better name for the method!
        extracted(requestedPathName, pathOrName, converter, listOfFiles, files); 
        requestedPathName = "name";
        converter = File::getName;
        extracted(requestedPathName, pathOrName, converter, listOfFiles, files);

    return files;
}

private void extracted(
           String requestedPathName, 
           String pathOrName, 
           Function<File,String> converter,
           File[] listOfFiles, 
           List<File> files) { 
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(converter.apply(file));
            }
        }
    }
}

last step is inlineing the variables:

     extracted("path", pathOrName, File::toString, listOfFiles, files);
     extracted("name", pathOrName, File::getName, listOfFiles, files);

The complete result (with a better name for the extracted method) is:

public static ArrayList<String> getFilePathOrName(String pathOrName) {
    ArrayList<String> files = new ArrayList<>();
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();

    addPathOrNameToList("path", pathOrName, File::toString, listOfFiles, files);
    addPathOrNameToList("name", pathOrName, File::getName, listOfFiles, files);       

    return files;
}

private void addPathOrNameToList(
           String requestedPathName, 
           String pathOrName, 
           Function<File,String> fileToStringConversion,
           File[] listOfFiles, 
           List<File> files) { 
    if(pathOrName.equalsIgnoreCase(requestedPathName)) {
        for (File file : listOfFiles) {
            if (file.isFile()) {
                files.add(fileToStringConversion.apply(file));
            }
        }
    }
}

We could turn the repeated method call into a loop if we would introduce another interface or an enum to encapsulate the differing data.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your elaborate response! If I understand correctly, you are first creating a new method to get rid of the duplicate if-statement, and then creating a new method to get rid of the double for-statement. What I am wondering - and please bear in mind I am very much a beginner programmer - is how this refactoring makes the code more readable and efficient, which I thought are main functions of refactoring, since to me this seems a lot more complex to understand than my initials code. \$\endgroup\$ – Dmrenger May 13 at 20:14
  • \$\begingroup\$ @Dmrenger "since to me this seems a lot more complex to understand than my initials code." --- Well, Then come back in 3 month... Your (original) solution does look more "readable" to you right now because it is your baby. You have thought about it a lot and it was the best solution you came up with. Any other solution is harder to understand because you need to make an effort to understand it while you already understood our own. In three month you will have forgotten about the details of your solution and so lost the context making it "easier" to understand... \$\endgroup\$ – Timothy Truckle May 14 at 9:42
  • \$\begingroup\$ Good point, I suppose that is true. Your code definitely makes it look a lot cleaner, so I will try to apply to those steps to more part of my code! Thanks again. \$\endgroup\$ – Dmrenger May 14 at 11:48
2
\$\begingroup\$

Some remarks from my end:

  • Folderpath should be a constant, because its easy to change if you use it multiple and it makes it more readable
  • FunctionArguments should be removed and instead create two methods with meaningful name, which do only one thing, like you did in your first refactoring :)
  • declare Interface instead of Classes especially by List, because then the caller can also use different implementations from List
  • if you use at least java 8 then you could use streams to handle simple straight forward things like filtering Lists or arrays

I would change it to this without any adding more complexicity to the main concern of your code.

private static final String FOLDER_PATH = "C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld";

public static List<String> getFilePaths(String path) {

    File folder = new File(FOLDER_PATH);
    File[] listOfFiles = folder.listFiles();

    return Arrays.stream(listOfFiles)
                 .filter(file -> file != null && file.isFile())
                 .map(file -> file.toString())
                 .collect(Collectors.toList());
}

public static List<String> getFileNames(String fileName) {

    File folder = new File(FOLDER_PATH);
    File[] listOfFiles = folder.listFiles();

    return Arrays.stream(listOfFiles)
                 .filter(file -> file != null && file.isFile())
                 .map(file -> file.getName())
                 .collect(Collectors.toList());

}

I hope this will help you :)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback! Good point about making the folder path a constant and using List instead of ArrayList. And thanks for confirming I was at least headed in the right direction. Question: your code looks functionally the same as mine. In what way exactly is it better? Could you elaborate for example why using streams are better than what I did? I use Java 8 :) \$\endgroup\$ – Dmrenger May 14 at 11:58
  • \$\begingroup\$ I just gave remarks regarding Code quality and not about functionality :) So on my end i think its more readable and maintainable. Regarding streams, i think if you just iterate through list with filtering and edititng value, then streams are more expressive and readable, but if you make complex things on elements that needs more then one operation for e.g. map() or filter(), i wouldn't use streams, because i dont like to debug them \$\endgroup\$ – Pepper Oni May 15 at 5:38
1
\$\begingroup\$

The only difference is the line that converts File objects to Strings. You should refactor the two methods into one method that accepts a Function and just pass it a lambda that performs the desired operation.

Also, File.toString() is definitely not what you want to call here. File.getPath() is probably the correct method.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

I'm agree with @TorbenPutkonen's answer that you can refactor your code using Function.

From your code:

ArrayList<String> files = new ArrayList<>();
if(pathOrName.equalsIgnoreCase("path")) { //adding strings to your list calling method toString }
else if(pathOrName.equalsIgnoreCase("name")) { //adding strings to your list calling method getName } }
return files;

Basically in your code you will return an empty list or if your String pathOrName is case insensitive equals to "path" or "name" you will return a new list of Strings using method toString or method getName.

You can create a Map<String, Function<File, String>> in this way:

Map<String, Function<File, String>> map = new HashMap<>();
map.put("path", (file) -> file.toString());
map.put("name", (file) -> file.getName());

Then you can return the new List created like below:

String lowercase = pathOrName.toLowerCase();
if (!map.containsKey(lowercase)) { return Collections.emptyList(); }
return Arrays.stream(listOfFiles)
        .filter(File::isFile)
        .map(file -> map.get(lowercase).apply(file))
        .collect(Collectors.toList());

The full code of the method is below:

public static List<String> getFilePathOrName(String pathOrName) {
    File folder = new File("C:\\Users\\damar\\OneDrive\\Documenten\\FolderTest\\Folderretouren\\Niet verdeeld");
    File[] listOfFiles = folder.listFiles();        
    Map<String, Function<File, String>> map = new HashMap<>();
    map.put("path", (file) -> file.toString());
    map.put("name", (file) -> file.getName());

    String lowercase = pathOrName.toLowerCase();
    if (!map.containsKey(lowercase)) { return Collections.emptyList(); }
    return Arrays.stream(listOfFiles)
            .filter(File::isFile)
            .map(file -> map.get(lowercase).apply(file))
            .collect(Collectors.toList());
    }

You could add a new parameter in your method passing the path of your file folder instead of instantiating it inside your method.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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