14
\$\begingroup\$

This project is my learning place for a few things:

  • MVP (model view presenter)
  • XML (parsing, editing and leveraging)
  • deeper swing functionality

Concerning the XML part I have already recieved a very nice review by rolfl on my previous question. Since then quite a lot of things have changed and the current state of the code is available on github

I implemented a few features, the most significant change since back then may be the free choice of translated locale. In addition to that I now support an "Unsaved Changes" dialog upon closing.
Furthermore I have removed interfaces that have only a single implementation (so basically, all), except for the OverviewView, which I want to implement with a different UI provider than swing.

Enter the Translation Helper.

As Entry point serves your trusty Main-Class:

public class Main {
    public static final String RUBBERDUCK_PATH = "RetailCoder.VBE/UI";
    public static final String ARGUMENT_MISMATCH = "Arguments do not match up. Please provide one single path to read the Rubberduck resx from";
    public static final String ILLEGAL_FOLDER = "Rubberduck .resx files can only be found under RetailCoder.VBE/UI. Please give a path that points to a Rubberduck UI folder";

    private Main() {
    }

    public static void main(final String[] args) {
        // parsing the first argument given into a proper path to load the resx
        // from
        if (args.length != 1 && args.length != 3) {
            // don't even bother!
            System.out.println(ARGUMENT_MISMATCH);
            return;
        }
        Path resxFolder = Paths.get(args[0]);
        // normalize path to allow checking
        resxFolder = resxFolder.normalize();

        if (!resxFolder.endsWith(RUBBERDUCK_PATH)) {
            System.out.println(ILLEGAL_FOLDER);
            return;
        }

        TranslationPresenter tp = new TranslationPresenter();
        OverviewModel m = new OverviewModel();
        OverviewView v = new SwingOverviewView();

        OverviewPresenter p = new OverviewPresenter(m, v, tp);
        p.initialize();
        p.loadFiles(resxFolder);
        // set the selected locales if they were specified on commandline
        // check whether they are available before that and fall back if they aren't
        if (args.length == 3) {
            final String leftLocale = args[1];
            final String rightLocale = args[2];
            if (m.getAvailableLocales().contains(leftLocale) && m.getAvailableLocales().contains(rightLocale)) {
                p.onTranslationRequest(leftLocale, Side.LEFT);
                p.onTranslationRequest(rightLocale, Side.RIGHT);
            }
            // "fallback"
        }
        p.show();
    }

}

Main does quite some things actually. The arguments given are parsed and run through a sanity check. Then we fire up the Presenter, Model and View and wire them together. Surely I could clean this up a little, but I didn't find it necessary yet...

Candidates, present yourself:

The presenter has a significant working field. It is the access point for the application and controls View as well as Model and is managing their interactions. User actions that cannot be handled by the View get propagated to the presenter. There a decision between 3 Options is made:

  1. Handle yourself
  2. Delegate to model
  3. Delegate to a separate presenter

This gets us to following class

public class OverviewPresenter {

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

    private final Map<Side, String> chosenLocale = new EnumMap<>(Side.class);
    private final OverviewModel model;
    private final OverviewView view;
    private final TranslationPresenter translationPresenter;

    private boolean initialized = false;

    public OverviewPresenter(final OverviewModel m, final OverviewView v, final TranslationPresenter p) {
        model = m;
        view = v;
        translationPresenter = p;

        view.initialize();
    }

    public void show() {
        if (!initialized) {
            initialize();
        }
        view.show();
    }

    public void initialize() {
        // initialization shall only happen once!
        if (initialized) {
            return;
        }
        view.register(this);
        model.register(this);
        translationPresenter.register(this);
        initialized = true;
    }

    public void onTranslationRequest(final String locale, final Side side) {
        chosenLocale.put(side, locale);
        rebuildView();
    }

    public void onException(final Exception e, final String message) {
        view.displayError(message, e.getMessage());
    }

    public void onParseCompletion() {
        rebuildView();
    }

    private void rebuildView() {
        List<Translation> left = model.getTranslations(chosenLocale.getOrDefault(Side.LEFT, DEFAULT_ROOT_LOCALE));
        List<Translation> right = model.getTranslations(chosenLocale.getOrDefault(Side.RIGHT, DEFAULT_TARGET_LOCALE));
        view.rebuildWith(left, right);
    }

