4
\$\begingroup\$

I am creating a simple Mp3 Files Arranger the Project is on Github

Questions:

  • How can I optimize this app?
  • Am I violating any OOP principle?
  • Is it readable?
  • How can I make it better?

GUI.java

package Mp3Arranger;

import static Mp3Arranger.Util.getSystemYear;
import static Mp3Arranger.Util.spitError;
import com.mpatric.mp3agic.ID3v2;
import com.mpatric.mp3agic.InvalidDataException;
import com.mpatric.mp3agic.Mp3File;
import com.mpatric.mp3agic.UnsupportedTagException;
import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Component;
import java.awt.Dimension;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;
import java.io.IOException;
import java.net.URL;
import java.util.ResourceBundle;
import javax.swing.*;

// Referenced classes of package Mp3Arranger:
//            Info, Actions
public class GUI extends JPanel implements ActionListener {

private int SCREEN_HEIGHT;
private int SCREEN_WIDTH;

JPanel pane, buttonsPane, bottomPane;
JTextField path;
JButton browse;
JFrame gui;
JButton go;
JFileChooser folder;
JLabel credit;
JProgressBar wait = new JProgressBar();
JComboBox<String> choice;
String[] items = {"Sort By", "By Artist", "By Album", "By Genre"};

public GUI() {

    super(new BorderLayout());
    initComponents();

    pane.add(path);
    super.add(pane, BorderLayout.PAGE_START);

    browse.setMnemonic('b');
    browse.addActionListener(this);

    choice.setEditable(false);
    choice.addActionListener(this);

    go.setMnemonic('g');

    buttonsPane.add(browse);
    buttonsPane.add(choice);
    buttonsPane.add(go);

    super.add(buttonsPane, BorderLayout.CENTER);

    go.addActionListener(this);

    folder.setFileSelectionMode(JFileChooser.DIRECTORIES_ONLY);
    folder.setMultiSelectionEnabled(false);

    wait.setVisible(false);

    bottomPane.add(wait);

    credit = new JLabel("Copyright to Aamir khan " + getSystemYear());
    bottomPane.add(credit);
    super.add(bottomPane, BorderLayout.SOUTH);

    Dimension pathPreferredSize = wait.getPreferredSize();
    pathPreferredSize.width = 250;
    wait.setPreferredSize(pathPreferredSize);
   //        Change the look and feel to Nimbus
    try {
        UIManager.setLookAndFeel(new javax.swing.plaf.nimbus.NimbusLookAndFeel());
    } catch (UnsupportedLookAndFeelException ex) {
        spitError(ex.toString());
    }

}

@Override
public void actionPerformed(ActionEvent e) {

    if (e.getSource() == browse) {

        int val = folder.showDialog(GUI.this, "Select");
        if (val == 0) {
            String source = folder.getSelectedFile().getPath();
            Info.setPath(source);
            path.setText(source);
        }

    }
    if (e.getSource() == choice) {
        String sortBy = choice.getSelectedItem().toString();
        if (sortBy.equals("Sort By")) {
            JOptionPane.showMessageDialog(null, "Choose a Valid Sort to Arrange Your Files", "Opps!", 0);
        } else {
            Info.setSortBy(sortBy);
        }
    }
    if (e.getSource() == go) {

        final URL ERROR_IMG = GUI.class.getResource("Img/error_go.png");
        final ImageIcon icon = new ImageIcon(ERROR_IMG);

        JLabel errorMsgLabel = new JLabel("<html><body><b>"
                + "No Mp3 Found "
                + "Please Reselect the folder containing .Mp3 Files"
                + "</b></body></html>");
        errorMsgLabel.setIcon(icon);
        errorMsgLabel.setForeground(Color.darkGray);

        File mp3Files[] = Actions.findMp3Files(path.getText());
        if (mp3Files.length == 0) {
            JOptionPane.showMessageDialog(path, errorMsgLabel, "Oo!", JOptionPane.INFORMATION_MESSAGE);
        } else if (Info.getSortBy() == null) {
            JOptionPane.showMessageDialog(null, "Please Select a Sort type to Proceed", "Error!", JOptionPane.INFORMATION_MESSAGE);
        } else {
            Info.setMp3(mp3Files);
            disableButtons(true);
            Info.setPath(path.getText());
            runTask();
        }
    }
}

void disableButtons(boolean b) {
    //No need to check for buttons since buttonsPane contains only Buttons
    for (Component btn : buttonsPane.getComponents()) {
        btn.setEnabled(!b);
    }
}

private void runTask() {
    SwingWorker<Void, Void> task;
    task = new SwingWorker<Void, Void>() {
        private String tag;
        private final Actions dataHandler = new Actions();

        @Override
        protected Void doInBackground() throws Exception {
            wait.setVisible(true);
            wait.setMinimum(0);
            wait.setMaximum(Info.getMp3().length);
            wait.setStringPainted(true);

            for (File mp3 : Info.getMp3()) {
                try {
                    Mp3File song = new Mp3File(mp3.getAbsolutePath());

                    if (song.hasId3v2Tag()) {
                        ID3v2 idv2 = song.getId3v2Tag();

                        switch (Info.getSortBy()) {

                            case "By Artist":
                                tag = idv2.getArtist();
                                tag = (tag == null) ? "Unknown Artist" : tag;
                                processMp3(mp3, tag);
                                break;

                            case "By Album":
                                tag = idv2.getAlbum();
                                tag = (tag == null) ? "Unknown Album" : tag;
                                processMp3(mp3, tag);
                                break;

                            case "By Genre":
                                tag = idv2.getGenreDescription();
                                tag = (tag == null) ? "Unknown Genere" : tag;
                                processMp3(mp3, tag);
                                break;

                            default:
                                spitError("Unknow Sort");
                        }

                    } else {
                        processMp3(mp3, "Un-Defined Tag");
                    }

                } catch (IOException | UnsupportedTagException | InvalidDataException ex) {
                    JOptionPane.showMessageDialog(null, ex.getMessage());
                }

            }

            JLabel taskdoneMsg = new JLabel();
            final ImageIcon EMO_ICON = new ImageIcon(GUI.class.getResource("Img/emoticon_smile.png"));
            taskdoneMsg.setIcon(EMO_ICON);
            taskdoneMsg.setText("<html><body><h3 style = color:Green;>Task Completed Successfully </h3></body></html>");

            JOptionPane.showMessageDialog(null, taskdoneMsg, "Done", JOptionPane.PLAIN_MESSAGE);

            return null;
        }

        @Override
        protected void done() {
            Info.setMp3(null);
            wait.setVisible(false);
            disableButtons(false);
        }

        private void processMp3(File mp3, String tag) throws IOException {
            final File destinationFolder = new File(Info.getPath() + File.separator + tag);
            if (!destinationFolder.exists()) {
                destinationFolder.mkdirs();
            }
            int tracker = dataHandler.CopyData(mp3.getAbsolutePath(), destinationFolder.toString());
            wait.setValue(tracker);
        }

    };

    task.execute();
}
//runTask end here

void showGUI() {
    Toolkit tk = Toolkit.getDefaultToolkit();
    Dimension screen = tk.getScreenSize();
    SCREEN_HEIGHT = screen.height / 4;
    SCREEN_WIDTH = screen.width / 4;

    gui.setSize(300, 130);
    gui.setLocation(SCREEN_WIDTH + gui.getWidth() / 2, SCREEN_HEIGHT + gui.getHeight() / 2);
    gui.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

    JComponent newContentPane = new GUI();
    newContentPane.setOpaque(true);

    gui.setContentPane(newContentPane);
    gui.setResizable(false);
    gui.setVisible(true);

    SwingUtilities.updateComponentTreeUI(gui);

    URL favicon = GUI.class.getResource(ResourceBundle.getBundle("Mp3Arranger/config/Bundle").getString("IMG/CONTROL_EQUALIZER_BLUE.PNG"));
    ImageIcon icon = new ImageIcon(favicon);
    gui.setIconImage(icon.getImage());
}

private void initComponents() {
    this.gui = new JFrame("Mp3 Arranger");
    this.go = new JButton(ResourceBundle.getBundle("Mp3Arranger/config/Bundle").getString("GO"));
    this.browse = new JButton(ResourceBundle.getBundle("Mp3Arranger/config/Bundle").getString("BROWSE.."));
    this.choice = new JComboBox<>(items);
    this.buttonsPane = new JPanel();
    this.path = new JTextField(System.getProperty("user.home") + File.separatorChar + "Music", 20);
    this.pane = new JPanel();
    this.bottomPane = new JPanel();
    this.folder = new JFileChooser();
}

public static void main(String args[]) {
    SwingUtilities.invokeLater(new Runnable() {

        @Override
        public void run() {
            new GUI().showGUI();
        }
    });

}

}

