8
\$\begingroup\$

This is my second iteration of a simple Java MIDI player. I've made several amendments and would like to see if the code is now correctly implemented and readable. This follows on from the first review.

With regards to the GUI class, is it a good idea to have musicInfo = midi.musicInfo in the GUI constructor? Or should I access midi.musicInfo directly from a GUI instance rather than using the musicInfo reference variable?

Also, as this is my first time using JavaDoc style comments, please provide any improvements that could be made with respect to the comments and any other improvements in general.

BeatBox class (main class)

package BeatBox;

public class BeatBox {
    GUI gui;
    Midi midi;

    /**
     * Constructor for the beat box program
     */
    public BeatBox(){
        midi = new Midi(new MusicInfo());
        gui = new GUI(midi);
        midi.setUpMidi();
    }

    public static void main(String[] args) {
        new BeatBox();
    }
}

GUI class

package BeatBox;

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.GridLayout;
import java.awt.Label;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.BorderFactory;
import javax.swing.BoxLayout;
import javax.swing.JButton;
import javax.swing.JCheckBox;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.WindowConstants;

/**
 * GUI class is used to produce the graphical representation of the beat box program
 */
public class GUI {
    private Midi midi;
    private MusicInfo musicInfo;
    private JFrame frame;

    /**
     * GUI Constructor
     * @param midi Midi object. Used to invoke methods in the button listeners
     */
    public GUI(Midi midi){
        this.midi = midi;
        musicInfo = midi.musicInfo;
        buildGUI();
    }


    /**
     * buildButtons - Builds the panel consisting of buttons
     */
    private void buildButtons(){
        JPanel buttonsPanel = new JPanel();
        buttonsPanel.setLayout(new BoxLayout(buttonsPanel, BoxLayout.Y_AXIS));
        JButton startButton = new JButton("Start");
        JButton pauseButton = new JButton("Pause");
        JButton resetButton = new JButton("Reset");

        startButton.addActionListener(new StartButtonListener());
        pauseButton.addActionListener(new PauseButtonListener());
        resetButton.addActionListener(new ResetButtonListener());

        buttonsPanel.add(startButton);
        buttonsPanel.add(pauseButton);
        buttonsPanel.add(resetButton);
        frame.add(BorderLayout.EAST, buttonsPanel);
    }

    /**
     * buildCheckBoxes - Builds a panel consisting of a grid of check boxes that are used to create a sound track
     */
    private void buildCheckBoxes(){
        // Check box
        GridLayout grid = new GridLayout(Midi.INSTRUMENT_NAMES.length, BeatBoxConstants.NUM_BEATS);
        grid.setVgap(0);
        grid.setHgap(2);
        JPanel checkBoxPanel = new JPanel(grid);
        for(int i = 0; i < Midi.instruments.length; i++){
            for(int j = 0; j < BeatBoxConstants.NUM_BEATS; j++) {
                JCheckBox checkBox = new JCheckBox();
                checkBoxPanel.add(checkBox);
                musicInfo.addCheckBox(i, j, checkBox);
            }
        }
        frame.add(BorderLayout.CENTER, checkBoxPanel);
    }

    /**
     * buildFrame - Builds the GUI frame
     */
    private void buildFrame(){
        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);
    }

    /**
     * buildGUI - Builds the GUI for the beat box program
     */
    private void buildGUI(){
        buildFrame();
        buildInstrumentLabel();
        buildCheckBoxes();
        buildButtons();
        displayFrame();
    }

    /**
     * buildInstrumentLabel - Builds the panel showing the instrument names
     */
    private void buildInstrumentLabel(){
        JPanel namePanel = new JPanel();
        namePanel.setLayout(new BoxLayout(namePanel, BoxLayout.Y_AXIS));
        for(int i = 0; i < Midi.instruments.length; i++){
            namePanel.add(new Label(Midi.instruments[i].getName()));
        }
        frame.add(BorderLayout.WEST, namePanel);
    }

    /**
     * displayFrame - Sets size and location of the GUI window and displays it
     */
    private void displayFrame(){
        frame.pack();
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }

    // LISTENERS
    /**
     * StartButtonListener - Deletes previous track, creates the new track based on the check boxes selected and starts
     * playing it
     */
    private class StartButtonListener implements ActionListener{
        @Override
        public void actionPerformed(ActionEvent ev){
            midi.buildTrackAndStart();
            midi.startSequencer();
        }
    }

    /**
     * PauseButtonListener - Pauses the sequencer
     */
    private class PauseButtonListener implements ActionListener{
        @Override
        public void actionPerformed(ActionEvent ev){
            midi.pauseSequencer();
        }
    }

    /**
     * ResetButtonListener - Pauses the sequencer and deselects all check boxes
     */
    private class ResetButtonListener implements ActionListener{
        @Override
        public void actionPerformed(ActionEvent ev){
            midi.pauseSequencer();
            for(int i = 0; i < Midi.INSTRUMENT_NAMES.length; i++){
                for(int j = 0; j < BeatBoxConstants.NUM_BEATS; j++){
                    musicInfo.deselectCheckBox(i, j);
                }
            }
        }
    }
}