    public void loadFiles(final Path resxFolder) {
        model.loadFromDirectory(resxFolder);
    }

    public String[] getLocaleOptions() {
        return model.getAvailableLocales().toArray(new String[]{});
    }

    public void onTranslationSubmit(final Translation t) {
        translationPresenter.hide();
        model.updateTranslation(t.getLocale(), t.getKey(), t.getValue());
        rebuildView();
    }

    public void onTranslationAbort() {
        translationPresenter.hide();
    }

    public void onTranslateRequest(final String key) {
        translationPresenter.setRequestedTranslation(
          model.getSingleTranslation(chosenLocale.getOrDefault(Side.LEFT, DEFAULT_ROOT_LOCALE), key),
          model.getSingleTranslation(chosenLocale.getOrDefault(Side.RIGHT, DEFAULT_TARGET_LOCALE), key)
        );
        translationPresenter.show();
    }

    public void onSaveRequest() {
        model.saveAll();
    }

    public void onWindowCloseRequest(WindowEvent windowEvent) {
        if (model.isNotSaved()) {
            // prompt to save changes
            int choice = JOptionPane.showConfirmDialog(windowEvent.getWindow(),
              "You have unsaved changes. Do you wish to save before exiting?",
              "Unsaved Changes",
              JOptionPane.YES_NO_CANCEL_OPTION);
            switch (choice) {
                case JOptionPane.YES_OPTION:
                    model.saveAll();
                    // fallthrough intended
                case JOptionPane.NO_OPTION:
                    view.hide();
                    System.exit(0);
                    break;
                case JOptionPane.CANCEL_OPTION:
                    // do nothing
                    break;
            }
        } else {
            System.exit(0);
        }
    }
}

No presenting without a view:

Whoever expected goodies now, will be sorely disappointed. The Translation Helper is incredibly ugly. Well at least it resizes nicely, has two columns and looks okay enough while doing that. That's enough for me.

To make this as simple as possible for me I settled on using a GridBagLayout to enable resizing without any additional code from my side. On initialization I set some constraints, and that's it. Well not quite. After the layouting is done there's basically two things that can happen:

  • The locales to display change
  • The presenter changes

For both cases I need to ensure integrity of functionality, and as such these two things happen in methods called externally. That said here's the Swing code. Anyone not interested in boring manual layouting and event-bindings should skip this block:

public class SwingOverviewView implements OverviewView {

    private static final Dimension MINIMUM_WINDOW_SIZE = new Dimension(800, 500);
    private static final Dimension DEFAULT_WINDOW_SIZE = new Dimension(1000, 700);

    private static final Dimension MENU_BAR_DIMENSION = new Dimension(800, 100);
    private static final Dimension BUTTON_DIMENSION = new Dimension(100, 40);

    private final JFrame window;
    private final JTable translationContainer;
    private final JPanel menuBar;
    private final JButton saveButton;
    private final JButton chooseLeft;
    private final JButton chooseRight;

    private OverviewPresenter presenter;

    public SwingOverviewView() {
        window = new JFrame("Rubberduck Translation Helper");
        window.setDefaultCloseOperation(JFrame.DO_NOTHING_ON_CLOSE);

        translationContainer = new JTable();
        translationContainer.setModel(new TranslationTable());
        menuBar = new JPanel();
        saveButton = new JButton("save");
        chooseLeft = new JButton("choose left");
        chooseRight = new JButton("choose right");
    }

    @Override
    public void register(final OverviewPresenter p) {
        presenter = p;
        saveButton.addActionListener(event -> presenter.onSaveRequest());
        chooseLeft.addActionListener(event -> chooseAndLoadLanguage(Side.LEFT));
        chooseRight.addActionListener(event -> chooseAndLoadLanguage(Side.RIGHT));

        window.addWindowListener(new WindowListener() {

            @Override
            public void windowOpened(WindowEvent windowEvent) {
                // nothing
            }

            @Override
            public void windowClosing(WindowEvent windowEvent) {
                p.onWindowCloseRequest(windowEvent);
            }

            @Override
            public void windowClosed(WindowEvent windowEvent) {
                // nothing
            }

            @Override
            public void windowIconified(WindowEvent windowEvent) {
                // nothing
            }

            @Override
            public void windowDeiconified(WindowEvent windowEvent) {
                // nothing
            }

            @Override
            public void windowActivated(WindowEvent windowEvent) {
                // nothing
            }

            @Override
            public void windowDeactivated(WindowEvent windowEvent) {
                // nothing
            }
        });
    }