Actions.java

package Mp3Arranger;
import static Mp3Arranger.Util.spit;
import static Mp3Arranger.Util.spitError;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.FilenameFilter;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;

/**
  *
   * @author toffe boy Aamir
 */
public class Actions {

private int count = 0;


public static File[] findMp3Files(String folderPath) {
    File folder = new File(folderPath);
    if (folder.isDirectory()) {
        File[] mp3files = folder.listFiles(new FilenameFilter() {
            @Override
            public boolean accept(File dir, String name) {
                return name.toLowerCase().endsWith(".mp3");
            }
        });
        return mp3files;
    }
    return null;
}

@SuppressWarnings("null")
public int CopyData(String song, String destination) throws IOException {

    final long START_TIME = System.currentTimeMillis();

    InputStream istream = null;
    OutputStream ostream = null;
    try {
        File afile = new File(song);
        File bfile = new File(destination + File.separator + afile.getName());
        istream = new FileInputStream(afile);
        ostream = new FileOutputStream(bfile);
        byte[] cfile = new byte[1024];

        while ((istream.read(cfile)) > 0) {
            ostream.write(cfile);
            ostream.flush();
        }
        istream.close();
        ostream.close();

        spit("Name : " + bfile.getName());
        spit(afile.getName() + " Copied into: " + bfile.getParent());

        float fsize = bfile.length() / (1024 * 1024f);
        spit("Total size : " + fsize + " MB");

        //Delete the file after copying
        afile.delete();

        final long END_TIME = System.currentTimeMillis();

        spit("Copied Successs full");
        spit("Copied in " + ((END_TIME - START_TIME) / 1000) + " seconds");

    } catch (IOException ex) {
        spitError(ex.getMessage());
    }
    return count++;}}

