Morse code converter

Question

I wanted an uncomplicated project to get myself started again. Although this works I was wondering if it's the best approach, especially when it comes to validation parts? It seems to me that I'm potentially looping quite a while, and this may not be the best or even common practice. I was hoping the CR community could enlighten me on this matter, as well as any other shortcomings this suffers from, as it has graciously done thus far.

public enum Morse {
    MC_1(".----", '1'), MC_2("..---", '2'),MC_3("...--", '3'),

    MC_4("....-", '4'), MC_5(".....", '5'), MC_6("-....", '6'),

    MC_7("--...", '7'), MC_8("---..", '8'), MC_9("----.", '9'),

    MC_0("-----", '0'), MC_A(".-", 'A'), MC_B("-...", 'B'),

    MC_C("-.-.", 'C'), MC_D("-..", 'D'), MC_E(".", 'E'),

    MC_F("..-.", 'F'), MC_G("--.", 'G'), MC_H("....", 'H'),

    MC_I("..", 'I'), MC_J(".---", 'J'), MC_K("-.-", 'K'),

    MC_L(".-..", 'L'), MC_M("--", 'M'), MC_N("-.", 'N'),

    MC_O("---", 'O'), MC_P(".--.", 'P'), MC_Q("--.-", 'Q'),

    MC_R(".-.", 'R'), MC_S("...", 'S'), MC_T("-", 'T'),

    MC_U("..-", 'U'), MC_V("...-", 'V'), MC_W(".--", 'W'),

    MC_X("-..-", 'X'), MC_Y("-.--", 'Y'), MC_Z("--..", 'Z'),
    MC_SPACE("/", ' ');

    public final String morse;
    public final char code;

    Morse(String morse, char code) {
        this.morse = morse;
        this.code = code;
    }
}

This contains the Main class:

import java.util.Scanner;

public class MorseCodeConverter {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        System.out.print("Enter String to be converted to Morse Code: ");
        String userInput = input.nextLine();
        System.out.println(morseConversion(userInput));
    }

    public static String morseConversion(String s) {
        String ms = s.toUpperCase().trim(); // modified string
        StringBuilder result = new StringBuilder();

        for (int i = 0; i < ms.length(); i++) {
            isValid(ms.charAt(i));

            for (Morse c : Morse.values()) {
                if (ms.charAt(i) == ' ' && ms.charAt(i - 1) == ' '); // Ignore consecutive spaces
                else if (ms.charAt(i) == c.code) {
                    result.append(c.morse);
                    if (i != ms.length() - 1) { result.append((" ")); }
                }
            }
        }
        return result.toString();
    }

    public static boolean isValid(char c) {
        for (Morse m : Morse.values()) {
            if (c == m.code) return true;
        }
        throw new IllegalArgumentException("Invalid Entry: " + c + ". Non-space input may only be: 0-9, A-Z, or a-z");
    }
}

Answer 1

MC_1(".----", '1'),
...

This is pretty repetitive. Unfortunately, you can't call the object simply 1, but you can do better:

MC_1(".----"),
...
Morse(String morse) {
    this.morse = morse;
    this.code = name().charAt(name().length - 1);
}

This assumes, you can use all needed chars as a part of the name, which is not the case. So let's handle it by adding another constructor (your original and using

MC_SPACE("/", ' ');

just like you did. But I'm not sure at all, if using an enum is the way to go. What about using a simple Map<String, char> and another the other way round or better a BiMap?

Encapsulate it in a class Codec and let the class do more work.

---

public static String morseConversion(String s) {
    String ms = s.toUpperCase().trim(); // modified string

As this is nothing but a cleanup of s, I'd simply modify it. There's a rule about never modifying arguments, but it's wrong. Ask yourself:

• What harm can happen by cleaning up s directly?
• What harm can happen by having both s and sm?

The answers should make it perfectly clear in this case.

---

for (int i = 0; i < ms.length(); i++) {
    isValid(ms.charAt(i));

According to the name, isValid is a predicate. It returns a boolean to be used elsewhere. Your method should be called checkIsValid or alike. Or make it to a predicate and use checkArgument.

---

Validation is fine, but not here. What you do is determining if the following loop will succeed. So you're doing all the work twice.

---

for (Morse c : Morse.values()) {
     if (ms.charAt(i) == ' ' && ms.charAt(i - 1) == ' ');

This test is independent of c, so it does not belong to the loop.

What's worse: if (..); else if is a really terrible way if writing conditions. It's pretty confusing. If you need something like this, write

     if (ms.charAt(i) == ' ' && ms.charAt(i - 1) == ' ') {
         // Do nothing. Ignoring consecutive spaces.
     } else if (ms.charAt(i) == c.code) {

---

It seems to me that I'm potentially looping quite a while, and this may not be the best or even common practice.

That's exactly what a Map is for. Feed it with the char. and it gives you the String. No loop needed.