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
}
}
h.j.k.
19.3k3 gold badges37 silver badges93 bronze badges
asked Jun 23, 2016 at 16:27
\$\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.
answered Jun 24, 2016 at 12:40
\$\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.

answered Jun 25, 2016 at 9:37
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.