current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

Are the catch statements ok or must I put in a println()? In general, is the code good-looking enough?

    public void music(String song) {         
        try {
    File soundFile = new File(song);
    AudioInputStream sound = AudioSystem.getAudioInputStream(soundFile);

    // load the sound into memory (a Clip)
    DataLine.Info info = new DataLine.Info(Clip.class, sound.getFormat());
    Clip clip = (Clip) AudioSystem.getLine(info);
    clip.open(sound);

    // play the sound clip
    clip.start();
    }
    catch (IOException e) {
    }
    catch (LineUnavailableException e) {
    }
    catch (UnsupportedAudioFileException e) {
    }
}
share|improve this question

3 Answers 3

up vote 10 down vote accepted

To be clear, you are not doing any exception handling. You are exception ostriching... putting your head in the sand and hoping the exceptions don't happen, or go away.

@tim has the right suggestions but has not gone quite far enough.

Yes, you should report exceptions for debugging, and to inform the user, but, you can do it a whole lot better than what you have. Java supports what's called 'multi-catch', which can simplify your code to:

public void music(String song) {         
    try {
        File soundFile = new File(song);
        AudioInputStream sound = AudioSystem.getAudioInputStream(soundFile);

        // load the sound into memory (a Clip)
        DataLine.Info info = new DataLine.Info(Clip.class, sound.getFormat());
        Clip clip = (Clip) AudioSystem.getLine(info);
        clip.open(sound);

        // play the sound clip
        clip.start();
    } catch (IOException
           | LineUnavailableException
           | UnsupportedAudioFileException e) {
        Logger.error("Unable to read clip. See exception", e);
    }
}

Note that your code has other problems as well, though. You are creating multiple instances of classes that have resources that should be closed:

  • AudioInputStream is a resource that should be closed, but you don't close it.
  • Clip also should be closed.

As a result of your poor handling of those resources you could end up in a situation where resources are 'hanging around' longer than they are needed, leading to poor memory use, and other complications. Your system unfortunately delegates-and-forgets about those resources, so fixing the situation is not easily possible with your current code.

share|improve this answer
up vote 2 down vote

Style: Formatting: Indent after opening { and unindent after closing } for better readability and by convention.

share|improve this answer
    
what about the catch? Must I put in System.out.println(); so I can know which one it is? Or no? –  Highlights Factory Feb 9 at 11:01
1  
Catching an Exception and not acting on it is very bad practice, as the Exceptions will go unnoticed this way. –  barq Feb 9 at 12:11
up vote 8 down vote

Must I put in System.out.println(); so I can know which one it is?

That would be one approach, and it would certainly be better than your current approach, which is silently failing. In case something goes wrong, this will make it very difficult to find out the problem.

If you get an exception, you should first try to think of recovering from it right then and there.

If that is not possible, I would just trow it upwards, and let the calling method handle it. In your case for example by playing a default song, or skipping to the next song, etc.

If all else fails, you should report a helpful and detailed error message to the end user. For debugging purposes, you should also print the exact exception - including stacktrace - to the console.

Just swallowing the exception is pretty much never the correct approach.

Misc

  • // play the sound clip isn't really a helpful comment, it's pretty obvious from the code already
  • If you handle all exceptions the same way anyways (in your case ignoring them), you can just catch the generic Exception.
  • as barq said, your indentation is partly off.
  • music is a little generic as a method name. maybe playSong, or playClip
share|improve this answer
1  
never ever catch the generic Exception - use either multicatch or generic superclass, but never Exception! - some hard-to-imagine errors are subclassing Exception, not Error - catching them by too wide catch definition will turn your debugging into hell, as you won't be able to handle them properly (not even knowing what they are about probably). You should only catch exceptions if you can properly handle them, says the wisdom of the community supported by Joshua Bloch and other. Apart from that, a good answer. –  vaxquis Feb 9 at 15:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.