Text-to-Morse code converter

Question

I created a morse converter from a .txt file.

I'm learning java and I created a simple morse converter. Convert only A to Z and 0 to 9 chars. Space char remain for split words in morse-code. At the beginning I used a big switch for any letters or number but it was no good.

public static String toMorse(byte[] bytes) {
    String text = new String(bytes);
    String morseText = "";
    text = text.toLowerCase();
    String[] morseChar
            = {".-",
                "-...",
                "-.-.",
                "-..",
                ".",
                "..-.",
                "--.",
                "....",
                "..",
                ".---",
                "-.-",
                ".-..",
                "--",
                "-.",
                "---",
                ".--.",
                "--.-",
                ".-.",
                "...",
                "-",
                "..-",
                "...-",
                ".--",
                "-..-",
                "-.--",
                "--.."};
    String[] morseNumber
            = {".----",
                "..---",
                "...--",
                "....-",
                ".....",
                "-....",
                "--...",
                "---..",
                "----.",
                "-----"};

    for (int i = 0; i < text.length(); i++) {
        if (text.charAt(i) >= 'a' && text.charAt(i) <= 'z') {
            morseText = morseText + morseChar[text.charAt(i) - 'a'];
        } else if (text.charAt(i) >= '0' && text.charAt(i) <= '9') {
            morseText = morseText + morseNumber[text.charAt(i) - '0'];
        } else if (text.charAt(i) == ' ') {
            morseText = morseText + text.charAt(i);
        }
    }
    return morseText;
}

Answer 1

One more review, where I'd like to point out two Java pitfalls you should avoid in professional code.

byte[] array to represent text

Your public static String toMorse(byte[] bytes) { ... } method signature gets the input as byte array and only internally creates a String out of that array.

That's dangerous, as that String(byte[] bytes) constructor is documented as (emphasis mine):

public String(byte[] bytes)

Constructs a new String by decoding the specified array of bytes using the platform's default charset. [...]

That means that the same byte array will produce different results when running under different operating systems, as even nowadays there are still many different character encodings in use all over the world and all over the various operating systems.

A computational method like your morse converter should have a String argument instead of a byte[] array, then it's run-anywhere reproducible. If necessary, have your file I/O methods handle character encodings.

Rule of thumb: to pass around things that represent text, use String.

Repeated String appending

In your loop, you build the output string by repeatedly doing lines like

morseText = morseText + morseChar[text.charAt(i) - 'a'];

That's ok if you know the text is rather short, but it will become quite slow for strings with thousands of characters.

Why? Every time the machine executes such a line, it creates a completely new morseText string, copying all the characters from the old morseText, plus the additional morse snippet. So, the first morse snippets get copied over and over and over again. To avoid that, use a single StringBuilder, and in every loop iteration, append() the morse snippet to that StringBuilder, and only after the loop, convert the StringBuilder to the String you want to return.

Answer 2

Hello and Welcome to Code Review!

Just one suggestion:

Your for loop at the bottom performs the same method calls several times. Its performance could be improved if you slightly retooled your loop like so:

int charCount = text.length;
for (int i = 0; i < charCount; i++) {
        char currentChar = text.charAt(i);
        if (currentChar >= 'a' && currentChar <= 'z') {
            morseText = morseText + morseChar[currentChar - 'a'];
        } else if (currentChar >= '0' && currentChar <= '9') {
            morseText = morseText + morseNumber[currentChar - '0'];
        } else if (currentChar == ' ') {
            morseText = morseText + currentChar;
        }
    }
}

By pulling these method calls up as variables, the number of calls your loop makes is significantly decreased. Your version calls text.length at the top of each iteration of the loop, and text.charAt(i) eight up to 6 times per loop. With this change you would call length once total, and text.charAt only once per loop.

Answer 3

You might consider using StringBuilder for better performance and call text.charAr(i) only once by making it a local variable. Also, I'd rather change the parameter to String, so you do not run into decoding issues.

public static void main(String[] args) throws IOException {
    String filePath = "/tmp/test";
    String text =  new String(Files.readAllBytes(Paths.get(filePath)));
}

public static String toMorse(String text) {
    StringBuilder morseText = new StringBuilder();
    text = text.toLowerCase();
    String[] morseChar =  // see above, didn't check
    String[] morseNumber ={
            "-----", //0 has to be the first one. programmers start counting by 0!
            ".----",
            "..---",
            "...--",
            "....-",
            ".....",
            "-....",
            "--...",
            "---..",
            "----."};

    for (int i = 0; i < text.length(); i++) {
        char character = text.charAt(i);
        if (character >= 'a' && character <= 'z') {
            morseText.append(morseChar[character - 'a']);
        } else if (character >= '0' && character <= '9') {
            morseText.append(morseNumber[character - '0']);
        } else if (character == ' ') {
            morseText.append(character);
        }
    }
    return morseText.toString();
}

Answer 4

I'd suggest adding a comment mentioning how the order of values in the arrays morseChar and morseNumber are important for the conversion.

At first glance I was confused how the conversion was being done.

You should declare morseChar and morseNumber as final class variables so they are easily visible & can be used in other methods.

