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.

I'm trying to apply sound today, and the goal in mind was a simple violin tuner. The actionListener seems repetitive/ How might I optimize it (what I tried just broke everything)? I also noticed the sounds bleed into each other. I don't mind that so much for the different keys (though I'm thinking of a toggle option for this) but it also is the case for the same key, which is undesirable. I'm not sure how to handle that.

/* Author: Luigi Vincent
A simple violin tuner, emits G, D, A, and E sounds to facilitate violin tuning
*/

import javax.swing.*;
import javax.sound.sampled.*;
import java.awt.*;
import java.awt.event.*;
import java.io.*;

public class ViolinTuner {
    public static void main(String[] args) {
        JFrame frame = new JFrame("Violin Tuner");
        // get and set icon for the program
        ImageIcon icon = new ImageIcon("Images/Icon.png");
        frame.setIconImage(icon.getImage());
        // Buttons
        final JButton G_KEY = new JButton("G");
        final JButton D_KEY = new JButton("D");
        final JButton A_KEY = new JButton("A");
        final JButton E_KEY = new JButton("E");
        // Listener to play sounds on click
        ActionListener violinSounds = new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                if (e.getSource() == G_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/G.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    }catch(Exception x) { x.printStackTrace(); }
                }
                else if (e.getSource() == D_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/D.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    }catch(Exception x) { x.printStackTrace(); }
                }
                else if (e.getSource() == A_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/A.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    }catch(Exception x) { x.printStackTrace(); }
                }
                else if (e.getSource() == E_KEY) {
                    try {
                        AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(new File("Sounds/E.wav").getAbsoluteFile());
                        Clip clip = AudioSystem.getClip();
                        clip.open(audioInputStream);
                        clip.start();
                    } catch(Exception x) { x.printStackTrace(); }
                }
            }
        };
        // Register buttons to listener
        G_KEY.addActionListener(violinSounds);
        D_KEY.addActionListener(violinSounds);
        A_KEY.addActionListener(violinSounds);
        E_KEY.addActionListener(violinSounds);
        // buttons to panel
        JPanel p = new JPanel();
        p.add(G_KEY);
        p.add(D_KEY);
        p.add(A_KEY);
        p.add(E_KEY);
        // Panel to frame
        frame.add(p);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setSize(225, 75);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }
};
share|improve this question

5 Answers 5

up vote 6 down vote accepted

Use MIDI

Why play a WAV file, when you could synthesize the tones using MIDI? The tones can be played with indefinite length. You wouldn't need to distribute supplementary files with the program. You can also easily modify the program to generate other pitches without having to obtain new WAV recordings.

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.GridLayout;
import javax.sound.midi.*;
import javax.swing.*;
import javax.swing.event.ChangeEvent;
import javax.swing.event.ChangeListener;

public class ViolinTuner {
    private static enum Note {
        G(55), D(62), A(69), E(76);

        public final int midiNoteNumber;

        Note(int midiNoteNumber) {
            this.midiNoteNumber = midiNoteNumber;
        }
    }

    // Orchestras tune to oboes.
    // http://music.stackexchange.com/q/22133/1941
    // http://www.midi.org/techspecs/gm1sound.php
    private static final int OBOE = 69;

    private final MidiChannel channel;
    private final AbstractButton[] buttons = new AbstractButton[Note.values().length];

    public ViolinTuner() throws MidiUnavailableException {
        this.channel = initMidiChannel(OBOE);

        JFrame frame = new JFrame("Violin Tuner");
        frame.setIconImage(new ImageIcon("Images/Icon.png").getImage());

        // buttons to panel
        JPanel notePanel = new JPanel(new GridLayout(2, 0, 8, 4));
        notePanel.setBorder(BorderFactory.createEmptyBorder(4, 8, 4, 8));
        int i = 0;
        for (Note note : Note.values()) {
            notePanel.add(buttons[i++] = makeButton(note));
        }

        // Component to impose a minimum width on the JFrame to ensure that the
        // window title is not truncated
        JPanel widener = new JPanel();
        widener.setPreferredSize(new Dimension(250, 0));

        // Panels to frame
        frame.setLayout(new BorderLayout());
        frame.add(notePanel, BorderLayout.CENTER);
        frame.add(widener, BorderLayout.SOUTH);
        frame.pack();

        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }

    private static MidiChannel initMidiChannel(int instrument) throws MidiUnavailableException {
        Synthesizer synth = MidiSystem.getSynthesizer();
        synth.open();
        MidiChannel channel = synth.getChannels()[0];
        // MIDI instruments are traditionally numbered from 1,
        // but the javax.midi API numbers them from 0
        channel.programChange(instrument - 1);
        channel.setChannelPressure(5);  // optional vibrato
        return channel;
    }

    private AbstractButton makeButton(final Note note) {
        AbstractButton button = new JToggleButton(note.name());
        button.addChangeListener(new ChangeListener() {
            @Override
            public void stateChanged(ChangeEvent e) {
                if (((AbstractButton)e.getSource()).isSelected()) {
                    for (AbstractButton b : ViolinTuner.this.buttons) {
                        // Turn off any note that might be being played
                        if (b != e.getSource()) {
                            b.setSelected(false);
                        }
                    }
                    ViolinTuner.this.play(note);
                } else {
                    ViolinTuner.this.silence();
                }
            }
        });
        return button;
    }

