9
\$\begingroup\$

I've been reading "Head First Java" and some of the early projects surround basic swing and basic MIDI.

So I've put the two together and written a basic "Synthesiser"/"Piano" using what I have learned.

I know that I need to add some safety features and there's probably some method optimisation staring me in the face but would love to know what you all think!

import javax.swing.*;
import javax.sound.midi.*;
import java.awt.*;
import java.awt.event.*;
import java.util.ArrayList;

public class VirtualKeyboard implements ActionListener {

    public static ArrayList<String> note = new ArrayList<String>();
    public static ArrayList<String> extendedNote = new ArrayList<String>();
    JTextField octaveChoice;
    JTextField instrumentChoice;

    public static void main (String[] args) {
        VirtualKeyboard gui = new VirtualKeyboard();

        String[] preNote = {"C","D","E","F","G","A","B"};
        String[] preExtendedNote = {"C","C#","D","D#","E","F","F#","G","G#","A","A#","B"};

        for (int i = 0; i < 7; i++) {
            note.add(preNote[i]);
        }

        for (int i = 0; i < 12; i++) {
            extendedNote.add(preExtendedNote[i]);
        }

        gui.setGUI();
    }

    public void setGUI() {
        JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
        JPanel keyPanel = new JPanel();
        JPanel controlPanel = new JPanel();

        JButton[] key = new JButton[7];

        for (int i = 0; i < 7; i++) {
            key[i] = new JButton(note.get(i));
            key[i].addActionListener(this);
            keyPanel.add(key[i]);
        }

        JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
        instrumentChoice = new JTextField(1);

        JLabel octaveChoiceLabel = new JLabel("Octave: ");
        octaveChoice = new JTextField(1);

        controlPanel.add(instrumentChoiceLabel);
        controlPanel.add(instrumentChoice);
        controlPanel.add(octaveChoiceLabel);
        controlPanel.add(octaveChoice);

        frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
        frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
        frame.setSize(500,200);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);
    }

    public void actionPerformed(ActionEvent e) {
        int finalNote = 0;
        Object source = e.getSource();
        if (source instanceof JButton) {
            JButton but = (JButton) source;
            System.out.print(but.getText());

            finalNote = (Integer.parseInt(octaveChoice.getText()) * 12) + extendedNote.indexOf(but.getText());

            System.out.print(" (" + finalNote + ")\n");

            playNote(finalNote, Integer.parseInt(instrumentChoice.getText()));
        }       
    }

    public void playNote(int finalNote, int finalInstrument) {
        try {

            Sequencer sequencer = MidiSystem.getSequencer();
            sequencer.open();
            Sequence sequence = new Sequence(Sequence.PPQ,4);
            Track track = sequence.createTrack();

            MidiEvent event = null;

            ShortMessage first = new ShortMessage();
            first.setMessage(192,1,finalInstrument,0);
            MidiEvent changeInstrument = new MidiEvent(first, 1);
            track.add(changeInstrument);

            ShortMessage a = new ShortMessage();
            a.setMessage(144,1,finalNote,100);
            MidiEvent noteOn = new MidiEvent(a, 1);
            track.add(noteOn);

            ShortMessage b = new ShortMessage();
            b.setMessage(128,1,finalNote,100);
            MidiEvent noteOff = new MidiEvent(b, 16);
            track.add(noteOff);

            sequencer.setSequence(sequence);
            sequencer.start();
        } catch (Exception ex) { ex.printStackTrace(); }

    }
}
\$\endgroup\$
10
\$\begingroup\$
import javax.swing.*;
import javax.sound.midi.*;
import java.awt.*;
import java.awt.event.*;

Most style guides will frown upon the wildcard import -- is it so much extra work to list the classes you actually depend on? This will also help you to avoid conflicts, where you want List to mean java.util.List rather than java.awt.List

public static ArrayList<String> note = new ArrayList<String>();
public static ArrayList<String> extendedNote = new ArrayList<String>();

ArrayList is an implementation detail, that consumers generally don't need to know about. The more usual declaration would look like:

public static List<String> note = new ArrayList<String>();
public static List<String> extendedNote = new ArrayList<String>();

Furthermore, by specifying that these are lists, you are claiming that they support insert. Is it really a good idea to allow other code to start inserting new notes into your lists?

An alternative would be to expose the notes as Iterable, rather than List, which helps disguise the fact that the underlying container supports inserts. Better would be to make that data private, and provide methods that give access to the notes.

