5
\$\begingroup\$

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(letter));
            }
            result.append(CHAR_SEPARATOR);
        }
        return result.toString().substring(0, result.length() - 1);
    }

    public static MorseString parse(String s) {
        // ...
    }

    /*
     * Returns the code as a String
     * e.g. if the object represents "hi" in Morse, it returns ".... .."
     */

    @Override
    public String toString() {
        return codeString;
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result
                + ((codeString == null) ? 0 : codeString.hashCode());
        result = prime * result + ((string == null) ? 0 : string.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (!(obj instanceof MorseString))
            return false;
        MorseString other = (MorseString) obj;
        if (codeString == null) {
            if (other.codeString != null)
                return false;
        } else if (!codeString.equals(other.codeString))
            return false;
        if (string == null) {
            if (other.string != null)
                return false;
        }
        return string.equals(other.string);
    }

    /*
     * Returns the result of the translations
     * e.g. if the object represents "hi" in Morse, it returns "hi"
     */

    public String asText() {
        return string;
    }

}

enum MorseCode {

    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("--.."),

    ZERO('0', "-----"),
    ONE('1', ".----"),
    TWO('2', "..---"),
    THREE('3', "...--"),
    FOUR('4', "....-"),
    FIVE('5', "....."),
    SIX('6', "-...."),
    SEVEN('7', "--..."),
    EIGHT('8', "---.."),
    NINE('9', "----."),

    PERIOD('.', ".-.-.-"),
    COMMA(',', "--..--");

    private char character;
    private String code;

    private MorseCode(char character, String code) {
        this.character = character;
        this.code = code;
    }

    private MorseCode(String code) {
        this.character = this.name().charAt(0);
        this.code = code;
    }

    // ...

}
\$\endgroup\$
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I use / only to show the separation. It is not there for any other purpose. \$\endgroup\$ – TheCoffeeCup Dec 28 '14 at 1:20
3
\$\begingroup\$

Quick gotchas:

  1. Change the block comments to Javadoc. See a concise reference or the official documentation.

  2. make the MorseCode a nested static class of the MorseString class, or make it another standalone public class, so that other interested class can access it. Also I would suggest to rename it to MorseCodeEnum or like kind so the name won't make people think this is the class that will do Morse code conversions.

\$\endgroup\$
  • 1
    \$\begingroup\$ It might be benficial to link to more official resources than some university's student portal. Additionally I find your naming advice somewhat nonsensical given that a proper IDE shows you what kind of type you get from a name, which makes the enum superfluous \$\endgroup\$ – Vogel612 Dec 27 '14 at 13:45
  • \$\begingroup\$ For sure I considered the official document, e.g. oracle.com/technetwork/articles/java/index-137868.html but I found it's too long and tedious so I searched other resources and found the link very concise and readable. \$\endgroup\$ – Haochen Xie Dec 27 '14 at 13:54
  • \$\begingroup\$ About the naming, it just make sense to call it an enumeration, since it's a enumeration of all Morse codes, not because it is an enum type. \$\endgroup\$ – Haochen Xie Dec 27 '14 at 13:56
  • 1
    \$\begingroup\$ That's a tautology... you're saying that it should be named Enum because it's an enumeration and not because it is an enumeration, because exactly that is what an enum type is.... \$\endgroup\$ – Vogel612 Dec 27 '14 at 13:58
  • 4
    \$\begingroup\$ In general, we prefer a mix of code suggestions and explanation. This answer would be much better with an example of how it looks to convert a block comment to Javadoc. Particularly since a student home page is particularly susceptible to link rot. \$\endgroup\$ – Brythan Dec 27 '14 at 14:08
3
\$\begingroup\$

Your regex could use a full overhaul... currently you're or-ing characters together into a capture group for any single character.

That's somewhat nonsensical, given you could use a proper character-class instead, which might even speed the regex up.

Use the following as your regex:

private static final Pattern VALID_MORSE_PATTERN =
   Pattern.compile("[" + Pattern.quote(".-/") + "\\s]*");
\$\endgroup\$

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.