3
\$\begingroup\$

I'm writing an application called "Learn Morse" in Java using Maven. A Morse code translator is a part of this app. I created encode and decode methods to translate in both sides Morse code and I moved the HashMap to the properties file. I also created a helper method to get keys by value.

pl/hubot/dev/learn_morse/model/Encoder.java:

package pl.hubot.dev.learn_morse.model;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.Properties;

public class Encoder {
    public final String encode(final String input)
            throws IOException,
            NoSuchFieldException {
        StringBuilder encoded = new StringBuilder();
        String lowerCaseInput = input.toLowerCase();
        for (int i = 0; i < lowerCaseInput.length(); i++) {
            char current = lowerCaseInput.charAt(i);
            encoded.append(getMorseCode().getOrDefault(
                    Character.toString(current), " "));
            encoded.append("   ");
        }
        return encoded.toString();
    }

    public final String decode(final String input)
            throws IOException,
            NoSuchFieldException {
        StringBuilder decoded = new StringBuilder();
        String lowerCaseInput = input.toLowerCase();
        for (String current : lowerCaseInput.split(" ")) {
            String key = getKeyByValue(getMorseCode(), current);
            if (key != null) {
                decoded.append(key);
            }
            decoded.append(" ");
        }
        return decoded.toString();
    }

    final Map<String, String> getMorseCode()
            throws IOException,
            NoSuchFieldException {
        Class<?> aClass = Encoder.class;
        ClassLoader classLoader = aClass.getClassLoader();
        String filename = "morse_code.properties";
        try (InputStream input = classLoader
                .getResourceAsStream(filename)) {
            if (input == null) {
                throw new FileNotFoundException(
                        "Sorry, unable to find "
                                + filename);
            }

            Properties properties = new Properties();
            properties.load(input);

            Map<String, String> translations = new HashMap<>();
            for (String key : properties.stringPropertyNames()) {
                String value = properties.getProperty(key);
                translations.put(key, value);
            }

            return translations;
        }
    }

    private <T, E> T getKeyByValue(final Map<T, E> map, final E value) {
        for (Map.Entry<T, E> entry : map.entrySet()) {
            if (Objects.equals(value, entry.getValue())) {
                return entry.getKey();
            }
        }
        return null;
    }
}

resources/morse_code.properties:

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 = __..
\u0105 = ._._
\u0107 = _._..
\u0119 = .._..
\u00E9 = .._..
ch = ____
\u0142 = ._.._
\u0144 = __.__
\u00F3 = ___.
\u015B = ..._...
\u017A = __.._
\u017C = __.._.
0 = _____
1 = .____
2 = ..___
3 = ...__
4 = ...._
5 = .....
6 = _....
7 = __...
8 = ___..
9 = ____.
. = ._._._
, = __..__
' = .____.
" = ._.._.
_ = ..__._
\: = ___...
; = _._._.
? = ..__..
\! = _._.__
- = _...._
+ = ._._.
/ = _.._.
( = _.__.
) = _.__._
\= = _..._
@ = .__._.
\$\endgroup\$
2
\$\begingroup\$

My main complaint about your code is that you rebuild the table used for translation for each and every character you are encoding or decoding. This is a major waste of computational power. In addition I've got some other minor comments:

  • Why the throws SomeException? – Do you really need these, as you don't explicitly throw them from your code? I'm not entirely sure on common or best practices here, so this might be a moot point, but couldn't these be removed in your code?
  • Add more space – The density of your code, makes it harder to read. Add a little vertical space here and there, and it will look a lot better. Consider opening your methods with a newline, so that the signature doesn't interfere with the start of the method.
  • How to access the ch from the properties – In the properties file, you have an entry for ch. How do get to this one? Or is this at typo?

    Also adding space before for loop or if blocks, helps structuring the code within a method. Finally, adding an extra newline so that it is two newlines between methods helps separate the methods.

  • Most variables are named well – Mostly good naming, but input is kind of vague. I would prefer text in encode() and morseCode in decode(), and avoid the lowerCaseInput. Having a variable name describing what has happened to it, seems a little strange.

Avoiding the getMorseCode() rebuild

You're in a class, and a class can have a constructor. Utilize this to build the needed HashMap. And instead of having a rather costly getKeyByValue() I would advice in this case duplicate the reverse map, as well. (Another option would be to use some external library to implement some bidirectional map, like the BiMap from the Guava library). This would lead to code resembling this untested code:

public class Coder {
  private HashMap <String, String> encodeMap;
  private HashMap <String, String> decodeMap;

  Coder() {
      this("morse_code.properties");
  }