    private void chooseAndLoadLanguage(Side side) {
        String locale = chooseLocale();
        presenter.onTranslationRequest(locale, side);
    }

    private String chooseLocale() {
        String[] localeOptions = presenter.getLocaleOptions();
        int selectedOption = JOptionPane.showOptionDialog(window,
          "Please choose the Locale out of following options:",
          "Choose Locale",
          JOptionPane.DEFAULT_OPTION,
          JOptionPane.QUESTION_MESSAGE,
          null,
          localeOptions,
          null);
        return localeOptions[selectedOption];
    }

    @Override
    public void initialize() {
        window.setLayout(new GridBagLayout());
        window.setSize(DEFAULT_WINDOW_SIZE);
        window.setMinimumSize(MINIMUM_WINDOW_SIZE);
        window.setBackground(new Color(0.2f, 0.3f, 0.7f, 1.0f));

        addMenuBar();
        addTranslationContainer();
        window.doLayout();
    }

    private void addTranslationContainer() {
        GridBagConstraints constraints = new GridBagConstraints();
        constraints.insets = new Insets(15, 15, 15, 15);
        constraints.weightx = 1.0;
        constraints.weighty = 1.0;
        constraints.fill = BOTH;
        constraints.gridx = 0;
        constraints.gridy = 1;

        JScrollPane scroller = new JScrollPane(translationContainer);
        scroller.setMinimumSize(new Dimension(800, 400));
        scroller.setSize(new Dimension(800, 400));
        window.add(scroller, constraints);
        bindEventListener();

        translationContainer.setDefaultRenderer(Object.class,
          new TranslationTableRenderer());
    }

    private void bindEventListener() {
        translationContainer.addMouseListener(new MouseListener() {

            @Override
            public void mouseClicked(final MouseEvent event) {
                if (event.getClickCount() != 2) { // only react to doubleclicks!
                    return;
                }
                final int row = translationContainer.rowAtPoint(event
                  .getPoint());
                final String key = ((TranslationTable) translationContainer
                  .getModel()).getKeyAt(row);
                presenter.onTranslateRequest(key);
            }

            @Override
            public void mouseEntered(final MouseEvent arg0) {
                // IGNORE
            }

            @Override
            public void mouseExited(final MouseEvent arg0) {
                // IGNORE
            }

            @Override
            public void mousePressed(final MouseEvent arg0) {
                // IGNORE
            }

            @Override
            public void mouseReleased(final MouseEvent arg0) {
                // IGNORE
            }

        });
    }

    private void addMenuBar() {
        GridBagConstraints constraints = new GridBagConstraints();
        constraints.insets = new Insets(15, 15, 15, 15);
        constraints.gridx = 0;
        constraints.gridy = 0;
        constraints.weightx = 1.0;
        constraints.weighty = 0.0;
        constraints.fill = BOTH;
        menuBar.setLayout(new GridBagLayout());
        menuBar.setBackground(new Color(0.4f, 0.2f, 0.4f, 0.2f));

        addToGridBag(menuBar, window, MENU_BAR_DIMENSION, constraints);

        GridBagConstraints buttonConstraints = (GridBagConstraints) constraints.clone();
        buttonConstraints.gridx = GridBagConstraints.RELATIVE;
        addToGridBag(chooseLeft, menuBar, BUTTON_DIMENSION, buttonConstraints);
        addToGridBag(chooseRight, menuBar, BUTTON_DIMENSION, buttonConstraints);
        addToGridBag(saveButton, menuBar, BUTTON_DIMENSION, buttonConstraints);
    }

    @Override
    public void rebuildWith(final List<Translation> left, final List<Translation> right) {
        translationContainer.setModel(new TranslationTable(left, right));
    }

    @Override
    public void displayError(final String title, final String errorMessage) {
        JOptionPane.showMessageDialog(window, errorMessage, title,
          JOptionPane.ERROR_MESSAGE);
    }

    @Override
    public void show() {
        window.setVisible(true);
    }

