7
\$\begingroup\$

I made a Morse code translator in Java in which a user inputs a word/words into a text field and the program converts it to Morse code and beeps the Morse code. I have a feeling that my code could be much better, and that it is not efficient enough. Please tell me if there is anything wrong with my code, including efficiency and coding conventions.

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;

import javax.sound.sampled.AudioSystem;
import javax.sound.sampled.Clip;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JTextField;

public class MorseCode extends JFrame implements ActionListener{

char[] alpha = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
        'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
        'w', 'x', 'y', 'z', '1', '2', '3', '4', '5', '6', '7', '8',
        '9', '0', ' ' };

String[] dottie = { ".-", "-...", "-.-.", "-..", ".", "..-.", "--.",
        "....", "..", ".---", "-.-", ".-..", "--", "-.", "---", ".--.",
        "--.-", ".-.", "...", "-", "..-", "...-", ".--", "-..-",
        "-.--", "--..", ".----", "..---", "...--", "....-", ".....",
        "-....", "--...", "---..", "----.", "-----", "|" };

public static void play(String filename)
{
    try
    {
        Clip clip = AudioSystem.getClip();
        clip.open(AudioSystem.getAudioInputStream(new File(filename)));
        clip.start();
    }
    catch (Exception exc)
    {
        exc.printStackTrace(System.out);
    }
}


JTextField wordField;
JTextField morseField;
JButton convertButton;
public MorseCode() {
    getContentPane().setLayout(null);
    setSize(300, 300);
    setVisible(true);
    setDefaultCloseOperation(EXIT_ON_CLOSE);

    JLabel lblEnglish = new JLabel("English");
    getContentPane().add(lblEnglish);
    lblEnglish.setBounds(10, 11, 46, 14);

    wordField = new JTextField();
    getContentPane().add(wordField);
    wordField.setBounds(10, 36, 264, 71);
    wordField.setColumns(10);

    convertButton = new JButton("Convert");
    getContentPane().add(convertButton);
    convertButton.addActionListener(this);
    convertButton.setBounds(98, 118, 89, 23);

    JLabel lblMorseCode = new JLabel("Morse Code");
    getContentPane().add(lblMorseCode);
    lblMorseCode.setBounds(10, 161, 98, 14);

    morseField = new JTextField();
    getContentPane().add(morseField);
    morseField.setBounds(10, 186, 264, 64);
    morseField.setColumns(10);
}



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



@Override
public void actionPerformed(ActionEvent event) {
    if(event.getSource() == convertButton) {
        String input = wordField.getText();
        String morse = "";
        char[] inputArray = input.toCharArray();
        for (int i = 0; i < inputArray.length; i++) {
            for (int j = 0; j < alpha.length; j++) {
                if(inputArray[i] == alpha[j]) {
                    morse += dottie[j] + " ";
                    if(alpha[j] == ' ') {
                        try {
                            Thread.sleep(10);
                        } catch (InterruptedException e) {
                            e.printStackTrace();
                        }
                    } else {
                        play("C:/Users/Dylan Black/Desktop/Dylan B./Morse Code/src/" + alpha[j] + ".wav");
                    }
                    try {
                        Thread.sleep(500);
                    } catch (InterruptedException e) { 
                        e.printStackTrace();
                    }
                }
            }
        }
        morseField.setText(morse);
    }

   }
}

Conversion method:

@Override
public void actionPerformed(ActionEvent event) {
    if(event.getSource() == convertButton) {
        String input = wordField.getText();
        String morse = "";
        char[] inputArray = input.toCharArray();
        for (int i = 0; i < inputArray.length; i++) {
            for (int j = 0; j < alpha.length; j++) {
                if(inputArray[i] == alpha[j]) {
                    morse += dottie[j] + " ";
                    if(alpha[j] == ' ') {
                        try {
                            Thread.sleep(10);
                        } catch (InterruptedException e) {
                            e.printStackTrace();
                        }
                    } else {
                        play("C:/Users/Dylan Black/Desktop/Dylan B./Morse Code/src/" + alpha[j] + ".wav");
                    }
                    try {
                        Thread.sleep(500);
                    } catch (InterruptedException e) { 
                        e.printStackTrace();
                    }
                }
            }
        }
        morseField.setText(morse);
    }

Play method:

public static void play(String filename)
{
    try
    {
        Clip clip = AudioSystem.getClip();
        clip.open(AudioSystem.getAudioInputStream(new File(filename)));
        clip.start();
    }
    catch (Exception exc)
    {
        exc.printStackTrace(System.out);
    }
}

Constructor:

public MorseCode() {
    getContentPane().setLayout(null);
    setSize(300, 300);
    setVisible(true);
    setDefaultCloseOperation(EXIT_ON_CLOSE);

    JLabel lblEnglish = new JLabel("English");
    getContentPane().add(lblEnglish);
    lblEnglish.setBounds(10, 11, 46, 14);

    wordField = new JTextField();
    getContentPane().add(wordField);
    wordField.setBounds(10, 36, 264, 71);
    wordField.setColumns(10);

    convertButton = new JButton("Convert");
    getContentPane().add(convertButton);
    convertButton.addActionListener(this);
    convertButton.setBounds(98, 118, 89, 23);

    JLabel lblMorseCode = new JLabel("Morse Code");
    getContentPane().add(lblMorseCode);
    lblMorseCode.setBounds(10, 161, 98, 14);

    morseField = new JTextField();
    getContentPane().add(morseField);
    morseField.setBounds(10, 186, 264, 64);
    morseField.setColumns(10);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ related SO Q&A about serialVersionUID \$\endgroup\$ – Arjan Aug 6 '16 at 17:52
  • 1
    \$\begingroup\$ Why do you have so many audio clips, and where do they come from? Couldn't you synthesize the sounds using MIDI instead? \$\endgroup\$ – 200_success Aug 18 '16 at 16:36
  • \$\begingroup\$ You could use the method tone(frequency, duration, volume) from stackoverflow.com/a/6700039/638028 . This gives you the flexibility to vary the output speed at runtime, and saves you having to keep all these wav files around. \$\endgroup\$ – Klitos Kyriacou Aug 18 '16 at 23:34
  • \$\begingroup\$ Also, I'm not sure your timings are correct. You seem to have a 500 ms wait between letters, with only an addition 10 ms between words. The space between letters should be a dash duration, and between words should be three times the dash duration. \$\endgroup\$ – Klitos Kyriacou Aug 18 '16 at 23:38
  • \$\begingroup\$ You can cheat by putting the Morse code for each letter into a single byte instead of a String, by treating the dots and dashes as the bit values 0 and 1. I'm just mentioning it as a point of interest. Although that makes it slightly more efficient, I wouldn't recommend it because it makes the code harder to understand. It may be useful if you are really short of memory, for example if targeting a cheap mobile phone with only a small amount of RAM. \$\endgroup\$ – Klitos Kyriacou Aug 18 '16 at 23:43
7
+50
\$\begingroup\$

One thing that catches my eye is how you have two arrays of chars/strings mapped to each other through related index locations. I did this myself back in the day, and it quickly became a nightmare to maintain. In your case, you should create a HashMap instead. I would expect your code to look like:

HashMap<char, String> charToCodeMap = new HashMap<char, String>();
charToCodeMap.put('a', ".-");
// Initialize here...

Now, instead of iterating through both lists for every character, you can just get the character you need to translate and call, for example, String codeForA = charToCodeMap.get('a'). This will (theoretically) reduce the time needed to run your code drastically, as map access is \$O(1)\$ (unless there are hash collisions), while iterating through a list is \$O(n)\$. However, as Klitos Kyriacou noted in the comments, the actual performance and theoretical performance are not necessarily the same.

\$\endgroup\$
  • \$\begingroup\$ Although the use of a Map makes the code clearer, I doubt it would make it faster. The time complexities that you quote are correct, but they don't tell the whole story. These are only useful when thinking about scalability. For this small collection of letters and numbers, I would expect an array to be much faster than a HashMap, since the data is held in contiguous memory which is going to be in the processor's L1 cache; whereas a HashMap would have to box & unbox between char and Character and use indirection. Don't assume better performance unless you measure. \$\endgroup\$ – Klitos Kyriacou Aug 18 '16 at 16:52
1
\$\begingroup\$

You can gain some millis by replacing your for loop and matching to select find the alphaby some ASCII maths. You just have to place 0 before 1 in your arrays :

private static final int INDEX_OF_ZERO = ('z'-'a')+1;

/*for (int j = 0; j < alpha.length; j++) {
  if(inputArray[i] == alpha[j]) {*/

if ( Character.isAlphabetic(c) ) {
  return DOTTIE[c-'a'];
} else if (Character.isDigit(c) ) {
  return DOTTIE[INDEX_OF_ZERO+(c-'0')];
} else if ( ' '==c ) {
  return DOTTIE[DOTTIE.length-1];
} else {
  return null;
}

But the longest task is to load all your wav files. The java.util.concurrent.ExecutorService can help here. You have to create two loops:

ExecutorService executor = Executors.newFixedThreadPool(2);
List<Future<Runnable>> futures = new ArrayList<>();

1 the first one submit Callable<Runnable> to the executor and add the Future to a list. The Callable<Runnable> prepare the Clip and return a runnable that start the Clip.

for (int i = 0; i < partition.length(); i++) {
  char character = partition.charAt(i);
  futures.add(executor.submit(()->prepare(character)));
}
executor.shutdown();

2 then you loop on your list of Future and get them to run the Runnable.

for (Future<Runnable> future : futures) {
  try {
    future.get().run();
    Thread.sleep(500);
  } catch (InterruptedException | ExecutionException e) {
    e.printStackTrace();
  }
}

By doing that, you are able to load your wav on the background while playing the beginning. prepare will just return a Runnable that sleep for ' ' or load the wav and return a runnable that start it

File file = new File(directory, character + ".wav");
Clip clip = AudioSystem.getClip();
clip.open(AudioSystem.getAudioInputStream(file));
return ()->{
    clip.start();
};

Finally, you can use SwingWorker, this is not a performance improvement but it give the feeling of that. I have made a try with one worker to translate the words and update morseField when done and another to play the sounds and enable the convertButton when done.

\$\endgroup\$
  • \$\begingroup\$ This works with ASCII, but Character.isAlphabetic() and isDigit() can both return true for various non-ASCII Unicode characters and then you'll get an ArrayIndexOutOfBoundsException. \$\endgroup\$ – Klitos Kyriacou Aug 19 '16 at 12:44
  • \$\begingroup\$ True. Then replace Character.isAlphabetic() with c>='a' && c<='z' and Character.isDigit by c>=0 && c<=9. \$\endgroup\$ – gervais.b Aug 19 '16 at 13:10
1
\$\begingroup\$

I think you should encapuslate any given "morse code" (i.e. a ., -, or |) in a distinct class (or in this case, an enum). This way you aren't bound to any particular representation of your morse code. I did something like this

public enum Morse {
    LONG,
    SHORT,
    PAUSE;

    public String toString() {
        switch (this) {
            case LONG:
                return "-";
            case SHORT:
                return ".";
            case PAUSE:
                return "|";
        }
        throw new IndexOutOfBoundsException();
    }
}

static HashMap<char, Morse[]> morseCode = new HashMap<char, Morse[]>();

static {
    morseCode.put('a', {Morse.SHORT, Morse.LONG});
    // etc
}

Then I think you should make a separate class that handles playing the sounds. I also don't see much point in having a ton of midi files that you load - this is unnecessarily slow, and just adds a lot of extra files to keep track of. Generating midi sounds is relatively easy in Java (I haven't tested this, so be warned), but you should be able to do something like this

public class MorseSound {
    Synthesizer synth;
    MidiChannel mc;
    Instrument instrument;

    static final int longDuration = 500;
    static final int shortDuration = 250;
    static final int pauseDuration = 1500;

    MorseSound() {    
        synth = MidiSystem.getSynthesizer();
        synth.open();
        mc = synth.getChannels()[0];
        instrument = synth.getDefaultSoundBank().getInstruments()[0];
        synth.loadInstrument(instrument);          
    }

    public void play(Morse code) {
        switch (code) {
            case LONG:
                mc.noteOn(60, 500);
                Thread.Sleep(longDuration);
                mc.noteOff(60);
                break;
            case SHORT:
                mc.noteOn(60, 500);
                Thread.Sleep(shortDuration);
                mc.noteOf(60);
                break;
            case PAUSE:
                Thread.Sleep(pauseDuration);
                break;
        }
    }
}

Then while playing it you just do something like this:

for (char character : input) {
    for (Morse code : morseCode.get(Character(character).toLowerCase())) {
        sound.play(code);
    }
}
\$\endgroup\$

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.