Skip to main content
Bounty Ended with 50 reputation awarded by Vogel612
added 315 characters in body
Source Link
TheCoffeeCup
  • 9.4k
  • 4
  • 36
  • 96

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.

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...

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).

I don't really like seeing empty return statements in Java, as there is always a way around them.

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...

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.

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...

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).

Source Link
TheCoffeeCup
  • 9.4k
  • 4
  • 36
  • 96

##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.

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...

##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.

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...