4
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Your mapping for numbers is incorrect, BTW. \$\endgroup\$ – TorbenPutkonen Apr 9 '19 at 6:06
  • \$\begingroup\$ Apart from some inefficiency, pointed out in multiple answers, this code is incorrect. Please fix it before the question gets put on hold as off-topic. \$\endgroup\$ – CiaPan Apr 9 '19 at 11:13
7
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. I improved my code. \$\endgroup\$ – Dalu_21 Apr 5 '19 at 18:47
  • \$\begingroup\$ Minor correction: the original loop invokes text.charAt(i) up to 6 times per iteration, not 8. Those are 5 times per all if-s and once in the conditional branch (it will never execute all three branches in a single iteration!) And those 5 only if the analyzed character is not a letter nor a digit. So a text consisting of, say, 70% letters, 10% digits and 20% spaces will require, on average, 3.8 calls to text.charAt(i) per character loop. Anyway it's still worth reducing to 1. Upvoted. \$\endgroup\$ – CiaPan Apr 9 '19 at 9:56
2
\$\begingroup\$

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();
}
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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;
 }
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why the setter methods in an enum? Haven't seen that before. \$\endgroup\$ – TomG Apr 7 '19 at 6:27
  • \$\begingroup\$ @TomG Good point, updated \$\endgroup\$ – dustytrash Apr 7 '19 at 14:55
  • 2
    \$\begingroup\$ This is fairly inefficient. While finding the morse code is already O(N) the enum.values() creates a clone of the internal array. So you'll be doing a lot of repetitive memory allocation. I would use Map instead or use an array like OP did (but hidden behind a dedicated obect that does error checking). \$\endgroup\$ – TorbenPutkonen Apr 8 '19 at 7:11
  • \$\begingroup\$ Also, naming the field as asciiCharacter is a bit misleading as the value it represents is already a decoded Java character, not an ASCII encoded binary value. \$\endgroup\$ – TorbenPutkonen Apr 8 '19 at 8:32
  • 1
    \$\begingroup\$ You're stil doing O(N) for an O(1) job. How much would you have to extend an enumeration with non-standard functionality to realize an enum is not a good tool for this job? \$\endgroup\$ – TorbenPutkonen Apr 9 '19 at 5:03
-2
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The comments for the numbers are incorrect. \$\endgroup\$ – TorbenPutkonen Apr 9 '19 at 6:05
  • 1
    \$\begingroup\$ @TorbenPutkonen The comments are correct, but the data are wrong. \$\endgroup\$ – CiaPan Apr 9 '19 at 10:31
  • \$\begingroup\$ @CiaPan I don't think that they work like that... \$\endgroup\$ – TorbenPutkonen Apr 9 '19 at 10:51
  • \$\begingroup\$ @TorbenPutkonen Like what? \$\endgroup\$ – CiaPan Apr 9 '19 at 11:00
-2
\$\begingroup\$

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(" "));
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Could you convert Morse to Text this way? \$\endgroup\$ – dustytrash Apr 9 '19 at 13:14
  • \$\begingroup\$ Sure. 987654321 \$\endgroup\$ – TorbenPutkonen Apr 10 '19 at 0:56

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.