Midi class

package BeatBox;

import javax.sound.midi.InvalidMidiDataException;
import javax.sound.midi.MidiEvent;
import javax.sound.midi.MidiSystem;
import javax.sound.midi.MidiUnavailableException;
import javax.sound.midi.Sequence;
import javax.sound.midi.Sequencer;
import javax.sound.midi.ShortMessage;
import javax.sound.midi.Track;

/**
 * Midi class deals with creating and playing the sound tracks produced by the user
 */
public class Midi {
    private Sequencer sequencer;
    private Sequence sequence;
    private Track track;

    static final String[] INSTRUMENT_NAMES = {"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"};
    private static final int[] INSTRUMENT_KEYS = {35, 42, 46, 38, 49, 39, 50, 60, 70, 72, 64, 56, 58, 47, 67, 63};
    static final Instrument[] instruments = new Instrument[INSTRUMENT_NAMES.length];
    MusicInfo musicInfo;

    /**
     * Midi constructor
     * @param musicInfo This is the information gathered from the user's input
     */
    public Midi(MusicInfo musicInfo){
        this.musicInfo = musicInfo;
        setUpInstruments();
    }

    /**
     * buildTrackAndStart - Builds track and begins playing it
     */
    void buildTrackAndStart(){
        try {
            sequence.deleteTrack(track);
            track = sequence.createTrack();
            makeTrack();
            sequencer.setSequence(sequence);
            sequencer.setLoopCount(Sequencer.LOOP_CONTINUOUSLY);
        }catch(InvalidMidiDataException invalidMidiDataException){
            invalidMidiDataException.printStackTrace();
        }
    }

    /**
     * makeBeat - Creates beats consisting of two events; a event that turns on the beat and another that turns it off
     * @param key the instrument used to play this beat
     * @param tick the time at which to play this beat
     * @throws InvalidMidiDataException
     */
    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 MIDI notes for the track
     * @param command the MIDI command
     * @param channel the channel associated with this event
     * @param one the first data byte
     * @param two the second data byte
     * @param tick the time at which the event occurs
     * @return a MidiEvent
     * @throws InvalidMidiDataException
     */
    private MidiEvent makeEvent(int command, int channel, int one, int two, int tick) throws InvalidMidiDataException{
        ShortMessage a = new ShortMessage();
        a.setMessage(command, channel, one, two);
        return new MidiEvent(a, tick);
    }

    /**
     * makeTrack - Creates the sound track based on the selected check boxes
     * @throws InvalidMidiDataException
     */
    private void makeTrack() throws InvalidMidiDataException{
        for(int i = 0; i < instruments.length; i++){
            for(int j = 0; j < BeatBoxConstants.NUM_BEATS; j++){
                if(musicInfo.isCheckBoxSelected(i, j)){
                    makeBeat(instruments[i].getKey(), j);
                }
            }
        }
        // Added to ensure that beat box plays the whole track before it loops again
        track.add(makeEvent(192, 9, 1, 0, BeatBoxConstants.NUM_BEATS - 1));
    }

    /**
     * setUpInstruments - Fills the instrument array with instances of Instrument
     */
    static void setUpInstruments(){
        for(int i = 0; i < instruments.length; i++){
            instruments[i] = new Instrument(INSTRUMENT_NAMES[i], INSTRUMENT_KEYS[i]);
        }
    }

    /**
     * setUpMidi - Sets up the MIDI sound system
     */
    void setUpMidi(){
        try {
            sequencer = MidiSystem.getSequencer();
            sequencer.open();
            sequence = new Sequence(Sequence.PPQ, 4);
            track = sequence.createTrack();
        }catch(InvalidMidiDataException invalidMidiDataException){
            invalidMidiDataException.printStackTrace();
        }catch(MidiUnavailableException midiUnavailableException){
            midiUnavailableException.printStackTrace();
        }
    }

    /**
     * startSequencer - Starts playing the sequencer
     */
    void startSequencer(){
        sequencer.start();
    }

    /**
     * pauseSequencer - Pauses the sequencer
     * Note: This does not stop the sound. Rather, it pauses it.
     */
    void pauseSequencer(){
        sequencer.stop();
    }
}

MusicInfo class

package BeatBox;

import javax.swing.JCheckBox;

/**
 * MusicInfo class deals with all the sound information gathered from the user
 */
public class MusicInfo {
    private JCheckBox[][] checkBoxArray;

    /**
     * MusicInfo constructor
     */
    public MusicInfo(){
        checkBoxArray = new JCheckBox[Midi.INSTRUMENT_NAMES.length][BeatBoxConstants.NUM_BEATS];
    }

    /**
     * addCheckBox - Assigns checkBox to the appropriate index location in checkBoxArray
     * @param a row index of checkBoxArray
     * @param b column index of checkBoxArray
     * @param checkBox the check box to be added to checkBoxArray
     */
    void addCheckBox(int a, int b, JCheckBox checkBox){
        checkBoxArray[a][b] = checkBox;
    }

