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:<root>   <!-- Snip -->   <data name=Resource_Key>       <value>Resource Value</value>   </data>   <!-- Snip --></root>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-StructureUse said DOM-Structure to present a side-by-side view of Resource ValuesAllow editing one side of the Resource Pair (that you translate into)And save the whole thing back into the resx-FileI 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<Translation> 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<Translation> 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 Presenter for that part. It's basically a Popup dialog and that's why it has following @implNote in the javadoc:Implementation will not happen with an additional separated View  interface, since there is in no way enough View-Logic to justify itSo here (to clean up on the interface side) comes the TranslationPresenter:public interface TranslationPresenter {    public void register(OverviewPresenter p);    public void show();    public void hide();    public void setRequestedTranslation(Translation t);}Now this all is just the skeleton of what is yet to come. Cramping the whole program into a single post would not make for good reviewable material though, so without further ado, here comes the part worrying me the most:public class OverviewModelImpl implements OverviewModel {    private static final String VALUE_NAME = value;    private static final String KEY_NAME = name;    private static final String ELEMENT_NAME = data;    private static final String FILE_NAME_FORMAT = RubberduckUI%s.resx;    private final Map<String, String> originalLocale = new HashMap<String, String>();    private final ExecutorService fileOperationService = Executors            .newSingleThreadExecutor();    private final BiConsumer<Path, Boolean> loadResxFile = (path, isTarget) -> {        SAXBuilder documentBuilder = new SAXBuilder();        try {            if (isTarget) {                this.translationDocument = documentBuilder.build(path.toFile());            } else {                Document doc = documentBuilder.build(path.toFile());                List<Element> translationElements = doc.getRootElement()                        .getChildren(ELEMENT_NAME);                translationElements.stream().forEach(                        element -> {                            originalLocale.put(element.getAttribute(KEY_NAME)                                    .getValue(), element                                    .getChildText(VALUE_NAME));                        });            }        } catch (JDOMException e) {            this.presenter.onException(e, Unspecified Parsing error);        } catch (IOException e) {            this.presenter.onException(e, Unspecified I/O Error);        } catch (Exception e) {            this.presenter.onException(e, Something went really wrong);        }    };    private final XPathFactory xPathFactory = XPathFactory.instance();    private OverviewPresenter presenter;    private Document translationDocument;    private Path currentPath;    private String currentTargetLocale;    @Override    public void register(final OverviewPresenter p) {        presenter = p;    }    @Override    public void loadFromDirectory(final Path resxFolder,            final String targetLocale) {        this.currentPath = resxFolder;        this.currentTargetLocale = targetLocale;        // for now there's only en-US root-Files        final Path rootFile = resxFolder.resolve(fileNameString());        final Path targetFile = resxFolder                .resolve(fileNameString(targetLocale));        Runnable buildDocument = () -> {            originalLocale.clear();            loadResxFile.accept(rootFile, false);            loadResxFile.accept(targetFile, true);            normalizeTargetLocale();            presenter.onParseCompletion();        };        fileOperationService.submit(buildDocument);    }    private void normalizeTargetLocale() {        List<Element> translationElements = translationDocument                .getRootElement().getChildren(ELEMENT_NAME);        Set<String> passedKeys = new HashSet<String>();        Iterator<Element> it = translationElements.iterator();        while (it.hasNext()) {            Element el = it.next();            String key = el.getAttribute(KEY_NAME).getValue();            if (!originalLocale.containsKey(key)) {                // LIVE COLLECTION!!                it.remove();                continue;            }            passedKeys.add(key);        }        // build new elements for newly created keys in root        originalLocale                .keySet()                .stream()                .filter(k -> !passedKeys.contains(k))                .forEach(                        k -> {                            Element newElement = new Element(ELEMENT_NAME);                            Element valueContainer = new Element(VALUE_NAME);                            valueContainer.setText(originalLocale.get(k));                            newElement.setAttribute(KEY_NAME, k);                            newElement.addContent(valueContainer);                            translationDocument.getRootElement().addContent(                                    newElement);                        });    }    private String fileNameString(final String localeIdent) {        return String.format(FILE_NAME_FORMAT, localeIdent.isEmpty() ?  : .                + localeIdent.toLowerCase());    }    @Override    public List<Translation> getTranslations() {        List<Element> translationElements = translationDocument                .getRootElement().getChildren(ELEMENT_NAME);        return translationElements.stream().map(el -> {            final String key = el.getAttribute(KEY_NAME).getValue();            final String currentValue = el.getChildText(VALUE_NAME);            return new Translation(key, originalLocale.get(key), currentValue);        }).collect(Collectors.toList());    }    @Override    public void updateTranslation(final String key, final String newTranslation) {        Element translationToUpdate = getValueElement(key);        translationToUpdate.setText(newTranslation);    }    private Element getValueElement(final String key) {        XPathExpression<Element> expression = xPathFactory.compile(/*/                + ELEMENT_NAME + [@ + KEY_NAME + =' + key + ']/                + VALUE_NAME, Filters.element());        // ohh damn that's so many assumptions        Element element = expression.evaluate(translationDocument).get(0);        return element;    }    @Override    public void save() {        XMLOutputter outputter = new XMLOutputter(Format.getPrettyFormat());        Path outFile = currentPath.resolve(fileNameString(currentTargetLocale));        try (OutputStream outStream = Files.newOutputStream(outFile,                StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE)) {            outputter.output(translationDocument, outStream);        } catch (IOException e) {            e.printStackTrace(System.err);            presenter.onException(e, Could not save File);        }    }    @Override    public Translation getSingleTranslation(final String key) {        final String currentValue = getValueElement(key).getText();        return new Translation(key, originalLocale.get(key), currentValue);    }}This is the model-class I currently use. The whole code in it's current state can be viewed on githubWhat I'm worried about most in this class: The state machine I have running in loadResxFileMy usage of JDOM and general Document manipulationExtensibility to additional resx features like CommentsAs always interesting to me are comments concerning:NamingPerformanceEverything that catches your eye :D
Translating the Rubberduck - bersetzen der Gummiente - Part 1: Modelling
java;xml;i18n;rubberduck
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 issuesI 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<Element> translationElements = translationDocument            .getRootElement().getChildren(ELEMENT_NAME);would be easier to follow as:    List<Element> 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 concernsWhat I'm worried about most in this class:The state machine I have running in loadResxFileMy usage of JDOM and general Document manipulationExtensibility to additional resx features like CommentsThe first thing I notice is regarding point 3. You have:private final Map<String, String> originalLocale = new HashMap<String, String>();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<String, String> originalLocale = new HashMap<String, String>();To:private final List<LanguageValue> originalLocale = new List<LanguageValue>();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<Element> 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<LanguageValue> 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 idea with these last two suggestions is that OverviewModelImpl doesn't really need to care how the XML is structured for each LanguageValue, it only needs to care about how to get the document to LanguageValue for construction of the List<LanguageValue>.I don't notice anything glaring about your document manipulation, other than one concern about this:    XPathExpression<Element> expression = xPathFactory.compile(/*/            + ELEMENT_NAME + [@ + KEY_NAME + =' + key + ']/            + VALUE_NAME, Filters.element());I don't like that at all. It just seems, wrong, somewhere. I'm not sure how it could be better, but that bit seems less-than-ideal.