  Coder(String filename) {
    InputStream input = null;
    Properties properties = new Properties();
    try {
      input = Morse.class.getClassLoader().getResourceAsStream(filename);
      if (input == null) {
        System.out.println("Sorry, unable to find " + filename);
        return; // Or throw bad exception... :-)
      }

      encodeMap = new HashMap<>();
      decodeMap = new HashMap<>();

      properties.load(input);

      for (String text : properties.stringPropertyNames()) {
        String morseCode = properties.getProperty(text);
        encodeMap.put(text, morseCode);
        decodeMap.put(morseCode, text);
      }

    // Catch and cleanup, if needed...  
    } catch (IOException ex) {
        ex.printStackTrace();

    } finally{
      if (input!=null) {
        try {
            input.close();
              } catch (IOException e) {
                  e.printStackTrace();
            }
      }
    }
  }


  public final String encode(final String textInput) {

    StringBuilder encoded = new StringBuilder();
    int textLength = textInput.toLowerCase().length();

    for (int i = 0; i < textLength; i++) {
      String lookup = Character.toString(textInput.charAt(i));
      encoded.append(encodeMap.getOrDefault(lookup, " "));
      encoded.append("   ");
    }

    return encoded.toString();
  }


  public final String decode(final String morseInput) {

    StringBuilder decoded = new StringBuilder();

    for (String morseCode : morseInput.toLowerCase().split(" ")) {
      decoded.append(decodeMap.getOrDefault(morseCode, " "));
      decoded.append(" ");
    }
    return decoded.toString();
  }
}

Here I've also renamed the class to Coder, as it seemed kind of strange to use the Encode class to decode stuff... I've also removed some temporary variables to avoid clutter.

I also added a second constructor allowing for providing a different filename to the constructor. This way you could change the entire functionality of the encode/decode by providing a different properties file.

Code is untested as I don't have access to do the resources thingy using my online java compiler, but you'll get the gist of the idea.

PS! Using the try-with-resources as Vogel612 exemplifies in his answer, is most likely a better, more up to date way to do the properties reading.

\$\endgroup\$
2
\$\begingroup\$

Simple-ish Performance Improvements:

Instead of reading the morse table in every time you call getMorseCode(), you should cache the lookup table in your instance in a field:

public class Encoder  {
    private static final Map<Character, String> morseCode = new HashMap<>();

You'll notice I already changed the "key" type of the Map. Instead of converting the char you obtain from charAt into a String (creating a whole new instance and all the class creation overhead), in this constellation the char just gets autoboxed into a Character, which may be faster, and generally is cleaner.

The best part about this is that this moves the NoSuchFileException and the NoSuchFieldException from the runtime (and your method signatures) to the startup of your application (or to be more correct the loading of the class).

You can initialize the map in a static initializer block like so:

static {
    try (InputStream props = Encoder.class.getClassLoader().getResourceAsStream("morse_code.properties")) {
        Properties properties = new Properties(props);
        for (String key : properties.stringPropertyNames()) {
            morseCode.put(key, properties.getProperty(key));
        }
    } 
 }

If we squint a little we can see another possible improvement here, together with getKeyByValue. Since we precompute the mappings for the morse code, couldn't we just precompute the reverse and benefit from map-lookup times instead of having to search through all values before getting what we want:

private static final Map<String, Character> reverseMorseCode = new HashMap<>();

And now we can expand our static initializer with:

for (String key : properties.stringPropertyNames()) {
    String morseCode = properties.getProperty(key);
    morseCode.put(key, morseCode);
    reverseMorseCode.put(morseCode, key);
}

Incorporating this should drastically speed up the decode method.


Complicated things:

Let's consider for a moment the task at hand from a "functional" perspective. What you effectively have is a simplistic cipher from cleartext into a dot-dash cipher. We can formulate this as a function on each character into our result space. We need to apply this "mapping function" to each character in our original input.

Java introduced something to fit exactly this in Java 8: Streams.

The encode method can be rewritten with Streams as follows:

public final String encode(final String input) {
     return input.chars()
         .mapToObject(c -> morseCode.getOrDefault((char) c, " "))
         .collect(Collectors.joining("   "));
}

This directly allows us to think of decode in terms of an "inverse function":

public final String decode(final String input) {
    return Arrays.stream(input.toLowerCase().split(" "))
        .map(token -> reverseMorseCode.getOrDefault(token, " "))
        .collect(Collectors.joining(""));
}

Something similar exists for reading from a file, which would allow us to read the morse mapping without going the way around Properties. It's called Files.lines.

Implementing the static initializer with Files.lines is left as an exercise for the reader ;)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy