Tests? Where are the tests?
Where are the unit tests?
MorseString import javax.sound.sampled looks like violation of SRP - Single Responsibility Principle
The import statements are already hinting at possibly misplaced responsibility, knowing the MorseString class from the previous review. More about this further down.
Correct usage of JavaDoc
In the source code fragment ...
public class MorseString {
/**
* This class represents a Morse Code String. It uses the International
* alphabet (M.1677).
*/
public static final char DOT = '.';
}
... the JavaDoc comment syntactically does not describe class MorseString, as intended, but the field char DOT.
JavaDoc comments are always for the one next thing that follows, that's how JavaDoc works.
More hints:
- Avoid redundant text like "this class" or "this method". The reader knows that it's a class or a method.
- Keep in mind that the first sentence is also extracted by JavaDoc as a brief for summary views.
- Write one sentence per line and do NOT use a hard limit for the line length. Because if you use a hard limit for the line length and do not use one sentence per line, you keep on reformatting text. Even worse, in a team, you end up with merge conflicts in JavaDoc comments, which just sucks and drives everyone crazy. New sentence - new line. Just like with normal source statements.
- No empty line between a JavaDoc comment and what it documents.
- Make use of the proper JavaDoc tags.
- Do not repeat / explain the obvious. Explain the non-obvious, give a holistic guide, i.e. use hyperlinks.
- The first sentence is automatically used by JavaDoc as brief description. Therefore, placing the first full stop correctly matters a lot. It's a good habit to always care even about such seemingly minor details like not forgetting the full stop.
Here's how it could look like for class MorseString and constructor MorseString(String):
/**
* A Morse String according to International alphabet M.1677.
* @see <a href="http://www.itu.int/dms_pubrec/itu-r/rec/m/R-REC-M.1677-1-200910-I!!PDF-E.pdf">M.1677 (PDF version)</a>.
* @see <a href="http://en.wikipedia.org/wiki/Morse_code">Morse code (Wikipedia)</a>
*/
public class MorseString {
// ...
/** Create a {@link MorseString} from a {@code String codeString}.
* The {@link #string} will be derived from {@code codeString}.
*
* @param codeString
* String with morse code from which to create this {@link MorseString}.
* @see #parseFromText(String) for a factory method that creates a {@link MorseString} from plain text.
* @see #parseFromMorse(String) for a factory method that creates a {@link MorseString} from a code string.
*/
private MorseString(String codeString) {
}
}
You may want to use a little bit less tags if readability is more important. Fact is JavaDoc still does not yet support markdown out-of-the-box (sigh).
However, if you favor Markdown (like me), you may simply want to Google for Markdown support for JavaDoc, there are solutions available.
DOT and DASH are probably not required.
We like to introduce constants for magic numbers because the names of these constants add meaningful information, like i.e.
private static final int SAMPLING_FREQUENCY_IN_HZ = 44100;
In the case of
public static final char DOT = '.';
I don't really see a value add, I think in this particular case it rather complicates things.
Except, of course, if you plan to replace these constants with variables and allow users to create different codings than just ".-", i.e. "._" instead.
DOT and DASH should probably be private.
I don't see how other classes would currently benefit from DOT and DASH. Oh yes, they could reference it. But we should keep in mind that they are public static final <primitive>s, which are replaced by the compiler at compile-time. If class Foo uses MorseString.DOT, and MorseString.DOT changes, class Foo needs to be recompiled.
If the intention of DOT and DASH being public is that other classes could use these constants, and that these constants might have different values in different setups, then these constants need to be hidden with accessor methods, otherwise the independent deployability of the users of MorseString would not work as expected.
Names string and codeString
How about renaming them to plainText and morseText?
I think that these names would communicate the intent better than the current names.
Especially, string itself, standing on its own, doesn't communicate any intent at all, it needs the presence of its sibling codeString to communicate its own intent.
string and codeString are final - good!
This tells me that instances of MorseString are immutable.
If I want a different MorseString, I need a new object.
With which I'm perfectly happy.
This is how similar classes like String, Integer etc. work as well.
This is how Date actually should have been designed, too.
And finally in Java 8 the new stuff in java.time is.
speed looks like a violation of SRP - Single Responsibility Principle
What is the relation between speed and MorseString - other than speed was put in the same class?
I think that speed is unrelated to MorseString, it is related to how to play a MorseString, and I could play the same MorseString with different speeds on different audio channels at the same time. Or wait, I can't, because the design doesn't allow me, I need to create a second MorseString with the same content first!
That's not how it should be.
If the responsibility of playing a Morse tune would be separated from the MorseString, one could play separate Morse tunes at the same time.
Meaning of speed unclear.
Is it characters per minute? Words per minute? Does a higher value indicate higher speed or lower speed?
Factory methods parseFromText and parseFromMorse - good!
These method names make it clear what they do, and I cannot get confused about what they take for input.
Constructor MorseString and factory methods parseFromText and parseFromMorse, lack of design symmetry.
In the case of parseFromText(), the responsibility for the "hard work" is with praseFromText().
In the case of praseFromMorse(), the responsibility for the "hard work" is with the constructor MorseString().
This asymmetry in responsibility has no obvious reason (I need to behave as if I knew not the history of the code).
I'd move the logic completely from the constructor into the factory methods and leave the constructor as dumb as it could be:
private MorseString(final String morseText, final String plainText) {
this.morseText = morseText;
this.plainText = plainText;
}
Position of VALID_MORSE_PATTERN
It's usual in Java to do the following sequence inside classes:
- fields
- static final fields.
- static fields.
- non-static fields
- constructors
- methods
The position of VALID_MORSE_PATTERN is after the constructor and thus violates this rule.
Name VALID_MORSE_PATTERN
Although the variable is never re-assigned, it is not in the nature of constants in the sense that it does not replace a primitive value constant or some similar constant object.
In such cases, it is not mandatory to use UPPER_CASE, lowerCamelCase would be accepted instead as well.
I personally prefer lowerCamelCase because UPPER_CASE feels like the source is SHOUTING AT ME.
Construction of VALID_MORSE_PATTERN - overly complex Regex
I think there are two group of people, those who understand regexes and those who don't.
For those who don't understand regexes, they're cryptic anyway, and Pattern.quote() probably makes no difference to them.
For those who understand regexes, Pattern.quote() makes it unnecessarily complicated.
I'd use Pattern.quote() in cases where the source code has no control over the String that needs to be quote()d.
In cases where the source code has control over the String, I would use the String directly.
The following should be an equivalent Pattern object, just that it's constructed in a simpler way:
private static final Pattern VALID_MORSE_PATTERN = Pattern.compile("[-.\\s]*");
SRP - Single Responsibility Principle violation in VALID_MORSE_PATTERN / isValidMorse()
This duplicates functionality from MorseCharacter.
And it's inconsistent because it claims that "------------------------" is a valid morse code, which is not.
If MorseCharacter would be the sole responsible for the task, there would be no such problem.
translate(String) overly complex.
I think the method is overly complex.
It has 15 lines of method body.
That's three times as much as I normally accept in my production code.
I'd consider simplifying or splitting it.
code.isEmpty() check in translate(String) should not be required.
If the algorithm is perfect, it will not need to treat code.isEmpty() as a special case.
The algorithm would automatically yield an empty plainText for an empty morseText.
Inconsistent parameter name code in translate(String).
The parameter code is named inconsistently from other places in the code, where a variable of the same semantic class would be named codeString.
And as mentioned above, I'd rather go for morseText (and plainText).
Spelling mistake in JavaDoc of translate(String)
Traslates -> Translates
Choose consistent names and stick to them.
The same type of data is called codeString in some places (field, constructor) and code in other places (translate()) and morse in yet other places (parseFromMorse()). Stick to one name and use it only. As said I suggest morseText and plainText.
JavaDoc of overridden toString()
When you override a method and provide new JavaDoc, you should make a conscious decision about whether you want to replace the overridden method's JavaDoc or whether you want to amend / extend it.
The way you wrote your JavaDoc means you replace the documentation.
However, the text suggests that you would rather want to extend the documentation.
In that case, use the following JavaDoc:
/**
* {@inheritDoc}
* Returns the code as a String, i.e. if this MorseString represents "hi", it returns ".... ..".
*/
@Override
public String toString() {
return codeString;
}
No blank line between a JavaDoc comment and what it documents.
It's a convention that there is no blank line between a JavaDoc comment and what it documents.
Actually that convention is there for all comments.
Overly-complex equals() method
The guard if (this == obj) is a performance optimization. As it is a usual pattern, you can keep it, but I wouldn't.
However, the null checks are redundant, you've programmed MorseString in a way that string and codeString will never be null.
You can directly go for
return codeString.equals(other.codeString) && string.equals(other.string);
Methods toString() and asText()
I would
- keep
toString() as it is.
- Rename method
asText() to getPlainText() which would return the string / plainText.
- Add a new method
getMorseText() which would return the codeString / morseText.
Method play() misplaced responsibility / mix of responsibilities.
The class MorseString contains business logic.
The method play() contains presentation (a delivery mechanism as Uncle Bob calls it).
Business Logic and presentation should be in separate classes, even in separate components (in IntelliJ IDEA: separate modules, in Eclipse: separate projects).
Move everything related to getting noisy to a separate class MorsePlayer or so.
Method play() could use a switch-case
There is a chain of four if-checks on the very same primitive variable c.
The advantage of a switch-case in this situation is that it makes it more obvious that it's switching on the same variable.
Actually, we could call this chain of four ifs a "switch-case in disguise".
Duplicate code in play()
The lines
try {
Thread.sleep(speed);
} catch (InterruptedException e) {
e.printStackTrace();
}
are duplicate code and can be refactored into a funtion - if taking the speed argument into a parameter.
private static void silence(final int durationInMilliSeconds) {
try {
Thread.sleep(durationInMilliSeconds);
} catch (final InterruptedException e) {
e.printStackTrace();
}
}
Then the code for play() would become 8 lines shorter.
Further refactoring of play() and sound()
Actually what you really want to do is some milliseconds of silence.
You can get those by sleeping.
But you can also get those by playing silence.
If you would add a double volume parameter, you could use sound() not only for the noise part of the Morse signal, but also for the silence part.
Then you could just check if you need noise:
public void play() throws LineUnavailableException {
for (final char c : morseText.toCharArray())
play(c);
}
private static void play(final char c) throws LineUnavailableException {
final boolean isNoisy = c == DOT || c == DASH;
final int units = c == MorseCharacter.WORD_SEPARATOR || c == DASH ? 3 : 1;
final sound(840, units * speed, isNoisy ? 1.0 : 0.0);
}
Contract of method setSpeed() regarding parameter speed unclear
How do I know what the meaning of speed is? Even worse, the meaning of field speed and paramter speed are different. What is speed? Is it a time? A velocity? A frequency? What unit? Is it words per minute or characters per minute? Is it on CODEX or on PARIS?
Method sound() creates the same byte[] contents over and over again.
Consider caching the byte[] for the sound. Basically there are only two sounds - dit and dah, and maybe a third one for one unit of silence. In future class MorsePlayer, you could create these byte[] whenever the speed or frequency are set.
Parameter names in method sound()
What I already like a lot about the parameter names in sound() is that they tell me the units. In this case, I can even guess that hz is probably the frequency for the tone and msecs is the duration of the tone.
Still, I would go for frequencyInHz and durationInMilliSeconds or durationIn_ms.
I wouldn't go for durationInMs because would that be milliseconds or megaseconds? SI prefixes are case-sensitive and that conflicts with camel case. Of course megaseconds makes no sense here, however, I always seek for rules which are universally applicable.
Responsibilities of sound()
sound() currently has the following responsibility structure:
- Create the sound
- Allocate the sound buffer
- Create the base wave
- Soften the attack
- Soften the release (which by the way I think is obfuscated already)
- Play the sound
- Allocate audio resources
- Play sound using audio resources
- Release audio resources
That's an awful lot for a single method!
I'd go, as already mentioned above, for a class MorsePlayer. You could make it allocate the audio resources in the constructor or an open() method, and release the audio resources in a close() method. The methods for creating the sounds could be createSound(), createWave(), smoothenAttack() and smoothenRelease(). They would be called whenever the parameters for sound creation are changed.
Magic numbers in sound()
The method sound uses two magic numbers, 8000 and 80.
I'd introduce constants of variables for them, like
private static final int SAMPLING_FREQUENCY_IN_HZ = 8000;
private static final int NUMBER_OF_FADE_SAMPLES = 80;
For 127, you could use Byte.MAX_VALUE.
For the 8 in AudioFormat(), you could use Byte.SIZE.
boolean arguments to AudioFormat() in sound()
I hate boolean arguments.
In most cases you cannot tell what they mean and you cannot remember in which order they were supposed to come.
In that case, I suggest introducing variables for readability.
Like this:
final boolean isSigned = true;
final boolean isBigEndian = false;
new AudioFormat(..., isSigned, isBigEndian);
Or constants, like this:
private static final boolean AUDIOFORMAT_SIGNED = true;
private static final boolean AUDIOFORMAT_UNSIGNED = false;
private static final boolean AUDIOFORMAT_BIG_ENDIAN = true;
private static final boolean AUDIOFORMAT_LITTLE_ENDIAN = false;
new AudioFormat(..., AUDIOFORMAT_SIGNED, AUDIOFORMAT_LITTLE_ENDIAN);
Or at least comments, like this:
new AudioFormat(..., /*signed: yes*/ true, /*little endian: no*/ false);
I'd go for the constants.
Thread.sleep()
It throws InterruptedException, which sucks but is a necessity. That however leaves us with code that sucks. You could play an inaudible sound instead by extending sound with a third parameter double volumeFactor which would be 1.0 for full volume and 0.0 for silence.
Wrong comment in play()
In play() you say // ... only sleep 100 ms is required.
But you do not do Thread.sleep(100);, you do Thread.sleep(speed);.
The code is correct, the comment is wrong.
If I knew less about Morse all I could tell is the code and the comment are inconsistent and would be left puzzled and baffled about which of those two is right and which is wrong and what to change, code or comment.
We could speak of units, then it becomes clear and independent of speed settings.
Is MorseCharacter still the right name?
MorseCharacter does not represent a single character, instead it is for translating characters using the codes.
How about renaming MorseCharacter to MorseAlphabet?
MorseCharacter: Use ResourceBundle for data?
Consider moving the data used by codes.put into a data file so that the source code needs no change when you extend the alphabet. The class which would typically be used for this is ResourceBundle, which effectively resolves to a .properties file loaded via ClassLoader and Properties.
How about a consistency check in MorseCharacter?
The maps codes and chars should have the same number of elements.
assert chars.size() == codes.size();
MorseCharacter.decode() might throw NullPointerException
When the Map chars has no entry for String s, the Character returned by chars.get(s) is null.
When converting null from Character into char it will throw a NullPointerException.
I'd rather check for null and throw IllegalArgumentException, which would be more informative.
MorseCharacter.encode() might return null
I think that's worth mentioning, or changing the contract to throw IllegalArgumentException instead.
Ideas for further programming
I'm sure you will not run out of ideas. Still allow me to share what came into my mind.
- Complete the Morse alphabet with more punctuation and international characters.
- Create a
MidiMorsePlayer which would use MIDI instead of samples for playing the Morse code.
- Make multiple Morse alphabets available, with M.1677 being the default.
- Create a
MorseSoundDecoder which takes sampled data and decodes it into a MorseString by analyzing the sounds and gaps.
- Extend the
MorsePlayer to create an entire audio sample for a MorseString.
- Create a GUI which the user can use to play around with Morse stuff.
- Add binary morse (1 for one unit of sound, 0 for one unit of gap, i.e. SOS being 101010001110111011100010101). The binary morse is, by the way, the perfect input for
MorsePlayers to play sound.
- Add the prosigns like starting signal and end of work.
- Add a configurable stereo effect which pans dit to one side and dahs to the other.
- Add support for Wabun code as well as switching between Morse code and Wabun code using the DO and SN prosigns.
DASHandDOTconstants pretty pointless and making the code harder to read but that's pretty subjective and could easily be argued differently. \$\endgroup\$