##Main
What is the purpose of:
private Main() {
}
I can currently think of two possibilities:
To prevent inheritance: Easily fixed with the final
keyword.
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:
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";
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...