Morse code string - follow-up

This is probably one of the many follow-ups coming. What I have edited:

• Added equals() and hashCode()

• Added . and , to my Morse code "dictionary"

• Used a Pattern for regex checking

• Edited method names

Concerns:

• Any bad practices in the new code?

• Does it "smell"?

```
import java.util.regex.Pattern;

public class MorseString {

public static final char CHAR_SEPARATOR = ' ';
public static final char WORD_SEPARATOR = '/';
public static final char DOT = '.';
public static final char DASH = '-';

private String string;
private String codeString;

/*
* Constructor that takes the Morse Code as a String as a parameter
*/

public MorseString(String s) {
if(!isValidMorse(s)) {
throw new IllegalArgumentException("s is not a valid Morse Code");
}
if(!s.isEmpty()) {
this.string = translate(s);
} else {
this.string = s;
}
this.codeString = s;
}

/*
* Checks if it is a valid Morse Code
*/

private static final Pattern VALID_MORSE_PATTERN = Pattern.compile(
"(" + Pattern.quote(Character.toString(DASH))
+ "|" + Pattern.quote(Character.toString(DOT))
+ "|" + Pattern.quote(Character.toString(WORD_SEPARATOR)) +
"|\\s)*");

public boolean isValidMorse(CharSequence ch) {
return VALID_MORSE_PATTERN.matcher(ch).matches();
}

/*
* Traslates from Morse in a String to a String
* e.g. ".... .." to "hi"
*/

private String translate(String code) {
String[] words = code.split(Character.toString(WORD_SEPARATOR));
StringBuilder result = new StringBuilder(words.length * words[0].length()); // Rough guess of size
for(String word : words) {
String[] letters = word.trim().split(Character.toString(CHAR_SEPARATOR));
for(String letter : letters) {
result.append(MorseCode.decode(let

I'm referring to this code with the \\ ... replaced from your previous post.

Where are the tests?

I do not see any tests. Where are the tests? You know, tests come first...

Morse: Usage of '/' to separate words?

That's AFAIK not standard Morse, or is it?

Morse - but which?

There are different Morse alphabets. Which one do you use? Mentioning the corresponding spec in a comment would be nice.

Do you want to support multiple Morse alphabets in the long run?

If construction is ambiguous, use a factory method instead.

Ideally code is self-explanatory. Looking at the constructor MorseString.MorseString(String) I cannot tell whether this constructs a MorseString from a String by translating the String argument into Morse code using the Morse alphabet, or whether this constructs a MorseString from a String which already is translated Morse code.

In such cases it is better to have private constructors and static factory methods. You already did it half way, MorseString.parse() is such a factory method.

You could consider declaring private MorseString(String) and provide another factory method instead.

Use final (partially a matter of taste).

The final keyword communicates that a variable is not going to change. I recommend it strongly for fields which do not change, and I recommend it even for all other variables.

Especially, use

public class MorseString {
    private final String string;
    private final String codeString;
}

This makes it more obvious that class MorseString is immutable. BTW the fact that class MorseString is immutable is good!

Premature optimization in translate(String)

Guessing the size of the result for the StringBuilder is premature optimization. And in case performance matters, there's a better way. How significant is words[0].length() really for the length? I would say that words[0] is a bad sample because when it comes to most Indo-German languages like English, French, German etc, there's a big chance that the first word of a sentence is very short like 'A', 'I' or 'You'. Wouldn't it be better to use a constant like, say, 7?

Or, if this is really important for performance, how about a self-tuning, conservative mechanism? Track the average length in a double averageWordLength variable, and use a conservative allocation like new StringBuilder((int) (words.length * (averageWordLength + 3))) if you really want to avoid reallocations of the StringBuilder's internal buffer.

But really, only optimize when you know that this is a performance hot spot.

Bug in translate(String).

I believe that translate(String) cannot process "". That's not very convenient. I'd actually call it a bug.

(I didn't test it, though - did I already ask where are the tests?)

This bug actually complicates the constructor MorseString unnecessarily.

Even if the case s.length() == 0 would need special treatment - then that's because of translate() and therefore responsibility of translate(), not the constructor. Checking s.length() in the constructor is misplaced responsibility.

Use meaningful names.

The parameter s in constructor MorseString is assigned to field codeString. Therefore it makes sense to name it codeString as well, not just s. String s is totally meaningless, whereas String codeString carries a lot of information.

How about calling the things consistently plainText and morseText? Just suggesting, and I think that would be consistent with how such stuff is usually called in the context of coding.

Provide more information in exceptions.

In the constructor, when you detect that s is not a valid Morse string, you do

throw new IllegalArgumentException("s is not a valid Morse Code");

That leaves the programmer clueless what exactly was invalid.

There are two steps about how to improve that.
First of all, include s in the exception message, like this:

throw new IllegalArgumentException("\"" + s + "\"" is not a valid Morse code");

The second step is a bit bigger, you could change isValidMorse(String) from using a regular expression into using a proprietary parser which can give more information.

Over-complicated constructor MorseString

Shouldn't the constructor simply look like this:

public MorseString(String codeString) {
    if (!isValidMorse(codeString))
        throw new IllegalArgumentException("\"" + codeString + "\"" is not a valid Morse code");
    string = translate(codeString);
    this.codeString = codeString;
}

The fact that in a special case s is assigned to both, string and codeString is confusing. It would've been better to use "" instead. But even better of course if translate(String) would simply accept a String s with s.length() == 0.

Avoid overly long methods

translate(String) and parse(String) are a bit lengthy. Consider simplifying and splitting them.

For example, parse(String) contains a special case `if (s.isEmpty()) re

StackExchange Code Review Q#74967, answer score: 10