    @Override
    public void hide() {
        window.setVisible(false);
    }
}

But what should I show?

Exactly. That's what the Model is responsible for. It does a significant bit of the actually interesting functionality, namely:

  • Parsing .resx files at a location
  • Writing edited .resx files back to that location

For that it relies on the java.nio-API and JDOM, as well as the new Streams. This is the interesting part that would need a whole rewrite to support arbitrary files and other interesting stuff. Luckily that is not what I want :)

public class OverviewModel {

    public static final String VALUE_NAME = "value";
    public static final String KEY_NAME = "name";
    public static final String SINGLE_TRUTH_LOCALE = "";

    private static final String ELEMENT_NAME = "data";
    private static final String FILE_NAME_FORMAT = "RubberduckUI%s.resx";
    private static final String FILENAME_REGEX = "^.*RubberduckUI\\.?([a-z]{2})?\\.resx$";

    private static final Pattern localeFinder = Pattern.compile(FILENAME_REGEX);

    private final Map<String, Document> translations = new HashMap<>();
    private final XPathFactory xPathFactory = XPathFactory.instance();
    private final XPathExpression<Element> valueExpression = xPathFactory.compile("/*/"
      + ELEMENT_NAME + "[@" + KEY_NAME + "=$key]/"
      + VALUE_NAME, Filters.element(), Collections.singletonMap("key", ""));

    private OverviewPresenter presenter;
    private Path currentPath;
    private final AtomicBoolean saved = new AtomicBoolean(true);
    public static final XMLOutputter XML_PRETTY_PRINT = new XMLOutputter(Format.getPrettyFormat());

    private static String parseFileName(final Path path) {
        final Matcher localeMatcher = localeFinder.matcher(path.getFileName().toString());
        if (localeMatcher.find()) { // should always be true, since we check beforehand
            final String locale = localeMatcher.group(1) == null
                                  ? SINGLE_TRUTH_LOCALE
                                  : localeMatcher.group(1);
            return locale;
        }
        throw new IllegalArgumentException("Argument was not a conform resx file");
    }

    public void register(final OverviewPresenter p) {
        presenter = p;
    }

    public void loadFromDirectory(final Path resxFolder) {
        this.currentPath = resxFolder;
        translations.clear();

        try (Stream<Path> resxFiles = Files.find(resxFolder, 1, (path,
            properties) -> path.toString().matches(FILENAME_REGEX),
          FileVisitOption.FOLLOW_LINKS)) {
            translations.putAll(resxFiles.collect(Collectors.toMap(
                OverviewModel::parseFileName, this::parseFile)
            ));
        } catch (IOException ex) {
            String errorMessage = String.format(
              "Could not access %s due to %s", resxFolder, ex);
            System.err.println(errorMessage);
            presenter.onException(ex, errorMessage);
        }

        normalizeDocuments();
        presenter.onParseCompletion();
    }

    private void normalizeDocuments() {
        final Set<String> singleTruth = translations
          .get(SINGLE_TRUTH_LOCALE)
          .getRootElement()
          .getChildren(ELEMENT_NAME)
          .stream()
          .map(el -> el.getAttribute(KEY_NAME).getValue())
          .collect(Collectors.toSet());

        translations.values().forEach(
          doc -> normalizeDocument(doc, singleTruth));
        saved.lazySet(false);
    }

    private void normalizeDocument(final Document doc, final Set<String> singleTruth) {
        final List<Element> localeElements = doc.getRootElement().getChildren(ELEMENT_NAME);
        Set<String> localeKeys = new HashSet<>();

        // remove keys not present in the Single truth
        for (Iterator<Element> it = localeElements.iterator(); it.hasNext(); ) {
            final Element el = it.next();
            if (!singleTruth.contains(el.getAttribute(KEY_NAME).getValue())) {
                it.remove();
                continue;
            }
            localeKeys.add(el.getAttribute(KEY_NAME).getValue());
        }

        singleTruth.stream()
          .filter(key -> !localeKeys.contains(key))
          .map(OverviewModel::createNewElement)
          .forEach(doc.getRootElement()::addContent);
    }

    private static Element createNewElement(String key) {
        Element newElement = new Element(ELEMENT_NAME);
        Element valueContainer = new Element(VALUE_NAME);
        valueContainer.setText("");

        newElement.setAttribute(KEY_NAME, key);
        newElement.addContent(valueContainer);
        return newElement;
    }