Info.java

package Mp3Arranger;
import java.io.File;

/**
*
* @author toffe boy Aamir
*/
public class Info {

private static String sortBy;
private static File[] mp3;
private static String TruePath;

public static void setSortBy(String sortBy) {Info.sortBy = sortBy;}

public static String getSortBy() {return Info.sortBy;}

public static void setMp3(File[] mp3) {Info.mp3 = mp3;}

public static File[] getMp3() {return mp3;}

public static String getPath() {return TruePath;}

public static void setPath(String TruePath) {Info.TruePath = TruePath;}
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ On github the code is rightly indented, here not; maybe you missed something when pasting? \$\endgroup\$ Aug 1, 2016 at 8:37

1 Answer 1

2
\$\begingroup\$

Conventions:

  • Package names are all lowercase, with consecutive words simply concatenated together (no underscores). For example, com.example.deepspace, not com.example.deepSpace or com.example.deep_space.
  • Local variable names are written in lowerCamelCase.

    For example mp3files should be mp3Files.

  • The square brackets form a part of the type, not the variable: String[] args, not String args[].

    For example use File[] files not File files[].

  • Braces are used where optional

    For example

    if (mp3Files.length == 0) {
        JOptionPane.showMessageDialog(path, errorMsgLabel, "Oo!", JOptionPane.INFORMATION_MESSAGE);
    } else if (Info.getSortBy() == null) {
        JOptionPane.showMessageDialog(null, "Please Select a Sort type to Proceed", "Error!", JOptionPane.INFORMATION_MESSAGE);
    } else {
    

    is far more readable with just one { } more.

    if (mp3Files.length == 0) {
        JOptionPane.showMessageDialog(path, errorMsgLabel, "Oo!", JOptionPane.INFORMATION_MESSAGE);
    } else {
        if (Info.getSortBy() == null) {
            JOptionPane.showMessageDialog(null, "Please Select a Sort type to Proceed", "Error!", JOptionPane.INFORMATION_MESSAGE);
        } else { ...
      }
    }
    

Java 8

  • Use lambda when that simplifies the code:

    File[] mp3files = folder.listFiles(new FilenameFilter() {
        @Override
        public boolean accept(File dir, String name) {
            return name.toLowerCase().endsWith(".mp3");
        }
    });
    

    will become

    File[] mp3files = folder.listFiles((dir, name) -> name.toLowerCase().endsWith(".mp3"));
    

    In this case the advice is to use java.nio

  • Avoid null as return value, use Optional when null is possible.

    What if here

    File mp3Files[] = Actions.findMp3Files(path.getText());
    if (mp3Files.length == 0) {
    

    mp3Files is null? findMp3Files should be return an Optional<File[]> and handle the case when the value is not present.

Tips:

  • Avoid using static class containing static information like the class Info. Those information are instance information that should be injected to the classes that need them. When using multi-thread statics values are always a nightmare.

  • Create an Enum for "Sort By". The enum itself could have the method to extract the tag. And thus you can avoid the default. Usin an enum the switch could be rewritten as tag = Info.getSortBy().getTag(idv2).

  • Avoid duplication and avoid multiple endings: in doInBackground you could write just once processMp3 just assign the correct value to tag and write it in the end of the method. Thus you will know what is the last line executed.

    Simple example:

    case(1):
      perform("a");
      break;
    case(2):
      perform("b");
      break;
      ...
    

    Should be rewritten as:

    int parameter = 0;
    ...
      case(1):
        parameter = "a";
        break;
      case(2):
        parameter = "b";
        break;
    perform(parameter);
    
\$\endgroup\$
3
  • \$\begingroup\$ thx for the advice, yes i can make things easier using lambda and other java 8 goodies but for that i have separate project where i am using javafx 8 and java8 but for this one i want it to compatible Java 1.7 \$\endgroup\$
    – Mak
    Aug 1, 2016 at 11:54
  • \$\begingroup\$ fine, but use java.nio (jdk 7) whenever possible. \$\endgroup\$ Aug 1, 2016 at 11:57
  • \$\begingroup\$ sure i will, btw can you elaborate your last point i didn't get it...an example will be more helpfull \$\endgroup\$
    – Mak
    Aug 1, 2016 at 11:59

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.