Simple Java MIDI player

Question

I created a simple program that allows you to create and play MIDI sounds. I've used the MVC approach and I’d like to know if there are any improvements to be made regarding the design and structure of the classes? Have I used a constants class correctly? I also feel that the checkBoxArrayin the Midi class should be in a separate class. Should it be?

Please also let me know of any improvements to be made on the readability and general layout of the program.

For those interested, I've updated my code with the advice given on this thread: Simple Java MIDI player followup

BeatBox (main class)

package BeatBox;
public class BeatBox {
 GUI gui;
 Midi midi;
 public BeatBox(){
 midi = new Midi();
 gui = new GUI(midi);
 midi.setUpMidi();
 }
 public static void main(String[] args) {
 new BeatBox();
 }
}

GUI class

package BeatBox;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.*;
public class GUI {
 private Midi midi;
 public GUI(Midi midi){
 this.midi = midi;
 buildGUI();
 }
 // buildGUI - creates the GUI for the beat box program
 void buildGUI(){
 JFrame frame = new JFrame("Cyber BeatBox");
 frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
 frame.setMinimumSize(new Dimension(600, 350));
 JPanel panel = new JPanel(new BorderLayout());
 panel.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
 frame.setContentPane(panel);
 // Instrument names
 Box nameBox = new Box(BoxLayout.Y_AXIS);
 for(int i = 0; i < BeatBoxConstants.NUM_INSTRUMENTS; i++){
 nameBox.add(new Label(BeatBoxConstants.instrumentNames[i]));
 }
 // Check box
 GridLayout grid = new GridLayout(16, 16);
 grid.setVgap(0);
 grid.setHgap(2);
 JPanel checkBoxPanel = new JPanel(grid);
 for(int i = 0; i < BeatBoxConstants.NUM_INSTRUMENTS; i++){
 for(int j = 0; j < BeatBoxConstants.NUM_BEATS; j++) {
 JCheckBox checkBox = new JCheckBox();
 checkBoxPanel.add(checkBox);
 midi.checkBoxArray[i][j] = checkBox;
 }
 }
 // Buttons
 JPanel buttonsPanel = new JPanel();
 buttonsPanel.setLayout(new BoxLayout(buttonsPanel, BoxLayout.Y_AXIS));
 JButton startButton = new JButton("Start");
 JButton pauseButton = new JButton("Pause");
 startButton.addActionListener(new StartButtonListener());
 pauseButton.addActionListener(new GUI.PauseButtonListener());
 buttonsPanel.add(startButton);
 buttonsPanel.add(pauseButton);
 // Add everything to the frame
 frame.add(BorderLayout.WEST, nameBox);
 frame.add(BorderLayout.CENTER, checkBoxPanel);
 frame.add(BorderLayout.EAST, buttonsPanel);
 frame.pack();
 frame.setLocationRelativeTo(null);
 frame.setVisible(true);
 }
 // LISTENERS
 private class StartButtonListener implements ActionListener{
 @Override
 public void actionPerformed(ActionEvent ev){
 midi.buildTrackAndStart();
 midi.startSequencer();
 }
 }
 private class PauseButtonListener implements ActionListener{
 @Override
 public void actionPerformed(ActionEvent ev){
 midi.pauseSequencer();
 }
 }
}

Midi class (used to create the sounds)

package BeatBox;
import javax.sound.midi.*;
import javax.swing.JCheckBox;
public class Midi {
 private Sequencer sequencer;
 private Sequence seq;
 private Track track;
 JCheckBox[][] checkBoxArray = new JCheckBox[BeatBoxConstants.NUM_INSTRUMENTS][BeatBoxConstants.NUM_BEATS];
 // buildTrackAndStart - used to create and begin playing the newly created track
 void buildTrackAndStart(){
 try {
 seq.deleteTrack(track);
 track = seq.createTrack();
 makeTrack();
 sequencer.setSequence(seq);
 sequencer.setLoopCount(Sequencer.LOOP_CONTINUOUSLY);
 }catch(InvalidMidiDataException invalidMidiDataException){
 invalidMidiDataException.printStackTrace();
 }
 }
 // makeBeat - this creates the noteOn and noteOff beats
 private void makeBeat(int key, int tick) throws InvalidMidiDataException{
 track.add(makeEvent(144, 9, key, 100, tick));
 track.add(makeEvent(128, 9, key, 100, tick + 2));
 }
 // makeEvent - creates the notes for the track
 private MidiEvent makeEvent(int comd, int chan, int one, int two, int tick) throws InvalidMidiDataException{
 ShortMessage a = new ShortMessage();
 a.setMessage(comd, chan, one, two);
 return new MidiEvent(a, tick);
 }
 // makeTrack - creates the sound track based on the check boxes selected by the user
 private void makeTrack() throws InvalidMidiDataException{
 for(int i = 0; i < BeatBoxConstants.NUM_INSTRUMENTS; i++){
 for(int j = 0; j < BeatBoxConstants.NUM_BEATS; j++){
 if(checkBoxArray[i][j].isSelected()){
 makeBeat(BeatBoxConstants.instruments[i], j);
 }
 }
 }
 // Added to ensure that beat box completes the whole 16 beats before it loops again
 track.add(makeEvent(192, 9, 1, 0, BeatBoxConstants.NUM_BEATS - 1));
 }
 // setUpMidi - this sets up the sound system for the user to edit
 void setUpMidi(){
 try {
 sequencer = MidiSystem.getSequencer();
 sequencer.open();
 seq = new Sequence(Sequence.PPQ, 4);
 track = seq.createTrack();
 }catch(InvalidMidiDataException invalidMidiDataException){
 invalidMidiDataException.printStackTrace();
 }catch(MidiUnavailableException midiUnavailableException){
 midiUnavailableException.printStackTrace();
 }
 }
 // startSequencer - starts playing the sequencer
 void startSequencer(){
 sequencer.start();
 }
 // pauseSequencer - pauses the sequencer. This does not stop the sound, rather it pauses it.
 void pauseSequencer(){
 sequencer.stop();
 }
}