    private Document parseFile(final Path path) {
        final Path xmlFile = path.getFileName();
        final SAXBuilder documentBuilder = new SAXBuilder();

        final Document doc;
        try {
            doc = documentBuilder.build(path.toFile());
            return doc;
        } catch (JDOMException e) {
            presenter.onException(e, "Unspecified Parsing error");
            throw new IllegalStateException("Unable to parse " + xmlFile, e);
        } catch (IOException e) {
            presenter.onException(e, "Unspecified I/O Error");
            throw new UncheckedIOException("Unable to read" + xmlFile, e);
        }
    }

    public List<Translation> getTranslations(final String locale) {
        Document document = translations.get(locale);
        final List<Element> translationElements = document.getRootElement()
          .getChildren(ELEMENT_NAME);

        return translationElements.stream()
          .map(el -> new Translation(locale, el))
          .sorted(Comparator.comparing(Translation::getKey))
          .collect(Collectors.toList());
    }

    public void updateTranslation(final String locale, final String key,
      final String newTranslation) {
        Element translationToUpdate = getValueElement(locale, key);
        translationToUpdate.setText(newTranslation);
    }

    private Element getValueElement(final String locale, final String key) {
        valueExpression.setVariable("key", key);
        return valueExpression.evaluateFirst(translations.get(locale));
    }

    public void saveAll() {
        for (Map.Entry<String, Document> entry : translations.entrySet()) {
            final Path outFile = currentPath.resolve(fileNameString(entry
              .getKey()));
            try (OutputStream outStream = Files.newOutputStream(outFile,
              StandardOpenOption.TRUNCATE_EXISTING,
              StandardOpenOption.WRITE)) {
                XML_PRETTY_PRINT.output(entry.getValue(), outStream);
                saved.lazySet(true);
            } catch (IOException e) {
                e.printStackTrace(System.err);
                presenter.onException(e, "Could not save File");
            }
        }
    }

    private String fileNameString(final String locale) {
        return String.format(FILE_NAME_FORMAT, locale.isEmpty() ? "" : "." + locale.toLowerCase());
    }

    public Translation getSingleTranslation(final String locale,
      final String key) {
        final String currentValue = getValueElement(locale, key).getText();
        return new Translation(locale, key, currentValue);
    }

    public List<String> getAvailableLocales() {
        return new ArrayList<>(translations.keySet());
    }

    public boolean isNotSaved() {
        return !saved.get();
    }
}

I left out a few things that are available on github for full executability, namely a small UI helper for common code in the gridbag setup process. Also I left out declarations for Side, Translation, the OverviewView interface and the TranslationPresenter that's responsible for the actual editing of a translation.

I plan to incorporate the changes suggested here and pack the code into a Jar for simple distribution and use as beta version.

I am especially interested in:

  • Separation of concerns between Model, View and Presenter
  • Necessity of a proper command line argument parser
  • Swing tricks to simplify the View

And as usual: all feedback is appreciated :)

\$\endgroup\$
1
  • \$\begingroup\$ It's not answer-worthy IMO, but some of your variables are named with only a letter, which is bad. And some comments aren't really useful \$\endgroup\$
    – IEatBagels
    Commented Nov 13, 2015 at 19:46

2 Answers 2

6
+50
\$\begingroup\$

Main

What is the purpose of:

private Main() {
}

I can currently think of two possibilities:

  1. To prevent inheritance: Easily fixed with the final keyword.

  2. To prevent other parts of code to instantiate your Main class: It doesn't really matter, does it? You have no code that is not static in the class, so instantiation does not matter.

public static final String RUBBERDUCK_PATH = "RetailCoder.VBE/UI";
public static final String ARGUMENT_MISMATCH = "Arguments do not match up. Please provide one single path to read the Rubberduck resx from";
public static final String ILLEGAL_FOLDER = "Rubberduck .resx files can only be found under RetailCoder.VBE/UI. Please give a path that points to a Rubberduck UI folder";