(Note: the spelling change of the variable name; this thing isn't a note, it's a collection of notes).

private static List<String> notes = new ArrayList<String>();

public static List<String> getNotes() {
    return Collections.unmodifiableList(notes);
}

The best answer isn't clear, as your example doesn't include any external consumers.

As a rule, you should try to keep variable declarations near their use -- it makes the code clearer to read, and helps with refactoring the code later.

public static void main (String[] args) {

    String[] preNote = {"C","D","E","F","G","A","B"};
    for (int i = 0; i < 7; i++) {
        note.add(preNote[i]);
    }

    String[] preExtendedNote = {"C","C#","D","D#","E","F","F#","G","G#","A","A#","B"};
    for (int i = 0; i < 12; i++) {
        extendedNote.add(preExtendedNote[i]);
    }

    VirtualKeyboard gui = new VirtualKeyboard();
    gui.setGUI();
}

Magic numbers are poor form; you should avoid them where possible.

    String[] preNote = {"C","D","E","F","G","A","B"};
    for (int i = 0; i < preNote.length; i++) {
        note.add(preNote[i]);
    }

You should also avoid writing code that has already been written for you.

    String[] preNote = {"C","D","E","F","G","A","B"};
    List<String> notes = Arrays.asList(preNote);

Or more directly

    List<String> note = Arrays.asList("C","D","E","F","G","A","B");

As an aside - it may make more sense to treat the notes as Enumerations, rather than Strings.

    JButton[] key = new JButton[7];

    for (int i = 0; i < 7; i++) {
        key[i] = new JButton(note.get(i));
        key[i].addActionListener(this);
        keyPanel.add(key[i]);
    }

Two problems here -- you're repeating your magic number again; and also you are creating an array that you never use.

    for (int i = 0; i < note.size(); i++) {
        JButton key = new JButton(note.get(i));
        key.addActionListener(this);
        keyPanel.add(key);
    }

But this is silly -- you put notes in a List, you should use it

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(this);
        keyPanel.add(key);
    }

OK, let's get some real meat...

public void setGUI() {
    JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
    JPanel keyPanel = new JPanel();
    JPanel controlPanel = new JPanel();

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(this);
        keyPanel.add(key);
    }

    JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
    instrumentChoice = new JTextField(1);

    JLabel octaveChoiceLabel = new JLabel("Octave: ");
    octaveChoice = new JTextField(1);

    controlPanel.add(instrumentChoiceLabel);
    controlPanel.add(instrumentChoice);
    controlPanel.add(octaveChoiceLabel);
    controlPanel.add(octaveChoice);

    frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
    frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
    frame.setSize(500,200);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);
}

OK, what you are doing here is building an object graph -- creating instances of objects and establishing the relationships between them. That shouldn't happen in an object that also does anything else (separation of concerns).

The VirtualKeyboard is an ActionListener that plays sounds. So it shouldn't also be doing the job of building it's own object graph. Building things is the domain of Factory objects, so you should implement the creation of the object graph elsewhere.

But - it's not easy here. The action listener holds two of the objects that are needed to build the UI, but all the others are created somewhere else. The riddle is solved by passing the JTextFields to the VirtualKeyboard, rather than the other way around....

private final JTextField octaveChoice;
private final JTextField instrumentChoice;

public VirtualKeyboard(JTextField octaveChoice, JTextField instrumentChoice) {
    this.octaveChoice = octaveChoice;
    this.instrumentChoice = instrumentChoice;
}

So we need the JTextField before we create VirtualKeyboard, which means that we need to create those objects in main(). This is actually pretty common -- main is a good place to put the object assembly you need when bootstrapping your application.

public static void main (String[] args) {
    JTextField instrumentChoice = new JTextField(1);
    JTextField octaveChoice = new JTextField(1);

    VirtualKeyboard gui = new VirtualKeyboard(octaveChoice, instrumentChoice);

    JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");
    JPanel keyPanel = new JPanel();
    JPanel controlPanel = new JPanel();

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(gui);
        keyPanel.add(key);
    }

    JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");

    JLabel octaveChoiceLabel = new JLabel("Octave: ");

    controlPanel.add(instrumentChoiceLabel);
    controlPanel.add(instrumentChoice);
    controlPanel.add(octaveChoiceLabel);
    controlPanel.add(octaveChoice);

    frame.getContentPane().add(BorderLayout.NORTH, keyPanel);
    frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);
    frame.setSize(500,200);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);

The code should be re-organized so that you can see how the parts relate....