    public void play(Note note) {
        this.channel.noteOn(note.midiNoteNumber, 127); 
    }

    public void silence() {
        this.channel.allNotesOff();
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                try {
                    new ViolinTuner();
                } catch (MidiUnavailableException noMidi) {
                    noMidi.printStackTrace();
                    System.exit(1);
                }
            }
        });
    }
}
share|improve this answer
1  
Hmm, I don't get the reason for using channel.programChange(instrument - 1); in your initMidiChannel method, why not set OBOE to 68? –  Legato Jul 20 at 16:35
    
I also get a WARNING about pref root node unable to open at root 0, and a regex returning error code 5. P.S. Thanks for the amazing suggestions, and the very many interpretations it's all making for a great study! –  Legato Jul 20 at 16:54
1  
@Legato I've added a comment explaining the - 1. –  200_success Jul 20 at 18:17
    
I've only tested this on my own Mac, where it runs flawlessly. Does it work for you? Maybe MIDI support is broken on your machine, since I don't use any regex. –  200_success Jul 20 at 18:20
    
Yeah, for me 0 exists (it's a grand piano), I actually modified this code to switch between Oboe, Piano, and Violin sounds. It works, I get those messages in the command prompt but it works fine. –  Legato Jul 20 at 20:57

Layout

Calling .setSize() with fixed dimensions isn't portable. For example, this is what happens on Mac OS X:

Mac OS X screenshot

Instead, call frame.pack() to automatically set the frame size according to its contents.

Mac OS X screenshot with frame.pack()

If you want the 2 × 2 layout, then use a suitable LayoutManager — for example, p.setLayout(new GridLayout(2, 0, 10, 6));

Don't Repeat Yourself

The code is awfully repetitive. You should avoid repeating yourself, since repetition makes maintenance difficult: cut-and-paste programming leads to cut-and-paste bugs. I recommend using an Enum to represent the four notes.

Explicitness

It's bad practice to catch (Exception x). I have no clue what kind of exception would likely cause the flow of execution to jump to the exception handler. You, as the author, might intend for certain exceptions to be caught, but you don't know what will actually happen. Starting with Java 7, you can list multiple exceptions to be handled by the same catch block, and that's what I recommend that you do.

Avoid overuse of wildcard imports. There exists a small probability that a future version of some package could add a class that clashes and breaks your program. Mostly, using too many wildcard imports makes it hard for human maintainers to resolve identifiers — the compiler may be good at searching the classpath, but I'm not. My suggested guidelines are:

  • It's OK to use wildcard imports with the standard Java packages that almost every Java programmer is familiar with (such as java.io.* or java.util.*).
  • It's OK to use one wildcard import per file, maybe two, but not much more.
  • If in doubt, import individual classes explicitly.
  • List your imports alphabetically.

By the way, you never used anything in java.awt.*.

Suggested implementation

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;
import java.io.IOException;
import javax.sound.sampled.*;
import javax.swing.BorderFactory;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.ImageIcon;
import javax.swing.SwingUtilities;

public class ViolinTuner {
    private static enum Note {
        G, D, A, E;

        public final File file;

        Note() {
            this.file = new File("Sounds", this.name() + ".wav").getAbsoluteFile();
        }
    }

    private static JButton makeJButton(final Note note) {
        JButton button = new JButton(note.name());
        button.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                try {
                    AudioInputStream audioInputStream = AudioSystem.getAudioInputStream(note.file);
                    Clip clip = AudioSystem.getClip();
                    clip.open(audioInputStream);
                    clip.start();
                } catch (IOException | UnsupportedAudioFileException | LineUnavailableException x) {
                    x.printStackTrace();
                }
            }
        });
        return button;
    }

    private static void createAndShowGUI() {
        JFrame frame = new JFrame("Violin Tuner");
        frame.setIconImage(new ImageIcon("Images/Icon.png").getImage());

        // buttons to panel
        JPanel notePanel = new JPanel(new GridLayout(2, 0, 8, 4));
        notePanel.setBorder(BorderFactory.createEmptyBorder(4, 8, 4, 8));
        for (Note note : Note.values()) {
            notePanel.add(makeJButton(note));
        }

        // Component to impose a minimum width on the JFrame to ensure that the
        // window title is not truncated
        JPanel widener = new JPanel();
        widener.setPreferredSize(new Dimension(250, 0));

        // Panels to frame
        frame.setLayout(new BorderLayout());
        frame.add(notePanel, BorderLayout.CENTER);
        frame.add(widener, BorderLayout.SOUTH);
        frame.pack();

        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                createAndShowGUI();
            }
        }
    }
}
share|improve this answer
    