Two things:

  1. Your lines are long. To be in the 80 character limit (or as close as possible) while still avoiding string concatenation, do:

    public static final String RUBBERDUCK_PATH = "RetailCoder.VBE/UI";
    public static final String ARGUMENT_MISMATCH =
            "Arguments do not match up. Please provide one single path to read the Rubberduck resx from";
    public static final String ILLEGAL_FOLDER =
            "Rubberduck .resx files can only be found under RetailCoder.VBE/UI. Please give a path that points to a Rubberduck UI folder";
    
  2. Why are they public? It has no real use as a public field. Make the fields that aren't supposed to be seen private.

OverviewPresenter

public void initialize() {
    // initialization shall only happen once!
    if (initialized) {
        return;
    }
    view.register(this);
    model.register(this);
    translationPresenter.register(this);
    initialized = true;
}

I think it looks better this way:

public void initialize() {
    // initialization shall only happen once!
    if (!initialized) {
        view.register(this);
        model.register(this);
        translationPresenter.register(this);
        initialized = true;
    }
}

I don't really like seeing empty return statements in Java, as there is always a way around them. It's my opinion; you may think different, and that's fine.

public void onWindowCloseRequest(WindowEvent windowEvent) {
    if (model.isNotSaved()) {
        // ...
        switch (choice) {
            case JOptionPane.YES_OPTION:
                model.saveAll();
                // fallthrough intended
            case JOptionPane.NO_OPTION:
                view.hide();
                System.exit(0);
                break;
            case JOptionPane.CANCEL_OPTION:
                // do nothing
                break;
        }
    } else {
        System.exit(0);
    }
}

The last case is not required if it does nothing. If you really want to tell a reviewer/code-reader that it will do nothing, simply use a comment. It is also understandable, as only the yes and no options should do anything, and the cancel button should be completely ignored, as it is in many real-life applications. I cannot think of a single situation where a cancel button will do anything...

SwingOverviewView

    window.addWindowListener(new WindowListener() {

        @Override
        public void windowOpened(WindowEvent windowEvent) {
            // nothing
        }

        @Override
        public void windowClosing(WindowEvent windowEvent) {
            p.onWindowCloseRequest(windowEvent);
        }

        @Override
        public void windowClosed(WindowEvent windowEvent) {
            // nothing
        }

        @Override
        public void windowIconified(WindowEvent windowEvent) {
            // nothing
        }

        @Override
        public void windowDeiconified(WindowEvent windowEvent) {
            // nothing
        }

        @Override
        public void windowActivated(WindowEvent windowEvent) {
            // nothing
        }

        @Override
        public void windowDeactivated(WindowEvent windowEvent) {
            // nothing
        }
    });

Horrendous useless methods... Use a WindowAdapter instead; it's pretty much the same thing, the only difference being you don't need to specify all the methods:

    window.addWindowListener(new WindowAdapter() {

        @Override
        public void windowClosing(WindowEvent windowEvent) {
            p.onWindowCloseRequest(windowEvent);
        }

    });

Same thing here:

    translationContainer.addMouseListener(new MouseListener() {

        @Override
        public void mouseClicked(final MouseEvent event) {
            if (event.getClickCount() != 2) { // only react to doubleclicks!
                return;
            }
            final int row = translationContainer.rowAtPoint(event
              .getPoint());
            final String key = ((TranslationTable) translationContainer
              .getModel()).getKeyAt(row);
            presenter.onTranslateRequest(key);
        }

        @Override
        public void mouseEntered(final MouseEvent arg0) {
            // IGNORE
        }

        @Override
        public void mouseExited(final MouseEvent arg0) {
            // IGNORE
        }

        @Override
        public void mousePressed(final MouseEvent arg0) {
            // IGNORE
        }

        @Override
        public void mouseReleased(final MouseEvent arg0) {
            // IGNORE
        }

    });

Use a MouseAdapter:

    translationContainer.addMouseListener(new MouseAdapter() {

        @Override
        public void mouseClicked(final MouseEvent event) {
            if (event.getClickCount() != 2) { // only react to doubleclicks!
                return;
            }
            final int row = translationContainer.rowAtPoint(event
              .getPoint());
            final String key = ((TranslationTable) translationContainer
              .getModel()).getKeyAt(row);
            presenter.onTranslateRequest(key);
        }

    });

OverviewModel

private static final Pattern localeFinder = Pattern.compile(FILENAME_REGEX);