    /**
     * deselectCheckBox - Deselects the check box in question provided it is already selected
     * @param a row index of checkBoxArray
     * @param b column index of checkBoxArray
     */
    void deselectCheckBox(int a, int b){
        if(isCheckBoxSelected(a, b)) {
            checkBoxArray[a][b].setSelected(false);
        }
    }

    /**
     * isCheckBoxSelected - Determines whether the check box has been selected
     * @param a row index of checkBoxArray
     * @param b column index of checkBoxArray
     * @return true if user checked the box and false if user did not
     */
    boolean isCheckBoxSelected(int a, int b){
        return checkBoxArray[a][b].isSelected();
    }
}

Instrument class

package BeatBox;

/**
 * Instrument class deals with the MIDI information of each instrument
 */
public class Instrument {
    private String name;
    private int key;

    /**
     * Instrument Constructor
     * @param name the instrument's name
     * @param key the key used to play the instrument in a Midi instance
     */
    public Instrument(String name, int key){
        this.name = name;
        this.key = key;
    }

    // GETTERS
    String getName(){
        return name;
    }

    int getKey(){
        return key;
    }
}

BeatBoxConstants class

package BeatBox;

/**
 * BeatBoxConstants contains constants that are used throughout the beat box program
 */
public class BeatBoxConstants {
    /**
     * This sets the number of beats that one loop of the track can have
     */
    static final int NUM_BEATS = 16;
}
\$\endgroup\$
13
\$\begingroup\$

Well, your post is over a year old, but hopefully this can still be of some use to you!

Javadoc

  1. No need to say the method name again - the method name is automatically part of the Javadoc.
  2. What are the parameter constraints? (To be fair, you don't enforce any constraints in your code, so it makes sense to not mention any in your Javadoc) That said, you should at least have some minimal constraints on your public methods and constructors (eg. The GUI constructor takes a Midi object - do you want to allow a consumer to pass in null?)
  3. When are your exceptions thrown? I see lots of classes which throw a InvalidMidiDataException, but as a consumer of your code, I would want to know what causes that exception to be thrown.
  4. Private methods. It's not wrong to add Javadoc on private methods, and some people will argue that every private method should have Javadoc. I don't necessarily agree (Feel free to come to your own conclusion on this one), and I am of the opinion that if a private method is so long that it needs documentation on what it does, you should probably clean that code.

GUI Class

  1. Private initializers. I try to not use them if possible, but I do understand that they have a place. When code is short and simple (like this project), then I find it much easier to read if all of the UI components are setup directly in the constructor. I've found that it's easier to understand what is all happening and build a proper UI hierarchy when they're setup sequentially. A little documentation of what is being setup can go a long way as well. Plus, any variables that need to be accessed elsewhere in the class can still be final since they're initialized in the constructor. That said, in a very complex and large class it can be more readable to break apart the constructor work into some private methods. But even then I might argue that your constructor is doing too much, or that your class is responsible for too many things.

    public GUI(final Midi midi) {
        // ...
        buildGUI();
    }
    
    // Each private method here is initializing some other UI component
    private void buildGUI() {
        buildFrame();
        buildInstrumentLabel();
        buildCheckBoxes();
        buildButtons();
        displayFrame();
    }
    

    could instead be:

    private final JFrame baseFrame;
    
    public GUI(final Midi midi) {
        // Setup base frame
        baseFrame = new JFrame();
        final JPanel basePanel = new JPanel(new BorderLayout());
        // ... continue setting frame attributes ...
    
        // Setup instrument labels
        final JPanel namePanel = new JPanel();
        // ... continue setting name panel attributes ...
        baseFrame.add(namePanel);
    
        // Setup check boxes
        final JPanel checkBoxPanel = new JPanel(new GridLayout());
        // .. continue setting check box panel attributes ...
        baseFrame.add(checkBoxPanel);
    
        // etc.
    }
    

    Again, maybe just personal preference, but I find this approach a bit easier to see how the hierarchy is being built up.

Instrument Class

Why not make this class an enum? By declaring the display Strings and key values as static variables in your Midi class, it's already constant. For example:

@SuppressWarnings("javadoc")
public enum Instrument {

    ACOUSTIC_BASS_DRUM(35, "Acoustic Bass Drum"), //$NON-NLS-1$
    ACOUSTIC_SNARE(38, "Acoustic Snare"), //$NON-NLS-1$
    // ... etc ...

    private final int key;
    private final String display;

    private Instrument(final int key, final String display) {
        this.key = key;
        this.display = display;
    }

    public int getKey() {
        return key;
    }

    public String getDisplay() {
        return display;
    }

}

This would also make a few things simpler in your code:

  1. Midi.setupInstruments() can be removed
  2. Instead of using a for loop and iterating with a counter, you can use the built-in List iterator and iterate over Instrument.values().

MusicInfo Class

Long story short, you don't need this (I also think there's a better approach than using a 2D array, but for now, let's assume the array stays).

The 2D array that's being wrapped here is a UI component, so it can easily become part of the GUI class. The methods in this class are just syntactic sugar, and really don't provide any additional benefits. Plus:

void deselectCheckBox(final int a, final int b) {
    if (isCheckBoxSelected(a, b)) {
        checkBoxArray[a][b].setSelected(false);
    }
}

can just as easily be:

void deselectCheckBox(final int a, final int b) {
    checkBoxArray[a][b].setSelected(false);
}

Who cares if it's not selected already? Doesn't hurt anything to set it to false again, plus now it's even more clear that each method is an unnecessary wrapper.

Midi Class

  1. Use static variables that are already part of the Javax MIDI library. For example, your 128 and 144 magic numbers can be replaced with ShortMessage.NOTE_ON and ShortMessage.NOTE_OFF respectively. This applies for the 192 PROGRAM_CHANGE message as well. I'm not sure what that's for or why it's necessary, but I'll take your word for it :)

    Similarly, that magic 9 can be made a private static final variable on the class. And the 100. I'm not sure what that does either. Velocity? Let's go with Velocity.

