3
\$\begingroup\$

I am rather new to coding, and I would like to see what improvements should I make to the code I wrote to ensure that I am using good practices, and that it will function as it's supposed to. The following code detects the operating system (I have it set for either Linux or Windows), detects the folders around it, detects the files around it, then moves the files into folders based on the similarity between the file and folder names.

import java.io.BufferedReader;
import java.io.InputStreamReader;

public class Organizer {
public static void main(String[] args) throws Exception{ //This part organizes the files

    String p1 = null;
    String p2 = null;
    if ((confstring()).equalsIgnoreCase("linux")){
        p1 = "bash";
        p2 = "-c";

    } else if ((confstring()).equalsIgnoreCase("windo")){
        p1 = "CMD";
        p2 = "/C";          
    } 
String newname = null ;
String[] confcmd = configure() ;//Retrieves the commands from the configure() function
String[] folder = execute(confcmd[0], p1,p2);//Executes the first command and gets a list of folders
String[] interact = new String[folder.length] ;

    for (int n=0 ; n < folder.length; n++){
        interact[n] = folder[n].substring(0,3);//Makes the keyword to compare the filenames to
        //System.out.println(interact[n]);
    }
String[] stdoutput = execute(confcmd[1],p1,p2);

    if ((confstring()).equalsIgnoreCase("linux")){//both of the following if statements get the filenames, compares them to the keyword, then moves and renames the files
        for ( int n=0 ; n < interact.length; n++){
            for(int i=0 ; i<stdoutput.length; i++){
                if((stdoutput[i]).contains(interact[n])){
                    //System.out.println(stdoutput[i]);
                    newname = confcmd[2] + " " + stdoutput[i] + " " + folder[n] +confcmd[3] + stdoutput[i] + ")" + "-" +stdoutput[i]; //The ")" results from a shell scripting quirk I had to address
                    //System.out.println(newname);
                    execute(newname, p1, p2);
                }
            }
        }
    } else if((confstring()).equalsIgnoreCase("windo")){
        for ( int n=0 ; n < interact.length; n++){
            for(int i=0 ; i<stdoutput.length; i++){
                if((stdoutput[i]).contains(interact[n])){
                    String[] date = execute(confcmd[3]+" "+stdoutput[i],p1,p2);
                    date[0] = date[0].substring(0,10);
                    date[0] = date[0].replace("/","-");
                    newname = confcmd[2] + " " + stdoutput[i] + " " + folder[n] + "\\" + date[0] + "-" + stdoutput[i];
                    System.out.println(newname);
                    execute(newname,p1,p2);//
                } 
            }
        }
    }
}
private static String[] configure(){//Sets the OS and the commands needed to organize the files
    String[] commands = null;
    String os = System.getProperty("os.name");
    os = os.substring(0, 5) ;
    if ((os).equalsIgnoreCase("linux")){
        commands = new String[4] ;
        commands[0] = "ls -d */" ;
        commands[1] = "find . -maxdepth 1 -type f -printf '%f\n'";
        commands[2] = "mv" ; 
        commands[3] = "$(date +%d-%m-%y -r " ;
    } else if ((os).equalsIgnoreCase("windo")){
        commands = new String[4];
        commands[0] = "DIR /B /AD" ;
        commands[1] = "DIR /B /A-D-H";
        commands[2] = "MOVE" ;
        commands[3] = "DIR /B /T:C";
    } else if ((os).equalsIgnoreCase("mac")){

    }
return  commands ; 
}
private static String confstring(){
    String os = System.getProperty("os.name");
    os = os.substring(0, 5) ;
    //System.out.println(os);
    return os;
}
private static String[] execute(String cmd, String p1, String p2) throws Exception{//Runs the commands
    int term = 0;
    String[] intermediate = new String[100];
    ProcessBuilder builder = new ProcessBuilder(p1, p2,  cmd);
        builder.redirectErrorStream(true);
        Process p = builder.start();
        BufferedReader r = new BufferedReader(new InputStreamReader(p.getInputStream()));
        String line;
        for (int n=0; true ; n++ ) {
            line = r.readLine();
            if (line == null) { intermediate[n]="EXIT";break; }
            System.out.println(line);
            intermediate[n] = line ;
        }
        for (int i = 0 ; !(intermediate[i]).equals("EXIT"); i++){
            term = i ;
        }
        term ++;
        //System.out.println(term);
        String[] output = new String[term];
        for (int k=0 ; k < term; k++){
            output[k]=intermediate[k];
        }
        return output; //output is used to organize files

}
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Some suggestions:

  • Don't wrap shell commands in Java unless you absolutely have to. Java supports listing files, moving them, getting dates etc. natively. As an added bonus you'll instantly support basically any platform without writing any platform specific code, and you won't have to do a bunch of string parsing to get the result in a usable format.
  • Format your code with a linter or IDE. It will make it much easier to read for others.
  • Short variable names (p1, confcmd, etc.) make your code harder to read. Programming is a brain problem, not a typing problem, so you should optimise for understanding rather than brevity.
  • Anything in your main function is basically not reusable in other Java code. If you can think of any use case of importing your code into another file, you should create a separate method for that functionality avoiding dependencies on the surrounding shell (such as arguments).
  • Inline comments are a code smell. Everywhere you feel the need for an inline comment you should ask yourself if you could refactor the code in such a way that the comment is unnecessary.
\$\endgroup\$
0
\$\begingroup\$

In addition to the points mentioned in @l0b0's answer...

Getting the OS

It's somewhat weird that you only get the first five letters from System.getProperty("os.name") to determine if it's Linux or Windo [sic]. Also, instead of repeatedly calling confstring() (which doesn't really explain what it is doing, perhaps call it getOSName()?), you should consider putting it into a static final variable, so that it can be referenced easily multiple times:

public class Organizer {

    private static final String OS_NAME = System.getProperty("os.name").toUpperCase();

    public static void main(String[] args) {
        String command = OS_NAME.startsWith("WINDOWS") ? "CMD" : "bash";
        String commandArg = OS_NAME.startsWith("WINDOWS") ? "/C" : "/c";
        // ...
    }
}

Actually, an enum-driven approach might be worth considering as well:

enum OS {
    LINUX("bash", "-c"),
    WINDOWS("CMD", "/C");

    public static final OS CURRENT = System.getProperty("os.name")
                                        .toUpperCase().startsWith("WINDOWS") 
                                            ? WINDOWS : LINUX;

    private final String command;
    private final String commandArg;

    private OS(String command, String commandArg) {
        this.command = command;
        this.commandArg = commandArg;
    }

    // for illustration, you should be using getters for command and commandArg
    public String toString() {
        return command + " " + commandArg;
    }
}

With the example above, calling System.out.println(OS.CURRENT) will print "CMD /C" on a Windows OS. An enum is also quite suitable in this case, where we have a pre-defined set of possible values to specify for. For example, if you want to use a different shell for Mac OS X (or macOS), you can do so by adding a new value and checking for it when deriving the value for OS.CURRENT.

\$\endgroup\$

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.