I initially used the frame.pack() method, but the issue I ran into/why I changed it is that it cuts out the title (I see this isn't the case on the Mac end), is there any way to circumvent this issue? –  Legato Jul 20 at 7:26
1  
I've added a widener component in Rev 2. –  200_success Jul 20 at 8:00

You can clean up the repetition by moving the ActionListener's code into a custom class, like this:

static class KeyButtonListener implements ActionListener {
    private final JButton button;
    private final String filename;

    KeyButtonListener(JButton button, String filename) {
        this.button = button;
        this.filename = filename;
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        AudioInputStream audioInputStream = null;
        try {
            audioInputStream = AudioSystem.getAudioInputStream(new File(filename));
            Clip clip = AudioSystem.getClip();
            clip.open(audioInputStream);
            clip.start();
        } catch (IOException | UnsupportedAudioFileException | LineUnavailableException x) {
            x.printStackTrace();
        } finally {
            if (audioInputStream != null) {
                try {
                    audioInputStream.close();
                } catch (IOException e1) {
                    e1.printStackTrace();
                }
            }
        }
    }
}

You can then create your listeners like this:

ActionListener gListener = new KeyButtonListener(G_KEY, "Sounds/G.wav");
ActionListener dListener = new KeyButtonListener(D_KEY, "Sounds/D.wav");
ActionListener aListener = new KeyButtonListener(A_KEY, "Sounds/A.wav");
ActionListener eListener = new KeyButtonListener(E_KEY, "Sounds/E.wav");

This cleans up the multiple if-else, because now there are 4 listeners instead of 1, and each is dedicated to one key. And there's no more need for checking the source of the event, as the listeners are dedicated to a single button.

Another change I did there, instead of catching Exception, I spell out all the possible exceptions that might be thrown there. This is a recommended practice in general. Often you might want to handle each exception a little bit differently, but in this simple case it just helps me see what exactly can go wrong in that block of code.

Finally, remember to close all streams that you opened, in this example the AudioInputStream.


Notice that the code still has much repetition: for each button, you repeat the following:

  • create the button
  • create a listener
  • add the listener to the button
  • add the button to the panel

You can do these in a loop:

JPanel panel = new JPanel();

for (String key : new String[]{"G", "D", "A", "E"}) {
    JButton keyButton = new JButton(key);
    ActionListener listener = new KeyButtonListener(keyButton, String.format("Sounds/%s.wav", key));
    keyButton.addActionListener(listener);
    panel.add(keyButton);
}

Minor things

My IDE doesn't like this line:

frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

It seems the correct constant to use is different:

frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);

Finally, all the comments in your code are redundant. It's better to remove them. Try to avoid comments as much as possible. If the code is not self-explanatory, try to improve it so it becomes self-explanatory. Here's a nice article on the topic:

http://blog.codinghorror.com/coding-without-comments/

share|improve this answer
    
Hmm, I tried something similar, it doesn't seem to affect anything. I wonder if I somehow should use a separate play method so the sounds don't overlap like they currently are. Thank you for this submission, the efficiency, readability is what I aspire to, I will try and separate my classes more. I'm actually not using an IDE (I thought as a beginner it would be too 'helpful' and maybe have too many shortcuts/hint that would inhibit my own understanding/learning) -- using WindowConstants.EXIT_ON_CLOSE just gives me a 'cannot find symbol' compile error. –  Legato Jul 20 at 15:55
    
Hm, if it doesn't compile then you don't have a choice, so it's a moot point. Never mind. About using an IDE, I gotta tell you that I'm huge fan of Vim, in fact it's still my favorite editor today, but for Java programming, it's really better to use a proper IDE like IntelliJ. You will save a LOT of time, and write better, more readable code, by correcting all its warnings. As long as you know how to build your programs without an IDE (which you absolutely must!), using an IDE won't hurt your learning experience, but help it. –  janos Jul 20 at 16:11

Initializing Swing on the Event Dispatch Thread

As noted in the Concurrency in Swing Java tutorial, Swing code must run in the Event Dispatch Thread, not the main thread. Therefore, you need to wrap the entire contents of your main() function inside SwingUtilities.invokeLater():

private static void createAndShowGUI() {
    JFrame frame = new JFrame("Violin Tuner");
    …
}

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

(I stumbled upon this problem the hard way.)

Adding a keyboard handler

While holding a violin in the left hand and a bow in the right hand, it can be difficult to grab a mouse and click on the appropriate button. You can, however, tap a key on a keyboard relatively easily. Therefore, I suggest adding a KeyListener to handle G, D, A, and E keys. I also suggest mapping Space to A = 440 Hz, since it is the easiest key to hit and the standard pitch to tune to.

share|improve this answer

Most of the really good feedback is already given, but I have one last recommendation:

If you're going to use audio clips instead of using MIDI, why not use a simple library like tinysound?

It takes care of so much, and is available under a BSD-2 clause license. https://github.com/finnkuusisto/TinySound

That way, you don't have to care about creating clips, making sure the Wave-files are in a correct form, etc.

It may not be the thing for you if you want to make a product to take on Cubase, but it's probably exactly the type of library you'd want now.

share|improve this answer

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.