Design

From a design perspective, I see a few issues:

  1. Your MVC usage is a bit skewed.

    We have the GUI class (clearly the View), MusicInfo (Unnecessary model that the View uses, but is also passed to Midi?), Midi (Controller, but is also directly accessing MusicInfo which should be internal to the View), Instrument (A model) and BeatBoxConstants (More on this later...).

    In the MVC pattern what is supposed to happen is this:

    1. User interacts with the view
    2. View notifies the controller of what happened
    3. Controller handles this/does some business logic/updates its state and models
    4. Controller updates the view according to its logic and state changes
    5. Repeat...
    

MVC Pattern

I prefer to accomplish this by creating the view, and injecting it into the controller in the constructor as follows:

// The view should be dumb. It will create the UI elements, but if there is user interaction, it should only notify the controller and not handle any logic.
@SuppressWarnings("javadoc")
public class BeatBoxView {
    private final List<BeatBoxViewListener> listeners;

    public class BeatBoxViewListener {
        public void onStartButtonClicked();
        public void onPauseButtonClicked();
        public void onResetButtonClicked();
    }

    public BeatBoxView() {
        final JButton startButton = new JButton("Start");
        // Instead of having three inner classes for each button, they can be implemented anonymously like this since they won't be used anywhere else and are very simple implementations
        startButton.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                for (BeatBoxViewListener listener : listeners) {
                    listener.onStartButtonClicked();
                }
            }
        });
        // Repeat for pause and reset buttons
    }

}

Just to reiterate, the view should be a very thin layer of the application. It does not have any business logic, and it should be the only place in your code where UI elements (AWT, Swing, etc. classes) reside. That is, no other classes should have ANY knowledge about what UI framework the view is using. Why? Those classes shouldn't care! (Separation of Concerns)

// The controller implements the view listener so that it can be notified by the view when something happens
@SuppressWarnings("javadoc")
public class BeatBoxController implements BeatBoxViewListener {
    public BeatBoxController(final BeatBoxView view, final Midi midi) {
        this.view = view;
        this.midi = midi;
    }

    @Override
    public void onStartButtonClicked() {
        midi.startSequencer();
    }

    @Override
    public void onPauseButtonClicked() {
        midi.stopSequencer();
    }

    @Override
    public void onResetButtonClicked() {
        midi.stopSequencer();
        view.clearSelectedBeats(); // Instead of directly accessing MusicInfo the view should expose a method to reset itself
    }
}

