Translating the Rubberduck - Übersetzen der Gummiente - Part 1: Modelling

So I have committed to making a German translation for the guys over at Rubberduck, which is localized via .resx files.

Now these files are basically XML-Files with a certain, rather simple document structure:

   
   
       Resource Value
   
   

Of course there's a little more to it, but as of now the program doesn't have to handle this.

The basic goal is:

• Parse the resx-file into a DOM-Structure

• Use said DOM-Structure to present a side-by-side view of Resource Values

• Allow editing one side of the Resource Pair (that you translate into)

• And save the whole thing back into the resx-File

I decided on a Presenter-First MVP approach, to develop this in a testable and extensible manner. This gives 4 rather massive interfaces to work upon:

public interface OverviewModel {
    void register(OverviewPresenter p);
    void loadFromDirectory(Path resxFolder, String targetLocale);
    List getTranslations();
    Translation getSingleTranslation(String key);
    void updateTranslation(String key, String newTranslation);
    void save();
}

public interface OverviewPresenter {

    public static final String DEFAULT_TARGET_LOCALE = "de";
    public static final String DEFAULT_ROOT_LOCALE = "";

    void show();
    void initialize();
    void loadFiles(Path resxFolder, String rootLocale, String targetLocale);
    void onException(Exception e, String message);
    void onParseCompletion();
    void onTranslationSubmit(Translation t);
    void onTranslateRequest(String key);
    void onTranslationAbort();
    void onSaveRequest();
}

public interface OverviewView {
    void register(OverviewPresenter p);
    void initialize();
    void show();
    void rebuildWith(List translations);
    void showError(String title, String errorMessage);
}

Why 4 and not 3, as MVP would suggest? Because the editing is happening in a separated view. This view consists of a Label, a Textbox and 2 Buttons. This is the reason that there is only a Pr

I wasn't originally going to post this answer as it seems like petty things, but alas, here it is. I mean no offense by any of it, sometimes I tend to type things out to seem that way, but it's all in good spirit. :)

Readability issues

I was reading through your code and couldn't figure out why it was so hard to read, until I realized you seem to be trying to adhere to a specific character-width policy. Let's not do that please.

I would also recommend standardizing your method-chaining line-breaks, method parameter line-breaks, and the like. If you're going to break one parameter on a method into a new line, do it for all of the parameters on that method. Likewise, if you are line-breaking one method-chain in a group, do it for all of the chains in that group. Otherwise, the eyes have to jump all over the place.

For example, I would rewrite:

try (OutputStream outStream = Files.newOutputStream(outFile,
            StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE)) {

as:

try (OutputStream outStream =
            Files.newOutputStream(
                outFile,
                StandardOpenOption.TRUNCATE_EXISTING,
                StandardOpenOption.WRITE)) {

Though, it is fairly difficult to break statements like that into multiple lines while maintaining readability, I find this much more readable than what is already there.

Likewise, this:

List translationElements = translationDocument
            .getRootElement().getChildren(ELEMENT_NAME);

would be easier to follow as:

List translationElements = translationDocument
            .getRootElement()
            .getChildren(ELEMENT_NAME);

I'm not sure on Java's best-practices on Lambda's, but I would try to avoid using large Lambda expressions like you have. The big ones - the 10+ liners - should be methods instead. Even the smaller 5-10 liners should be methods.

I tend to think of Lambda's as small bits of code that are too small to justify a method.

On to my view of your main concerns


What I'm worried about most in this class:

• The "state machine" I have running in loadResxFile

• My usage of JDOM and general Document manipulation

• Extensibility to additional resx features like Comments

The first thing I notice is regarding point 3. You have:

private final Map originalLocale = new HashMap();

And

originalLocale.put(element.getAttribute(KEY_NAME)
                                .getValue(), element
                                .getChildText(VALUE_NAME));

Which indicate that the only intent was to place only the Key/Value pair in the list.

Personally, I would create a class to represent a LanguageValue or similar.

public class LanguageValue {
    private String key;
    private String value;

    public String getKey() {
        return key;
    }

    public String getValue() {
        return value;
    }
}

Obviously this bit is easy, but then you should have a method (or for ease-of-use a constructor) in it:

public LanguageValue(Element element) {
    key = element.getAttribute(KEY_NAME).getValue();
    value = element.getChildText(VALUE_NAME);
}

And move the following into it:

private static final String VALUE_NAME = "value";
private static final String KEY_NAME = "name";

Then, it should (if my Java is correct) be as simple as:

translationElements.stream().forEach(
                    element -> {
                        originalLocale.put(new LanguageValue(element));
                    });

Which can simplify to (courtesy of Vogel612 in chat):

translationElements.stream()
    .map(LanguageValue::new)
    .forEach(originalLocale::put);

This will make it significantly easier to add support for additional properties in the XML. Comments, etc. (Not sure what else you would need, but it's a start.)

I would also change:

private final Map originalLocale = new HashMap();

To:

private final List originalLocale = new List();

There's no need for a String key here, as Lambda's can take care of that.

(Obviously there's more to change than just these few lines to change this feature, but hopefully it's helpful.)

Regarding the loadResxFile, I would consider moving a lot of that method to inside LanguageValue, as a static method. Or at least this bit:

List translationElements = doc.getRootElement()
                    .getChildren(ELEMENT_NAME);

            translationElements.stream().forEach(
                    element -> {
                        originalLocale.put(element.getAttribute(KEY_NAME)
                                .getValue(), element
                                .getChildText(VALUE_NAME));
                    });

You can pass doc as a parameter to it, and have it return a List back to your loadResxFile method. Then you can also move the following constant into LanguageValue as well:

private static final String ELEMENT_NAME = "data";

The

StackExchange Code Review Q#95004, answer score: 11