7
\$\begingroup\$

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");
    }
}
\$\endgroup\$
10
\$\begingroup\$
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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would also recommend Map, as that would open up the possibility to extend / modify the Morse Alphabet. There are specific Swedish characters for example that is not in the original enum. \$\endgroup\$ – Simon Forsberg Dec 15 '14 at 10:48

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.