BeatBoxConstants (holds static info to be used by the other classes)

package BeatBox;
public class BeatBoxConstants {
 static final String[] instrumentNames = {"Bass Drum", "Closed Hi-Hat", "Open Hi-Hat", "Acoustic Snare", "Crash Cymbal",
 "Hand Clap", "High Tom", "Hi Bongo", "Maracas", "Whistle", "Low Conga", "Cowbell", "Vibraslap", "Low-mid Tom",
 "High Agogo", "Open Hi Conga"};
 static final int[] instruments = {35, 42, 46, 38, 49, 39, 50, 60, 70, 72, 64, 56, 58, 47, 67, 63};
 static final int NUM_BEATS = 16;
 static final int NUM_INSTRUMENTS = instrumentNames.length;
}

Answer 1

Disclaimer: I don't really know that much about music, so some of my assumptions might be wrong.

Have I used a constants class correctly?

I wouldn't say so. I would only use a constants class for things which are not part of the state of any objects, and which do not expose the inner workings of objects (eg a debug field, database credentials, fixed strings for user output, etc).

The numbers in instruments (which should be all upper case) seem to be random numbers (as in, they are not numbers a musician would recognize, but numbers that are specific to your program). They also seem to interact with instrumentNames, which is not good (see below).

NUM_INSTRUMENTS isn't really needed at all. NUM_BEATS is the only value that I might leave in a constants class.

I’d like to know if there are any improvements to be made regarding the design and structure of the classes?

One thing that directly jumped out at me is that it's really hard to see how you make the sounds of different instruments. It seems to depend on the passed key in makeBeat(int key, int tick). The call of that depends on the order of instruments in the instruments constant, and that it is in the correct order regarding instrumentNames, and displayed in the same order inside the GUI.

It's never a good idea to put that much weight on the order of different lists being correct (because those lists work independently of each other).

I would introduce an explicit Instrument class, which has a name and a key (if key is the technical term).

I would then pack the whole tick thing into a class as well (because of my lack of music knowledge it's hard to say how. Some ideas: Could a list of ticks be part of an instrument? Or could there be TickLists which each own one instrument?).

I also feel that the checkBoxArray in the Midi class should be in a separate class. Should it be?

Yes. Your Midi class is basically your model and controller, but it definitely isn't your GUI, so it shouldn't have JCheckBoxs. Instead, it should have the Instrument/TickList structure described above, which it can then expose to the GUI.

Misc

• a reset button would be great for usability, and should be easy to add.
• don't import *, instead make all your imports explicit, so readers know what classes you use.
• use JavaDoc style method comments (they are easier to read as they make input parameters and output explicit, and they can be displayed in IDE tooltips, published on the web, etc).
• Naming: seq can be a bit confusing, as it's unclear if it is sequencer or sequence; I would just write it out. At first, I thought comd is some music term, but as it stands for command, just write it out as well. Same for chan (channel).

• Calculus5000: Great, thanks for your help! I’m a bit unsure what’s meant about the correct use of a constants class. Isn’t NUM_BEATS also part of the object (e.g. checkBoxArray in the Midi class)? Also, should instruments and instrumentNames be replaced by INSTRUMENTS and INSTRUMENT_NAMES? If so, then what’s the definition of a constant in Java, because I thought that constants are immutable but with final arrays you can still change the elements? Or is the definition of a constant a static final variable?
• tim: @Calculus5000 I assumed NUM_BEATS is something like a configuration? Eg, it could also be 20 and it wouldn't really matter for the rest of the code (only the sound produced)? And yes, static final variables are conventionally written all upper case. And in Java, there isn't really a thing as a constant object (well, a final instance of an immutable object could be seen as one, I guess). But by convention, static final + all upper case signals that something should be considered a constant.
• Calculus5000: Also, should I break up the code in buildGUI() in the GUI class?
• Calculus5000: Yes, that's correct. NUM_BEATS can be any number (16 was arbitrarily chosen). I'm just a bit confused as to why that can be a constant and instrumentNames shouldn't?
• tim: @Calculus5000 I would probably do so, yes. Maybe create functions for the creation of the main frame, creation of checkboxes, and creation of buttons (basically anything you signal via comments right now). And NUM_BEATS doesn't seem to have any meaning inside the program. But instrumentNames does. If I change one of the strings, your whole program would behave rather oddly (because my changed string doesn't fit in with the number stored in instruments). Additionally, creating a class/enum for instruments will probably lead to cleaner code in general.