6
\$\begingroup\$

I am trying to do some translation of text files. I have a translation .csv file and a file I need to translate (tab delimited). The translation is creating a file with an extra column of the translation.

I'd like to hear any suggestions on this.

import com.google.common.base.Joiner;
import com.google.common.base.Splitter;
import com.google.common.collect.Lists;
import com.google.common.io.Files;

import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;


public class TextFileManipulationExample {

  public static void main(String[] args) {
    new TextFileManipulationExample().translate();
  }

  private void translate() {
    List<String> translationFileContent = readFile("dictionary.csv");
    Stream<List<String>> translationLines = translationFileContent.stream().map(
      line -> Splitter.on(",").splitToList(line));
    Map<List<String>, String> translationMap = translationLines.collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));
    List<String> toTranslateLines = readFile("file_to_translate.txt");
    Stream<List<String>> listWithTranslation = toTranslateLines.stream().map(line -> {
      List<String> list = Lists.newArrayList(Splitter.on("\t").splitToList(line));
      if (translationMap.containsKey(list)) {
        list.add(translationMap.get(list));
      }
      return list;
    });
    String contentToWrite = listWithTranslation.map(lineList -> Joiner.on(",").join(lineList)).collect(
      Collectors.toList()).stream().reduce((t, u) -> t + "\n" + u).get();
    writeFile(contentToWrite, "translated.csv");
  }

  private void writeFile(String contentToWrite, String fileName) {
    try {
      Files.write(contentToWrite, new File(fileName), Charset.defaultCharset());
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }

  private List<String> readFile(String fileName) {
    try {
      return Files.readLines(new File(fileName), Charset.defaultCharset());
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }
}
\$\endgroup\$
  • \$\begingroup\$ Map<List<String>, String> translationMap: Does it mean that translating "hi" and "hey" to "g'day" will contain the following entry? ["hi", "hey"] => "g'day" \$\endgroup\$ – h.j.k. Oct 22 '15 at 9:53
  • 1
    \$\begingroup\$ yes that is the meaning. I thought about using apache common's Pair, but didnt want to use another framework in that case. Do you know if guava has something similar? \$\endgroup\$ – oshai Oct 22 '15 at 12:19
  • \$\begingroup\$ @oshai stackoverflow.com/q/156275/1896169 \$\endgroup\$ – Justin Oct 22 '15 at 22:27
3
\$\begingroup\$

You are using Java 8's functional stream programming, but you leave out method chaining. Depending on how you chain methods, it could be more or it could be less readable. You might be imagining this:

readFile("dictionary.csv").stream().map(line -> Splitter.on(",").splitToList(line)).collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));

There is no way I could read that. However, consider this:

Map<List<String>, String> translationMap = readFile("dictionary.csv").stream()
            .map(line -> Splitter.on(",").splitToList(line))
            .collect(
                Collectors.toMap(
                    l -> Lists.newArrayList(l.subList(0, 2)),
                    l -> l.get(2)
                )
            );

I find this much more readable. By simply reading vertically, I can determine that the code does this:

  1. Reads dictionary.csv into a list of strings (via readFile).
  2. Split each string in the dictionary on ",", into a List<String>.
  3. Collect the List<String>s into a Map, where
    1. The keys are the first two elements of each list, still in a List
    2. The values are the 3rd element of each list.

On the other hand, your code as is is littered with type names; do I really need to know that that is a Stream<List<String>> when that's easily determined from context?:

List<String> translationFileContent = readFile("dictionary.csv");
Stream<List<String>> translationLines = translationFileContent.stream().map(
  line -> Splitter.on(",").splitToList(line));
Map<List<String>, String> translationMap = translationLines.collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));

Alternatively, if you really like the temporary variables, put some empty lines in there to separate logical sections. Also, consider using Project Lombok, where you could just do this:

List<String> translationFileContent = readFile("dictionary.csv");
val translationLines = translationFileContent.stream().map(
    line -> Splitter.on(",").splitToList(line));
val translationMap = translationLines.collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));

(Coder discretion advised on where you should use val).

\$\endgroup\$
  • \$\begingroup\$ I agree your version of chaining is more readable. However, I prefer not to rely on indentation and line breaks. my smart IDE might just reformat indentations and my colleagues might change indentation or line breaks :-) How about breaking it to small methods? \$\endgroup\$ – oshai Oct 23 '15 at 6:20
  • 1
    \$\begingroup\$ @oshai The edit made it hard to understand my recommendations in the answer, so I pulled it out into another answer. \$\endgroup\$ – Justin Oct 23 '15 at 6:48
2
\$\begingroup\$

Chaining

Avoid temporary variables and just chain all the methods, it is common practice in FP.

Utility class

Java forces you to catch some exceptions, you may want to read files without that tedious try catch other times, so I suggest a FileHandler class to contain your readfile and writefile.

Arguments

The translate method should take input and output filenames as arguments.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good comments, thanks. Regarding the chaining, IMHO it makes the code less readable. The solution that I can think of is if I want to eliminate those variables, replace them with many small methods that will indicate what I am doing. I will try to do something like that. \$\endgroup\$ – oshai Oct 22 '15 at 19:33
-1
\$\begingroup\$

You want to remove the function chaining and temporary variables by replacing it with other functions. I strongly recommend against this.

Breaking this into small methods, as you suggest, isn't good. Functional programming is designed to be highly readable as is like this. Hiding the details behind functions makes it hard to understand:

Map<List<String>, String> translationMap = convertTranslationLinesToMap(
    splitEachLineByComma(readFile("dictionary.csv"))
);

And each of those functions would be trivial one-two lines of code. That's not what functions are for.

Your IDE should format your code with newlines and indents, or leave the code unformatted when it is wrapped across lines. Making individual functions to do this would be the analogue of making individual functions the following pre-Java 8 code:

List<String> translationFileContent = readFile("dictionary.csv");
Map<List<String>, String> translationMap = new HashMap<>();

for (String line : translationFileContent) {
    List<String> list = Splitter.on(",").splitToList(line);
    translationMap.put(Lists.newArrayList(list.subList(0, 2)), list.get(2));
}

This code should not be moved into its own function as it's highly specific to this location and refactoring into a function just seems useless. Don't do this for the Java 8 code. In all honesty, separating each call into temporary variables is probably cleaner than making new functions for everything.

Additionally, formatting the Java 8 code across multiple lines isn't "relying on indentation and newlines" any more than the other code example is. Try reading this:

List<String> translationFileContent = readFile("dictionary.csv"); Map<List<String>, String> translationMap = new HashMap<>(); for (String line : translationFileContent) { List<String> list = Splitter.on(",").splitToList(line); translationMap.put(Lists.newArrayList(list.subList(0, 2)), list.get(2)); }

Similarly, Java 8 code is not meant to be formatted like that.

Regarding the concern of colleagues formatting away the line breaks, you could add a comment explaining why it is like that. But poorly chosen IDE format settings could still interfere, unless you did something like this (this isn't good in practice, though):

Map<List<String>, String> translationMap = readFile("dictionary.csv").stream()
                                                              // For each line,
            .map(line -> Splitter.on(",").splitToList(line))  // split on commas, then
            .collect(                                         // collect the result
                Collectors.toMap(                             // into a map
                    l -> Lists.newArrayList(l.subList(0, 2)), // whose keys are the first two elements of each line
                    l -> l.get(2)                             // and values are the third.
                )
            );

This isn't a good idea in general; we shouldn't need comments at the end of each line.

\$\endgroup\$
  • \$\begingroup\$ Why was this downvoted? \$\endgroup\$ – Justin Nov 24 '15 at 3:13

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.