4
\$\begingroup\$

This is a update of this question since the requirements for this task changed.

I have a function that takes a description as a String and returns a shortened version of the description.

The shortening is done by checking if a word matches the key of the Map<String,String> MAPPER and replaces the word by its abbreviation defined as the value in the Map. If the word cannot be found in the Map it will not be replaced (it stays the same).

If the value of a word is <remove> the word will be removed from the output string

If the value of a word/character is <remove after> everything after that word/character will be removed from the output string.

If the value of a word/character is <remove after incl> everything after that word/character will be removed from the output string. Including the word/character itself

If the value of a word is <remove before> everything up to that word will removed from the output string

There can be multiple abbreviations for the same word.

AUTOMATIC GAIN CONTROL --> AGC

CONTROL --> CTRL

In any case the function should always take the abbreviation which takes the most words.

Example

Map containing following entries:

AUTOMATIC GAIN CONTROL --> AGC

CONTROL --> CTRL

IDENTIFIER --> ID

THE --> <remove>

. --> <remove after incl>

: --> <remove after>

TH --> <remove before>

Input:

The top control identifier: xyz

Output:

top CTRL ID:

Input:

Lorem impsum TH automatic gain control. This will be deleted

Output:

TH AGC

Code

public final class Shortener {

    public static String mappingFile="";
    private static final Comparator<String> KEY_COMPARATOR = new Comparator<String>() {
        @Override
        public int compare(String s1, String s2) {
            int diff = Integer.valueOf(s2.length()).compareTo(Integer.valueOf(s1.length()));            
            return diff == 0 ? s1.compareToIgnoreCase(s2) : diff;
        }
    };
    private static Map<String,String> MAPPER;

    public static String shortenText(String input) {
        StringBuilder result = new StringBuilder(input);
        for (Map.Entry<String, String> entry : MAPPER.entrySet()) {
            Matcher matcher;
            if (entry.getValue().equals("<remove after>") || entry.getValue().equals("<remove after incl>")){
                matcher = Pattern.compile("(?i).*" + entry.getKey() + ".*").matcher(result);
            }
            else matcher = Pattern.compile("(?i)\\b" + entry.getKey() + "\\b").matcher(result);

            while (matcher.reset().find()) {
                if (entry.getValue().equals("<remove after>")){
                    result.delete(result.indexOf(entry.getKey())+entry.getKey().length(),result.length());
                    return result.toString();
                }
                if (entry.getValue().equals("<remove after incl>")){
                    result.delete(result.indexOf(entry.getKey()),result.length());
                    return result.toString();
                }
                if (entry.getValue().equals("<remove before>")){
                    result.delete(0,result.indexOf(entry.getKey()));
                    return result.toString();
                }

                result.replace(matcher.start(), matcher.end(), entry.getValue());

                if (entry.getValue().isEmpty() && result.length() != 0) {                   
                    result.deleteCharAt(matcher.start());                   
                }               
            }           
        }
        return result.toString();
    }

    public static void importMap() {
        FileInputStream fstream;
        MAPPER = new TreeMap<String, String>(KEY_COMPARATOR);
        try {
            if(mappingFile.isEmpty()) {         
                String fileSperator = System.getProperty("file.separator");
                String shortenerConfigPath = System.getProperty("user.dir")+fileSperator+"config"+fileSperator+"shortener.ini";
                mappingFile = shortenerConfigPath;
            }
            fstream = new FileInputStream(mappingFile);
            BufferedReader br = new BufferedReader(new InputStreamReader(fstream));

            String strLine;

            while ((strLine = br.readLine()) != null){
              String[] arr = strLine.split("\t");
              if (arr.length!=2){
                  System.out.println("Error Shortener import");
              }
              else {
                  if (arr[1].equals("<remove>")){
                      MAPPER.put(arr[0], "");
                  }
                  else MAPPER.put(arr[0], arr[1]);
              }
            }           
            br.close();     


        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        }

    }

}

As you can see I need a different regex pattern for the <remove after> and <remove after incl> cases in order to achieve the correct output. For the other cases a regex that looks for words is ok. I feel like the shortenText() could be improved. Returning the String early for the special cases seems like it would cause errors but so far that has worked for me.

The Map MAPPER is populated by reading the contents of a tab delimited file.

\$\endgroup\$
  • \$\begingroup\$ Is the matching supposed to be case sensitive? \$\endgroup\$ – Misha Sep 5 '15 at 2:44
  • \$\begingroup\$ No it is not. However, any found term will be replaced with its abbreviation as it is defined in the mapping file (case sensitive) \$\endgroup\$ – isADon Sep 6 '15 at 10:15
  • \$\begingroup\$ I think there is a bug when having multiple entries that map to <remove after> or <remove after incl>. Returning the String early seems suspicious but I have not found any other way to achieve the goal. \$\endgroup\$ – isADon Sep 6 '15 at 10:21
3
\$\begingroup\$

It's me again. :)

Quote your regex!

Your code is broken because you forgot to quote("."), which carries special meaning in regex. Without quoting:

Pattern.compile("(?i).*..*")

Will match pretty much anything at least a character long, but then the following will fail for lines that do not contain a literal "." character, throwing a StringIndexOutOfBoundsException:

 if (entry.getValue().equals("<remove after incl>")){
    result.delete(result.indexOf(entry.getKey()),result.length());
    return result.toString();
}

Early returns

Returning the String early for the special cases seems like it would cause errors but so far that has worked for me.

This does not work for your second example, as your shortenText() method 'gives up' after removing the text before TH and does not trim away ". This will be deleted". I think you are looking to break out of the while-loop instead of returning:

while (matcher.reset().find()) {
    if (entry.getValue().equals("<remove after>")){
        result.delete(result.indexOf(entry.getKey()) + entry.getKey().length(),
                        result.length());
        break;
    }
    if (entry.getValue().equals("<remove after incl>")){
        result.delete(result.indexOf(entry.getKey()), result.length());
        break;
    }
    if (entry.getValue().equals("<remove before>")){
        result.delete(0, result.indexOf(entry.getKey()));
        break;
    }
    result.replace(matcher.start(), matcher.end(), entry.getValue());
    if (entry.getValue().isEmpty() && result.length() != 0) {                   
        result.deleteCharAt(matcher.start());
    }               
}    

Placeholders

Once again, as previously covered in your earlier question, using a String-based placeholder is not recommended, especially now that you have more values to cover. A typo from <remove after incl> to <remove atfer incl will potentially make debugging painfully hard at 3 am.

State, and file reading

Just to elabroate slightly on @MattPutnam's answer, you should consider making a Shortener instance per-mappingFile, instead of making the field public static. This makes the usage of each instance clearer, as they are tied to one mapping file. What can remain static is your importMap() method, though I think it should be taking in an argument to the file's location, and returning the desired Map.

On a related note, I think this can be in a method itself, as defaulting to a mapping file when the given argument is invalid isn't really part of the file-reading handling, but predates it.

private static String getOrDefault(String mappingFile) {
    if(mappingFile == null || mappingFile.isEmpty()) {         
        String fileSperator = System.getProperty("file.separator");
        return System.getProperty("user.dir") + fileSperator + 
                "config" + fileSperator + "shortener.ini";
    }
    return mappingFile;
}

Encapsulation

Now that you have different Pattern expressions and handling to cater, you may want to consider putting them into classes that encapsulates these states and behaviors for each target/replacement pair.

For example, you can have a Processor class that internalizes what to look for, and how to handle it:

public static final class Processor {
    private final String target;
    private final String replacement;
    private final Pattern pattern;
    private final Action action;

    // StringBuilder instances will be passed to this method for manipulation
    public void accept(StringBuilder result) { ... }
}

It can have some static methods that lets you specify, using readable method names, the desired behavior. For example:

private static final Set<Processor> MAPPER = getMapper();

private static Set<Processor> getMapper() {
    // PROCESSOR_COMPARATOR will compare Processor instances
    // in a similar way to how the 'keys' aka targets are sorted
    Set<Processor> mapper = new TreeSet<Processor>(PROCESSOR_COMPARATOR);
    mapper.add(Processor.remove("THE"));
    mapper.add(Processor.replace("AUTOMATIC GAIN CONTROL", "AGC"));
    mapper.add(Processor.replace("CONTROL", "CTRL"));
    mapper.add(Processor.replace("IDENTIFIER", "ID"));
    mapper.add(Processor.removeBefore("TH"));
    mapper.add(Processor.removeAfterIncluding("."));
    mapper.add(Processor.removeAfter(":"));
    return mapper;
}

Over here, Action is an enum that handles the requested actions:

enum Action {
    REPLACE {
        // replacement implementation goes here
        public void accept(StringBuilder result, Processor processor) { ... }
    },
    REMOVE_BEFORE {
        // remove-before implementation goes here
        public void accept(StringBuilder result, Processor processor) { ... }
    },
    REMOVE_AFTER {
        // remove-after implementation goes here
        public void accept(StringBuilder result, Processor processor) { ... }
    },
    REMOVE_AFTER_INCL {
        // remove-after-including implementation goes here
        public void accept(StringBuilder result, Processor processor) { ... }
    };
}

And yes, for the Java 8 fans with a keen eye, Processor should implement Consumer<StringBuilder> and Action should implement BiConsumer<StringBuilder, Processor>.

\$\endgroup\$
  • \$\begingroup\$ I was hoping you would answer me again:) I can see the problem you pointed out with the regex. But I don't understand how I should use the quote() method. What should I replace matcher = Pattern.compile("(?i).*" + entry.getKey() + ".*").matcher(result); with? \$\endgroup\$ – isADon Sep 7 '15 at 8:01
  • \$\begingroup\$ @isADon Pattern.compile("(?i).*" + Pattern.quote(entry.getKey()) + ".*"). \$\endgroup\$ – h.j.k. Sep 7 '15 at 8:11
  • \$\begingroup\$ I found one other problem just now. The pattern is case insensitive as it should but indexOf() is case sensitive. This leads to problems when it comes to handling the special arguments. Is there anything better than doing result.delete(result.toString().toLowerCase().indexOf(entry.getKey().toLowerCase()),result.length());? \$\endgroup\$ – isADon Sep 7 '15 at 8:26
  • \$\begingroup\$ @isADon wells the answer/enhancement I had in mind is that with the right regex pattern, you can continue to use matcher.start() and matcher.end() to get the indices, instead of having to do an indexOf(). You may want to get more familiarized with regex first. :) \$\endgroup\$ – h.j.k. Sep 7 '15 at 8:30
  • 1
    \$\begingroup\$ Ok I will try to look in to it. Thanks again \$\endgroup\$ – isADon Sep 7 '15 at 8:52
4
\$\begingroup\$

It should be possible to make shortenText() more efficient, but as it's written I think it's pretty clear and I had no problem deciphering it, which is good. A more optimized version would likely be much harder to understand, so I would say only worry about it if it becomes a problem.

while (matcher.reset().find()) is suspicious but I think I see why you did that.

My main comment is that by making everything static, you can only ever have one shortener, and using it is kind of weird. You have to initialize it by changing static fields and calling static methods in a specific order. It would be better to make a proper class, so you could create multiple shorteners with different mapping files, and also clean up the interface. As a user of this utility, I would want to write:

Shortener s1 = new Shortener(path_to_file);
foo(s1.shorten(someString));
Shortener s2 = new Shortener(another_path_to_file);
foo(s2.shorten(someString));
\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback. For my use case there will never be any use for more than one Shortener object. \$\endgroup\$ – isADon Sep 6 '15 at 10:19

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.