If you may add symbols to the code, a class or enum containing the MorseCode and english character can be added. This also makes the opposite conversion very easy.

 // Note: Simply add numbers and symbols to this enum. 
 // The Order does not matters.
 public enum MorseCharacter
 {
     A(".-", 'a'),
     B("-...", 'b'),
     C("-.-.", 'c'),
     D("-..", 'd'),
     E(".", 'e'),
     F("..-.", 'f'),
     G("--.", 'g'),
     H("....", 'h'),
     I("..", 'i'),
     J(".---", 'j'),
     K("-.-", 'k'),
     L(".-..", 'l'),
     M("--", 'm'),
     N("-.", 'n'),
     O("---", 'o'),
     P(".--.", 'p'),
     Q("--.-", 'q'),
     R(".-.", 'r'),
     S("...", 's'),
     T("-", 't'),
     U("..-", 'u'),
     V("...-", 'v'),
     W(".--", 'w'),
     X("-..-", 'x'),
     Y("-.--", 'y'),
     Z("--..", 'z');

     private final String morseCode;
     private final char character;
     private static final MorseCharacter[] ALL_VALUES = values();

     MorseCharacter(String morseCode, char character)
     {
         this.morseCode = morseCode;
         this.character = character;
     }

     public String getMorseCode()
     {
         return morseCode;
     }

     public char getCharacter()
     {
         return character;
     }

     public static MorseCharacter valueOf(char character)
     {
         for (MorseCharacter morseChar : ALL_VALUES)
         {
             if (morseChar.getCharacter() == character)
             {
                 return morseChar;
             }
         }

         throw new RuntimeException("Morse character not found: " + character);
     }
 }

 public static String toMorse(String text) 
 {
     String morseText = "";
     text = text.toLowerCase();

     for (int i = 0; i < text.length(); i++) 
     {
         morseText += MorseCharacter.valueOf(text.charAt(i)).getMorseCode();
     }
     return morseText;
 }

Answer 5

I add comments in the arrays

String[] morseChar
            = {".-",    //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
    String[] morseNumber
            = {".----", //0
                "..---",//1
                "...--",//2
                "....-",//3
                ".....",//4
                "-....",//5
                "--...",//6
                "---..",//7
                "----.",//8
                "-----"};//9

Answer 6

You have defined the morse code mapping arrays as method variables. This means that they are allocated from stack every time you call the method. This is fairly ineficient. You should move them to class constants:

private static final String[] MORSE_CHAR
        = {".-",
            "-...",
    ...

private static final String[] MORSE_NUMBER
        = {".----",
            "..---",
    ...


public static String toMorse(byte[] bytes) {
    ...
}

But we can make it better. Morse conversion in this case is fundamentally mapping characters to strings, so why don't we encapsulate the conversion to a separate class to comply with the single responsibility principle? We can take advantage of Unicode being compatible with ASCII and define the mappings as a simple array where the array index represents a ASCII code points and the value in the array represents the Morse code that represents the ASCII character. And look at that, Java has an interface for us... :) We have to make some special cases for word separators, since we are using data types that cannot fully represent the intricacies of Morse code (e.g. space is mapped to space). Ideally we would map to dots, dashes and delays of different lengths (or if we want to split hairs, a dash is just a dot that lasts three times as long, so the mapping would be to sounds and silences of certain lengths).

A really fancy solution would return Optional<String> instead of a nullable String.

import java.util.function.IntFunction;

/**
 * Convert Unicode code points to morse code strings.
 * Code points that can not be represented in morse are retuned as
 * null. Space is returned as space.
 */
public class CodePointToMorseStringConverter implements IntFunction<String> {

    public static final IntFunction<String> INSTANCE = new CodePointToMorseStringConverter();

    private static final String[] ASCII_TO_MORSE = {
        null, // 0
        ... // 30 nulls removed for brevity
        null, // 31
        "", // 32, space
        "-.-.--", // 33, !
        ".-..-.", // 34, "
        null, // 35
        null, // 36
        null, // 37
        null, // 38
        ".----.", // 39, '
        "-.--.-", // 40, ( (brackets)
        "-.--.-", // 41, ) (brackets)
        null, // 42
        null, // 43
        "--..--", // 44, ,
        "-....-", // 45, -
        ".-.-.-", // 46, .
        "-..-.", // 47, / (fraction bar)
        "-----", // 48, 0
        ".----",
        "..---",
        "...--",
        "....-",
        ".....",
        "-....",
        "--...",
        "---..",
        "----.", // 57, 9
        "---...", // 58, :
        null, // 59
        null, // 60
        "-...-", // 61, =
        null, // 62
        "..--..", // 63, ?
        ".--.-.", // 64, @
        ".-", // 65, A
        "-...",
        "-.-.",
        "-..",
        ".",
        "..-.",
        "--.",
        "....",
        "..",
        ".---",
        "-.-",
        ".-..",
        "--",
        "-.",
        "---",
        ".--.",
        "--.-",
        ".-.",
        "...",
        "-",
        "..-",
        "...-",
        ".--",
        "-..-",
        "-.--",
        "--..", // 90, Z
        null, // 91
        null, // 92
        null, // 93
        null, // 94
        null, // 95
        null, // 96
        ".-", // 97, A
        "-...",
        "-.-.",
        "-..",
        ".",
        "..-.",
        "--.",
        "....",
        "..",
        ".---",
        "-.-",
        ".-..",
        "--",
        "-.",
        "---",
        ".--.",
        "--.-",
        ".-.",
        "...",
        "-",
        "..-",
        "...-",
        ".--",
        "-..-",
        "-.--",
        "--.." // 122, Z
    };

    @Override
    public String apply(int i) {
        if (i >= 0 && i <= ASCII_TO_MORSE.length) {
            return ASCII_TO_MORSE[i];
        } else {
            return null;
        }
    }
}

Conversion then becomes a stream operation. We can wrap this into a method if we like. Or even make a Function<String, String>. But that'll be an exercise for later.

String text = "Hello world!";
String morse = text.codePoints()
    .mapToObj(CodePointToMorseStringConverter.INSTANCE)
    .filter(Objects::nonNull)
    .collect(Collectors.joining(" "));