In an example workflow, let's say we're playing a track and click the reset button. The view added a listener to the reset button, so that listener will fire. The implementation is simple and only notifies the views listeners the the button was clicked (the view is supposed to be dumb, remember?). The controller now is notified, and it is smart so it will know what to do! The controller takes charge and stops the sequencer (updating the state), and then tells the view to reset itself. And that completes the cycle! (Interact with view -> view notifies controller -> controller modifies state -> controller updates view)

  1. Back to the MusicInfo class, and the GUI class. Do away with MusicInfo. I view your UI as having rows. Each row is for an instrument, and each row has some checkboxes which represent beats. To model this, we can make InstrumentRow:

    @SuppressWarnings("javadoc")
    public class InstrumentRow {
    
        private final String name;
        private final int numBeats;
        private final List<JCheckBox> checkBoxes;
        private final List<InstrumentRowListener> listeners;
    
        public interface InstrumentRowListener {
            public void onCheckBoxSelectionChanged(final String name, final int beat);
        }
    
        public InstrumentRow(final JPanel nameParent, final JPanel checkBoxParent, final String name, final int numBeats) {
            this.name = name;
            this.numBeats = numBeats;
            checkBoxes = new ArrayList<JCheckBox>();
            listeners = new ArrayList<InstrumentRowListener>();
    
            nameParent.add(new Label(name));
    
            for (int i = 0; i < numBeats; ++i) {
                final JCheckBox checkBox = new JCheckBox();
                checkBoxes.add(checkBox);
                checkBox.addActionListener(new ActionListener() {
                    @SuppressWarnings("synthetic-access")
                    @Override
                    public void actionPerformed(final ActionEvent e) {
                        for (final InstrumentRowListener listener : listeners) {
                            listener.onCheckBoxSelectionChanged(name, checkBoxes.indexOf(checkBox));
                        }
                    }
                });
                checkBoxParent.add(checkBox);
            }
    
        }
    
        public void addListener(final InstrumentRowListener listener) {
            listeners.add(listener);
        }
    
        public String getName() {
            return name;
        }
    
        public List<Integer> getSelectedBeats() {
            final List<Integer> selectedBeats = new ArrayList<Integer>();
            for (int i = 0; i < checkBoxes.size(); ++i) {
                if (checkBoxes.get(i).isSelected()) {
                    selectedBeats.add(i);
                }
            }
            return selectedBeats;
        }
    
        public void clearSelectedBeats() {
            for (final JCheckBox checkBox : checkBoxes) {
                checkBox.setSelected(false);
            }
        }
    }
    
    public class BeatBoxView implements InstrumentRowListener {
    
        // ... other super fun GUI stuff
    
        @Override
        public void onCheckBoxSelectionChanged(final String name, final int beat) {
            // Can tell Midi to update its sequence
        }
    
    }
    
    public class BeatBoxViewController implements BeatBoxViewListener {
    
        public BeatBoxViewController(final Midi midi) {
            this.midi = midi;
        }
    
        // ... other controller stuffs
    
        @Override
        public void onCheckBoxSelectionChanged() {
            // Tell midi to update the sequence/track
        }
    
    }
    

    Now, you might be wondering how that helps at all. For one, it abstracts out (to a reasonable extent) all of the work needed to create a row, add the label, handle changes to a checkbox, etc. This also helps with extensibility. Say you wanted to add a button to your app that adds a new instrument (or even one to delete). Previously you would've had to do a bunch of work to update and maintain that 2D array of JCheckBox objects. Now, all you would need to do is add a method to your UI to addInstrument(), which would create a new InstrumentRow instance and you're all set!

    I won't go into much detail on this, but one thing that is also a good practice is to separate your view into an interface and an implementation. The implementation would be specific to the UI framework that you have chosen. For example:

    public interface BeatBoxView {
        public interface BeatBoxViewListener {
            public void onStartButtonClicked();
            public void onPauseButtonClicked();
            public void onResetButtonClicked();
            public void onCheckBoxSelectionChanged(final String name, final int beat);
        }
    
        public void addListener(final BeatBoxViewListener listener);
        public Map<String, List<Integer>> getSelectedBeats();
        public void clearSelectedBeats();
    }
    
    public class BeatBoxViewSwing implements BeatBoxView {
        // Override all of the BeatBoxView interface methods using Swing components
    }
    
  2. Midi class. If Midi is no longer to be sharing the MusicInfo object (since it doesn't exist anymore!), it needs to know when to update the track, and what to update it to. One option (there are probably better ways) is to expose a method such as:

    public class Midi {
    
        public void setTrackFromTicks(final Map<Instrument, List<Integer>> ticksByInstrument) {
            sequence.deleteTrack(track);
            track = sequence.createTrack();
            try {
                makeTrack(ticksByInstrument);
                sequencer.setSequence(sequence);
            } catch( ... ) {
                // Handle that MIDI exception!
            }
            sequencer.setLoopCount(Sequencer.LOOP_CONTINUOSLY);
        }
    
        private void makeTrack(final Map<Instrument, List<Integer>> ticksByInstrument) throws InvalidMidiDataException {
            for (final Entry<Instrument, List<Integer>> entry : ticksByInstrument.entrySet()) {
                for (final Integer tick : entry.getValue()) {
                    makeBeat(entry.getKey().getKey(), tick.intValue());
                }
            }
            // Added to ensure that beat box plays the whole track before it loops again
            currentTrack.add(makeEvent(192, 9, 1, 0, BeatBoxConstants.NUM_BEATS - 1));
        }
    
        private void makeBeat(final int key, final int tick) throws InvalidMidiDataException {
            // Same as before
        }
    
    }
    

BeatBoxConstants

Constant Class (of Interface) are a bad anti-pattern. Instead, we can easily just pass in the number of beats to the constructors!

public Midi(final int numBeats) {
    this.numBeats = numBeats;
    // ... other setup ...
}

public BeatBoxView(final int numBeats) {
    // ...
    for (final Instrument instrument : Instrument.values()) {
        final InstrumentRow row = new InstrumentRow(namePanel, checkBoxPanel, instrument.getDisplay(), numBeats);
        // ... other row stuff ...
    }
    // ...
}

Resulting Code

With all of the design changes in mind, here's what the result could look like:

BeatBox.java

/**
 * BeatBox application.
 */
public class BeatBox {

    private final BeatBoxView gui;
    private final Midi midi;

    /**
     * Constructor for the beat box program
     * @param numBeats The number of beats. Cannot be negative.
     */
    public BeatBox(final int numBeats) {
        if (numBeats < 0) {
            throw new IllegalArgumentException("numBeats cannot be negative"); //$NON-NLS-1$
        }
        midi = new Midi(numBeats);
        gui = new BeatBoxView(numBeats);
        new BeatBoxViewController(gui, midi);
    }

    /**
     * Main method.
     * @param args Runtime arguments.
     */
    public static void main(final String[] args) {
        final int numBeats = 16; // Could easily read this from user input at runtime!
        new BeatBox(numBeats);
    }
}

BeatBoxView.java

/**
 * GUI class is used to produce the graphical representation of the beat box program
 */
public class BeatBoxView implements InstrumentRowListener {

    private final JFrame frame;
    private final List<InstrumentRow> instrumentRows;
    private final List<BeatBoxViewListener> listeners;

    /**
     * Provides methods to notify when the user interacts with the {@link BeatBoxView}.
     */
    public interface BeatBoxViewListener {

        /**
         * Method to be called when the start button is clicked.
         */
        public void onStartButtonClicked();

        /**
         * Method to be called when the pause button is clicked.
         */
        public void onPauseButtonClicked();

        /**
         * Method to be called when the reset button is clicked.
         */
        public void onResetButtonClicked();

        /**
         * Method to be called when the selection state of a check box changes.
         * @param name The name of the instrument to which the check box belongs. Cannot be null or empty.
         * @param beat The beat number of the check box. Cannot be negative.
         */
        public void onCheckBoxSelectionChanged(final String name, final int beat);

    }

    /**
     * Constructor.
     * @param numBeats The number of beats to display. Cannot be negative.
     */
    public BeatBoxView(final int numBeats) {
        if (numBeats < 0) {
            throw new IllegalArgumentException("numBeats cannot be negative"); //$NON-NLS-1$
        }

        instrumentRows = new ArrayList<InstrumentRow>();
        listeners = new ArrayList<BeatBoxViewListener>();

        // Build base frame
        frame = new JFrame("Cyber BeatBox"); //$NON-NLS-1$
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setMinimumSize(new Dimension(600, 350));
        final JPanel panel = new JPanel(new BorderLayout());
        panel.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
        frame.setContentPane(panel);

        // Build instrument labels
        final JPanel namePanel = new JPanel();
        namePanel.setLayout(new BoxLayout(namePanel, BoxLayout.Y_AXIS));
        frame.add(BorderLayout.WEST, namePanel);

        // Build check boxes
        final GridLayout grid = new GridLayout(Instrument.values().length, numBeats);
        grid.setVgap(0);
        grid.setHgap(2);
        final JPanel checkBoxPanel = new JPanel(grid);
        frame.add(BorderLayout.CENTER, checkBoxPanel);

        // Populate the rows
        for (final Instrument instrument : Instrument.values()) {
            final InstrumentRow row = new InstrumentRow(namePanel, checkBoxPanel, instrument.getDisplay(), numBeats);
            row.addListener(this);
            instrumentRows.add(row);
        }

        // Build control buttons
        final JPanel buttonsPanel = new JPanel();
        buttonsPanel.setLayout(new BoxLayout(buttonsPanel, BoxLayout.Y_AXIS));

        final JButton startButton = new JButton("Start"); //$NON-NLS-1$
        startButton.addActionListener(new ActionListener() {
            @SuppressWarnings("synthetic-access")
            @Override
            public void actionPerformed(final ActionEvent e) {
                for (final BeatBoxViewListener listener : listeners) {
                    listener.onStartButtonClicked();
                }
            }
        });
        buttonsPanel.add(startButton);

        final JButton pauseButton = new JButton("Pause"); //$NON-NLS-1$
        pauseButton.addActionListener(new ActionListener() {
            @SuppressWarnings("synthetic-access")
            @Override
            public void actionPerformed(final ActionEvent e) {
                for (final BeatBoxViewListener listener : listeners) {
                    listener.onPauseButtonClicked();
                }
            }
        });
        buttonsPanel.add(pauseButton);

        final JButton resetButton = new JButton("Reset"); //$NON-NLS-1$
        resetButton.addActionListener(new ActionListener() {
            @SuppressWarnings("synthetic-access")
            @Override
            public void actionPerformed(final ActionEvent e) {
                for (final BeatBoxViewListener listener : listeners) {
                    listener.onResetButtonClicked();
                }
            }
        });
        buttonsPanel.add(resetButton);

        frame.add(BorderLayout.EAST, buttonsPanel);

        frame.pack();
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }

    /**
     * Adds a {@link BeatBoxViewListener}.
     * @param listener The {@link BeatBoxViewListener}. Cannot be null.
     */
    public void addListener(final BeatBoxViewListener listener) {
        if (listener == null) {
            throw new IllegalArgumentException("listener cannot be null"); //$NON-NLS-1$
        }
        listeners.add(listener);
    }

    /**
     * Returns a {@link Map} of all selected beats by their {@link Instrument} display string.
     * @return A non-null, possibly empty {@link Map}.
     */
    public Map<String, List<Integer>> getSelectedBeats() {
        final Map<String, List<Integer>> beats = new HashMap<String, List<Integer>>();
        for (final InstrumentRow row : instrumentRows) {
            beats.put(row.getName(), row.getSelectedBeats());
        }
        return beats;
    }

    /**
     * Clears the beat check boxes for all instruments.
     */
    public void clearSelectedBeats() {
        for (final InstrumentRow row : instrumentRows) {
            row.clearSelectedBeats();
        }
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void onCheckBoxSelectionChanged(final String name, final int beat) {
        if (name == null || name.trim().isEmpty()) {
            throw new IllegalArgumentException("name cannot be null or empty"); //$NON-NLS-1$
        }
        if (beat < 0) {
            throw new IllegalArgumentException("beat cannot be negative"); //$NON-NLS-1$
        }
        for (final BeatBoxViewListener listener : listeners) {
            listener.onCheckBoxSelectionChanged(name, beat);
        }
    }

}

BeatBoxViewController.java

/**
 * Controller for the {@link BeatBoxView}.
 */
public class BeatBoxViewController implements BeatBoxViewListener {

    private final BeatBoxView view;
    private final Midi midi;

    /**
     * Constructor.
     * @param view The {@link BeatBoxView}. Cannot be null.
     * @param midi The {@link Midi} instance. Cannot be null.
     */
    public BeatBoxViewController(final BeatBoxView view, final Midi midi) {
        if (view == null) {
            throw new IllegalArgumentException("view cannot be null"); //$NON-NLS-1$
        }
        if (midi == null) {
            throw new IllegalArgumentException("midi cannot be null"); //$NON-NLS-1$
        }
        this.view = view;
        this.midi = midi;
        view.addListener(this);
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void onStartButtonClicked() {
        midi.startSequencer();
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void onPauseButtonClicked() {
        midi.pauseSequencer();
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void onResetButtonClicked() {
        view.clearSelectedBeats();
        midi.pauseSequencer();
        midi.setTrackFromTicks(new HashMap<Instrument, List<Integer>>());
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void onCheckBoxSelectionChanged(final String name, final int beat) {
        final Map<Instrument, List<Integer>> ticksByInstrument = new HashMap<Instrument, List<Integer>>();
        for (final Entry<String, List<Integer>> entry : view.getSelectedBeats().entrySet()) {
            ticksByInstrument.put(Instrument.getByDisplay(entry.getKey()), entry.getValue());
        }
        midi.setTrackFromTicks(ticksByInstrument);
    }

}

Added bonus: You can select new check boxes and the player will now start playing the new track without having to pause and restart the player

InstrumentRow.java

/**
 * Models a row in the UI for an instrument and all of its beats.
 */
public class InstrumentRow {

    private final String name;
    private final int numBeats;
    private final List<JCheckBox> checkBoxes;
    private final List<InstrumentRowListener> listeners;

    /**
     * Provides methods to respond to user interaction with the beat check boxes.
     */
    public interface InstrumentRowListener {
        /**
         * Method to be called when the selection state of a check box changes.
         * @param name The name of the row. Cannot be null or empty.
         * @param beat The beat number for the check box that was changed. Cannot be negative.
         */
        public void onCheckBoxSelectionChanged(final String name, final int beat);
    }

    /**
     * Constructor.
     * @param nameParent The parent {@link JPanel} for all instrument names. Cannot be null.
     * @param checkBoxParent The parent {@link JPanel} for the check boxes. Cannot be null.
     * @param name The name of the row. Cannot be null or empty.
     * @param numBeats The number of beats in the row. Cannot be negative.
     */
    public InstrumentRow(final JPanel nameParent, final JPanel checkBoxParent, final String name, final int numBeats) {
        if (nameParent == null) {
            throw new IllegalArgumentException("nameParent cannot be null"); //$NON-NLS-1$
        }
        if (checkBoxParent == null) {
            throw new IllegalArgumentException("checkBoxParent cannot be null"); //$NON-NLS-1$
        }
        if (name == null || name.trim().isEmpty()) {
            throw new IllegalArgumentException("name cannot be null or empty"); //$NON-NLS-1$
        }
        if (numBeats < 0) {
            throw new IllegalArgumentException("numBeats cannot be negative"); //$NON-NLS-1$
        }
        this.name = name;
        this.numBeats = numBeats;
        checkBoxes = new ArrayList<JCheckBox>();
        listeners = new ArrayList<InstrumentRowListener>();

        nameParent.add(new Label(name));

        for (int i = 0; i < numBeats; ++i) {
            final JCheckBox checkBox = new JCheckBox();
            checkBoxes.add(checkBox);
            checkBox.addActionListener(new ActionListener() {
                @SuppressWarnings("synthetic-access")
                @Override
                public void actionPerformed(final ActionEvent e) {
                    for (final InstrumentRowListener listener : listeners) {
                        listener.onCheckBoxSelectionChanged(name, checkBoxes.indexOf(checkBox));
                    }
                }
            });
            checkBoxParent.add(checkBox);
        }

    }

    /**
     * Adds the listener.
     * @param listener The {@link InstrumentRowListener}. Cannot be null.
     */
    public void addListener(final InstrumentRowListener listener) {
        if (listener == null) {
            throw new IllegalArgumentException("listener cannot be null"); //$NON-NLS-1$
        }
        listeners.add(listener);
    }

    /**
     * Returns the name of the row.
     * @return The non-null, non-empty row name.
     */
    public String getName() {
        return name;
    }

    /**
     * Returns the number of beats in the row.
     * @return The non-negative number of beats in the row.
     */
    public int getNumBeats() {
        return numBeats;
    }

    /**
     * Returns a collection of the beats which have been selected by the user.
     * @return The non-null, possibly empty collection of beats.
     */
    public List<Integer> getSelectedBeats() {
        final List<Integer> selectedBeats = new ArrayList<Integer>();
        for (int i = 0; i < checkBoxes.size(); ++i) {
            if (checkBoxes.get(i).isSelected()) {
                selectedBeats.add(i);
            }
        }
        return selectedBeats;
    }

    /**
     * Deselects all beats in the row.
     */
    public void clearSelectedBeats() {
        for (final JCheckBox checkBox : checkBoxes) {
            checkBox.setSelected(false);
        }
    }

}

Instrument.java

/**
 * Models the available instruments.
 */
public enum Instrument {

    /**
     * The ACOUSTIC_BASS_DRUM.
     */
    ACOUSTIC_BASS_DRUM(35, "Acoustic Bass Drum"), //$NON-NLS-1$
    /**
     * The ACOUSTIC_SNARE.
     */
    ACOUSTIC_SNARE(38, "Acoustic Snare"), //$NON-NLS-1$
    /**
     * The HAND_CLAP.
     */
    HAND_CLAP(39, "Hand Clap"), //$NON-NLS-1$
    /**
     * The CLOSED_HIGH_HAT.
     */
    CLOSED_HIGH_HAT(42, "Closed High Hat"), //$NON-NLS-1$
    /**
     * The OPEN_HIGH_HAT.
     */
    OPEN_HIGH_HAT(46, "Open High Hat"), //$NON-NLS-1$
    /**
     * The LOW_MID_TOM.
     */
    LOW_MID_TOM(47, "Low Mid Tom"), //$NON-NLS-1$
    /**
     * The CRASH_CYMBAL_1.
     */
    CRASH_CYMBAL_1(49, "Crash Cymbal 1"), //$NON-NLS-1$
    /**
     * The HIGH_TOM.
     */
    HIGH_TOM(50, "High Tom"), //$NON-NLS-1$
    /**
     * The COWBELL.
     */
    COWBELL(56, "Cowbell"), //$NON-NLS-1$
    /**
     * The VIBRASLAP.
     */
    VIBRASLAP(58, "Vibraslap"), //$NON-NLS-1$
    /**
     * The HIGH_BONGO.
     */
    HIGH_BONGO(60, "High Bongo"), //$NON-NLS-1$
    /**
     * The OPEN_HIGH_CONGA.
     */
    OPEN_HIGH_CONGA(63, "Open High Conga"), //$NON-NLS-1$
    /**
     * The LOW_CONGA.
     */
    LOW_CONGA(64, "Low Conga"), //$NON-NLS-1$
    /**
     * The HIGH_AGOGO.
     */
    HIGH_AGOGO(67, "High Agogo"), //$NON-NLS-1$
    /**
     * The MARACAS.
     */
    MARACAS(70, "Maracas"), //$NON-NLS-1$
    /**
     * The LONG_WHISTLE.
     */
    LONG_WHISTLE(72, "Long Whistle"); //$NON-NLS-1$

    private final int key;
    private final String display;

    private Instrument(final int key, final String display) {
        if (key < 0) {
            throw new IllegalStateException("key cannot be negative"); //$NON-NLS-1$
        }
        if (display == null || display.trim().isEmpty()) {
            throw new IllegalStateException("display cannot be null or empty"); //$NON-NLS-1$
        }
        this.key = key;
        this.display = display;
    }

    /**
     * Returns an {@link Instrument} which matches the given display string.
     * @param display The display string for which a matching Instrument will be found.
     * @return The matching {@link Instrument} if one exists, otherwise {@code null}.
     */
    public static Instrument getByDisplay(final String display) {
        for (final Instrument instrument : Instrument.values()) {
            if (instrument.getDisplay().equals(display)) {
                return instrument;
            }
        }
        return null;
    }

    /**
     * Returns the key.
     * @return The non-negative key.
     */
    public int getKey() {
        return key;
    }

    /**
     * Returns the display string.
     * @return The non-null, non-empty display string.
     */
    public String getDisplay() {
        return display;
    }

}

Keep in mind these are only my suggestions. Take them as you will, and hopefully at least they will teach a bit about good design patterns!

\$\endgroup\$
  • 3
    \$\begingroup\$ Wow, this answer is incredible. Kudos! \$\endgroup\$ – Phrancis May 5 '16 at 23:28

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.