AudioMetaTagger: recursively read files and gather metadata

Question

So the idea is to read metadata recursively in a chosen directory, load their metadata and display it into a table.

Here is the simplified example of what I am doing.

public class AudioMetadataExtractorTest {

    public static void main(String[] args) {
       String audioFiles = "C:\\Music";

       File dir = new File(audioFiles);
       Collection col = checkData(dir);

       for(Object object : col){
        File fil = (File) object;
        System.out.println(fil.getAbsolutePath());
        try {
            printMetaData(fil);
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
       System.out.println(col.size());
   }

In this case I am using the apachi.commons library to gather all files.

    public static Collection checkData(File dir){
    Collection files = FileUtils.listFiles(
              dir, 
              new RegexFileFilter("^(.*mp3)"), 
              DirectoryFileFilter.DIRECTORY
            );
    return files;
}

And with jaudiotagger read and print the metadata.

    public static void printMetaData(File file) throws Exception {
    AudioFile audioFile = null;
    audioFile = AudioFileIO.read(file);
    Tag tag = audioFile.getTag();
    for (FieldKey key : FieldKey.values()) {
        if(key==FieldKey.ALBUM){
        System.out.println(key + " " + tag.getFirst(key));
        }
    }
}

While this code works, it is extremely slow. I am confident there is a more efficient way of doing this.

• Is it bad practice to loop over objects and cast them within the loop?
• Is the jaudiotagger library an intrinsically slow library?

Answer 1

welcome to code review! Here are some thoughts:

Casting

To get rid of the casting you can use generics (see also: https://docs.oracle.com/javase/tutorial/java/generics/types.html) like so:

public static Collection<File> checkData(File dir){
    Collection<File> files = FileUtils.listFiles(
        dir, 
        new RegexFileFilter("^(.*mp3)"), 
            DirectoryFileFilter.DIRECTORY
        );
    return files;
}

This way in your main function you can avoid the casting:

 Collection<File> files = checkData(dir);
 for(File file: files) {

This is to be preferred, as casting is to be avoided when possible (Interesting related discussion: https://stackoverflow.com/questions/4167304/why-should-casting-be-avoided).

Code

Why initialize a variable first and then assign the value? You can easily do it in one line, which is nicer to read:

AudioFile audioFile = AudioFileIO.read(file);

Formatting

On the nitpicky side, you should take a look at how code formatting is usually done in java: https://docs.oracle.com/javase/tutorial/essential/io/formatting.html

Using decent indentation and spacing allows for easier reading of the code. The best advice here is probably using the auto-formatter of your IDE (eclipse, intelliJ, etc.), as it is tedious to do it by hand.

Performance

Judging from your code, I don't see any specific place to make it a lot faster using the library. It looks like the library reads the entire file to get the metaData, which is of course very I/O intense, and therefor slow. More advanced libraries like JAudioTagger (http://www.jthink.net/jaudiotagger/examples_read.jsp) might be able to do this in a more efficient way.

Hope this helps!

Answer 2

A few minor things in addition to @Nihathrael's good points.

---

This loop is pointless:

for (FieldKey key : FieldKey.values()) {
    if(key==FieldKey.ALBUM){
    System.out.println(key + " " + tag.getFirst(key));
    }
}

This is exactly the same but simpler, and without unnecessary looping:

FieldKey key = FieldKey.ALBUM;
System.out.println(key + " " + tag.getFirst(key));

---

In this code:

Collection col = checkData(dir);

Both the variable name and the method are poorly named. This would be better:

Collection<File> mp3Files = findMp3Files(dir);

Especially as your code gets longer, good names will help a lot with readability.

---

You catch generic Exception and declare to throw generic Exception in many places. Use the most specific exception type possible. This can make your code less buggy by not catching unintentionally a type of Exception that you didn't expect to happen. It also makes the code more readable, as the reviewer can see what can possibly go wrong.

Answer 3

Re checkData(...): I find Java 7+'s java.nio.file.SimpleFileVisitor described in The Java™ Tutorials, Walking the File Tree extremely helpful. Thus avoiding a dependency to an extra library, Apache Commons, as long as no other things of it are used anyway.