1
\$\begingroup\$

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?
\$\endgroup\$
1
\$\begingroup\$

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!

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the well documented answer. (+1) I will take a look at the casting reference for a deeper understanding of it. As for the formatting I must practice that habit to notice the subtle differences. \$\endgroup\$ – Scavence Nov 28 '14 at 14:20
  • \$\begingroup\$ @Scavence I had another look at some libraries for extracting meta data from audio files and found this one: jthink.net/jaudiotagger/examples_read.jsp Looks like it could do what you need (haven't tested it though). \$\endgroup\$ – Nihathrael Nov 28 '14 at 17:01
  • \$\begingroup\$ tag.getFirst(FieldKey.TITLE) works more code efficient and spares me the loop, which is always good. But unfortunately performance-wise still (quite) the same. \$\endgroup\$ – Scavence Nov 28 '14 at 18:14
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This loop is pointless: Figured the tag.getfirst() method was more appropriate, but thanks for pointing that out. About the exceptions: multicatch when needed, use a logger, and throw user-relevant exceptions to main frame to be revealed to the user is sufficient? howtodoinjava.com/2013/04/04/… \$\endgroup\$ – Scavence Nov 28 '14 at 20:02
  • 1
    \$\begingroup\$ That's a pretty nice article. If you follow everything in there, that should be pretty good! \$\endgroup\$ – janos Nov 28 '14 at 20:07
1
\$\begingroup\$

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.

\$\endgroup\$

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.