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()) return new MorseString("");. Ideally algorithms are written in a way that such special cases are implicit (and if they are developed with TDD/TPP, they usually end up this way automatically).
Look at this sum(int... ops) method:
public static int sum(final int... operands) {
int sum = 0;
for (final int operand : operands)
sum += operand;
return sum;
}
No special case for operands.length == 0.
Handling the special cases specially makes algorithmic functions less robust (if we're not speaking of recursive algorithms): You remove a potential test case from testing the core algorithm.
Redundant null check in equals()
In equals(), the check if (obj == null) is redundant. It is defined, guaranteed and (hopefully) well-known that x instanceof Y with x == null always evaluates false.
EDIT: The equals() could be as simple as this, given that codeString and string are never null:
public boolean equals(final Object obj) {
return obj instanceof MorseString ? equals((MorseString) obj) : false;
}
public boolean equals(final MorseString obj) {
return codeString.equals(obj.codeString) && string.equals(obj.string);
}
Or if you want to go without a second overloaded equals(), like this:
public boolean equals(final Object obj) {
if (!(obj instanceof MorseString))
return false;
final MorseString o = (MorseString) obj;
return codeString.equals(o.codeString) && string.equals(o.string);
}
My personal preference is on the variant with the two equals() because the individual methods are shorter and pure single-statement expression functions.
Just compare this with your isValidMorse(CharSequence) method. This is how functions ideally look like. Your isValidMorse(CharSequence) method is really nice.
Inconsistent null check in equals() and hashCode()
The other null check if (codeString == null) actually is inconsistent. The constructor is (currently) written in a way that codeString can never be null. The constructor takes String s, invokes s.isEmpty() without null-check, which means if this succeeds, s is guaranteed to not be null, then assignes codeString = s.
Consider using Objects.hashCode() and Objects.hash() for hashCode().
The expression (obj == null ? 0 : obj.hashCode()) can be replaced by Objects.hashCode(obj).
However, your hashCode() could be as simple as this:
@Override
public int hashCode() {
return Objects.hash(codeString, string);
}
enums are for programmers. Plus, put abstraction in code, details in data.
I wouldn't use an enum for the job of translating between plain text and morse text.
Also, decode and encode are unnecessarily slow. They are O(n) with n being the size of the Morse alphabet.
A Map<Character, String> / Map<String, Character> would be O(log(n)), and it would be more maintainable. As soon as someone sees Map they think "Oh, lookup. Clear." Whereas the for-loop has to be read and understood in order to understand that it's a lookup.
If performance matters, you can actually have O(C) by using an array with the character as index and special values like null or -1 to denote invalids / gaps in the array.
This works in both directions. A String which represents a single character from the Morse alphabet and therefore contains only . and - can actually be represented as a binary digit by converting '.' into 0 and '-' into 1. You need a known start bit in order to distinguish whether the code starts with 0 or 1. That way you can convert the String into a small int number and use that for looking up the character.
EDIT:
Violation of SRP - Single Responsibility Principle regarding plausibility check. Consequence: Inconsistent MorseString.parse(String) vs enum MorseCode.
While your enum MorseCode allows for latin characters, digits, comma and period, MorseString.parse(String) will still reject such strings because of the guard if (!s.matches("[\\s\\dA-Za-z]*")).
The root cause is the violation of the SRP - Single Responsibility Principle. Both classes / methods, MorseString.parse(String) and MorseCode.encode(char), are responsible for the plausibility check. The responsibility of the plausibility check has been duplicated, it's not in a single place, and now it has become inconsistent.
The guard actually is entirely redundant. MorseString.parse(String) could purely take the responsibility of dealing with multiple characters, i.e. taking care of the loop.
Avoid break and continue if possible.
break and continue are goto in disguise, i.e. a violation of structured programming. Okay, sometimes we need them. But in the case of MorseString.parse(String), the continue can easily be avoided by using an else.
Possible violation of the SRP - Single Responsibility Principle between MorseString.parse(String) and MorseCode.encode(char) regarding conversion.
MorseCode seems to be responsible for converting all characters except some, like ' ' (SP).
Consider how much simpler MorseString.parse(String) would be if MorseCode would also take care of ' '.
Actually, MorseString.parse(String) should be as simple as this:
public static MorseString parse(final String plainText) {
final StringBuilder result = new StringBuilder();
for (final char c : plainText.toCharArray())
result.append(MorseCode.encodeForText(c)).append(CHAR_SEPARATOR);
return new MorseString(result.toString().trim());
}
I refer to a not yet existent method encodeForText which would return Strings which would be one pause length short to not have overly long pause lengths for those Morse alphabet code words which contain pauses at their ends.