static final fields are usually ALL_CAPS_WITH_UNDERSCORES_AS_SPACES. You do fine with that everywhere, but here... localeFinder should be LOCALE_FINDER.

EDIT: I know that you don't like the spacing, but I will leave it here as a reference as it is the standard java conventions (eclipse formatting implies that too).

private void normalizeDocuments() {
    final Set<String> singleTruth = translations
      .get(SINGLE_TRUTH_LOCALE)
      .getRootElement()
      .getChildren(ELEMENT_NAME)
      .stream()
      .map(el -> el.getAttribute(KEY_NAME).getValue())
      .collect(Collectors.toSet());

    translations.values().forEach(
      doc -> normalizeDocument(doc, singleTruth));
    saved.lazySet(false);
}

Code that is part of the same command but is on a separate line should be 8-spaced:

private void normalizeDocuments() {
    final Set<String> singleTruth = translations
            .get(SINGLE_TRUTH_LOCALE)
            .getRootElement()
            .getChildren(ELEMENT_NAME)
            .stream()
            .map(el -> el.getAttribute(KEY_NAME).getValue())
            .collect(Collectors.toSet());

    translations.values().forEach(
            doc -> normalizeDocument(doc, singleTruth));
    saved.lazySet(false);
}

Again here:

private void normalizeDocument(final Document doc, final Set<String> singleTruth) {
    // ...

    singleTruth.stream()
            .filter(key -> !localeKeys.contains(key))
            .map(OverviewModel::createNewElement)
            .forEach(doc.getRootElement()::addContent);
}

And here:

public List<Translation> getTranslations(final String locale) {
    Document document = translations.get(locale);
    final List<Element> translationElements = document.getRootElement()
            .getChildren(ELEMENT_NAME);

    return translationElements.stream()
            .map(el -> new Translation(locale, el))
            .sorted(Comparator.comparing(Translation::getKey))
            .collect(Collectors.toList());
}

And a lot of other parts of your code, not just this class...

\$\endgroup\$
1
  • \$\begingroup\$ Concerning the 8 spaces. I have a personal dislike against indenting the 'same logical step' so deeply. Instead I generally use a half-indent in all my code that sees such patterns. In this case I favor consistent and personally preferred formatting over the 8 Spaces you propose. Aside from that this review found some missing details and taught me abou Adapters. Thanks for that :) \$\endgroup\$
    – Vogel612
    Commented Nov 3, 2015 at 0:11
10
\$\begingroup\$
public static final String RUBBERDUCK_PATH = "RetailCoder.VBE/UI";

This value shouldn't be compiled into the binary. It should be a configurable setting that's modifiable without needing to recompile.

"RetailCoder.VBE" is a relic of the original project, before it was even called "Rubberduck", when the Mug formerly known as @retailcoder was mucking around with the VBE API. The project has had 2 if not 3 major architecture changes in the past, nothing says it's not going to have another in the future: this RUBBERDUCK_PATH value is coupled with a project structure you have no control over, that you can't assume isn't going to ever change in the future.

The ILLEGAL_FOLDER message is also dependent on the RUBBERDUCK_PATH value:

public static final String ILLEGAL_FOLDER = "Rubberduck .resx files can only be found under RetailCoder.VBE/UI. Please give a path that points to a Rubberduck UI folder";

Why not just concatenate RUBBERDUCK_PATH into the message instead of making 2 places for it to change?


The FILENAME_REGEX works now:

private static final String FILENAME_REGEX = "^.*RubberduckUI\\.?([a-z]{2})?\\.resx$";

That will support a filename like RubberduckUI.de.resx, but Rubberduck could very well be tweaked one day, to support more localized translations - and a filename like Rubberduck.de-CH.resx or Rubberduck.de-AT wouldn't match the regex, despite being valid. Of course there shouldn't be much need to make a whole new translation for a localized translation.. but we can't know that - especially if/when we get into languages like zh-CN or zh-TW, which may require completely different wordings and tokens.

In other words:

private String fileNameString(final String locale) {

It's not a locale, it's really the language - the format being language-LOCALE, each being a 2-character code. Referring to the language code with "locale" might get confusing if/when localized translations are supported.

\$\endgroup\$
1
  • \$\begingroup\$ fixed in the latest version :) \$\endgroup\$
    – Vogel612
    Commented Feb 12, 2016 at 23:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.