A simple MP3 file arranger

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 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);

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 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);

StackExchange Code Review Q#136512, answer score: 2