public static void main (String[] args) {

    JFrame frame = new JFrame("Rhysy's Virtual Keyboard!!!");

    JPanel controlPanel = new JPanel();

    JLabel instrumentChoiceLabel = new JLabel("Instrument Choice: ");
    controlPanel.add(instrumentChoiceLabel);

    JLabel octaveChoiceLabel = new JLabel("Octave: ");
    controlPanel.add(octaveChoiceLabel);

    JTextField instrumentChoice = new JTextField(1);
    controlPanel.add(instrumentChoice);

    JTextField octaveChoice = new JTextField(1);
    controlPanel.add(octaveChoice);


    VirtualKeyboard gui = new VirtualKeyboard(octaveChoice, instrumentChoice);


    frame.getContentPane().add(BorderLayout.SOUTH, controlPanel);


    JPanel keyPanel = new JPanel();

    for (String currentNote : note) {
        JButton key = new JButton(currentNote);
        key.addActionListener(gui);
        keyPanel.add(key);
    }
    frame.getContentPane().add(BorderLayout.NORTH, keyPanel);


    frame.setSize(500,200);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setVisible(true);

It's common to create classes that do all this work -- done well, it tends to improve both testability and re-use.

Let's look at the action code again

public void actionPerformed(ActionEvent e) {
    int finalNote = 0;
    Object source = e.getSource();
    if (source instanceof JButton) {
        JButton but = (JButton) source;
        System.out.print(but.getText());

        finalNote = (Integer.parseInt(octaveChoice.getText()) * 12) + extendedNote.indexOf(but.getText());

        System.out.print(" (" + finalNote + ")\n");

        playNote(finalNote, Integer.parseInt(instrumentChoice.getText()));
    }       
}

There are at least three things going on here...

public void actionPerformed(ActionEvent e) {
    Object source = e.getSource();
    if (source instanceof JButton) {
        JButton but = (JButton) source;

        String note = but.getText();
        String octave = octaveChoice.getText();
        String instrument = instrumentChoice.getText();


        int finalNote = (Integer.parseInt(octave) * 12) + extendedNote.indexOf(note);
        int finalInstrument = Integer.parseInt(instrument);


        playNote(finalNote, finalInstrument);
    }
}

You've got the ActionListener, which is reading Strings out of awt, you've got a Parser, which knows how to turn those strings into notes and instruments, and you've got a Player that knows how to turn notes and instruments into sounds.

This separation is important, because you should be able to do test that the parser does the right thing without assembling the entire UI for it. You should be able to replace this player with another one that uses a different set of magic numbers to generate ShortMessages.

\$\endgroup\$
3
\$\begingroup\$
  • If this class is intended to represent the whole UI, window and all, you might consider extending JFrame and using this rather than frame.

  • Right now, when you initially create a VirtualKeyboard, it's unusable...and it doesn't need to be. The stuff that setGui does is a constructor's job, and should probably be in a constructor instead.

  • (Well, aside from setVisible(true), which should probably be in main. The caller should be allowed to decide whether/when to show the window.)

  • The stuff that plays notes probably shouldn't be in the same class as your GUI.

  • It looks like what you're doing in playNote is creating a Sequencer, setting it up with a one-note track, and playing it. That's rather a roundabout way of playing a note, and is going to cause you issues later -- since you have to know the length of the note in advance, you're going to run into trouble when you want to hold a note for longer than one beat.

    Consider using javax.sound.midi.Synthesizer. It seems designed for more "interactive" cases, like yours. You can start and stop notes virtually at will.

    Of course, any decent amount of control is also going to require splitting up playNote. Currently, you emit events to set the instrument, start the note, and then stop it. Each of those things could be done independently -- and probably should.

\$\endgroup\$
2
\$\begingroup\$

Your question reminds me a lot of one of my recent reviews…

Event handling

MIDI

  • Use the higher-level Synthesizer interface instead of Sequencer.
  • You could use Synthesizer.getAvailableInstruments() to populate a drop-down menu of instruments, which would be much nicer than asking the user to enter an instrument number.
  • In any case, the Instrument and Octave widgets should have sensible default values.
  • Calling sequencer.open() every time you want to play a single note seems wasteful. Furthermore, on my machine (Java 1.7 on OS X 10.9), MIDI initialization takes about one second, leading to a hesitation before being able to sound the first note. Instead, initialize MIDI once when the program starts.

General

\$\endgroup\$

protected by Community Jan 27 '16 at 18:18

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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