I’m interested in which approach to prefer. Consider some code which retrieves a translation for some text. It has to respect these constraints:
- Return translation only if the text will be used on a UI. If it’s used some where else (debug log), return the normalized version of the text.
- Text normalization is costly (cpu resources). We should do it only when it is required.
- Loading the translation is costly as well.
- If translation for the text is missing or empty, return normalized original text as well.
First approach:
boolean useFallback = true;
if (textUsedForUi()) {
final String translation = getNonEmptyTranslation(text);
if (translation != null) {
result.setText(translation);
useFallback = false;
}
}
if (useFallback) {
result.setText(normalize(text));
}
String getNonEmptyTranslation(String text) {
String translation = loadTranslation(text);
if (StringUtils.isBlank(translation)) {
return null;
} else {
return translation;
}
}
Second approach:
String translation = getTranslation(text);
if (translation != null) {
result.setText(translation);
} else {
result.setText(normalize(text));
}
String getTranslation(String text) {
if (!textUsedForUi()) {return null;}
String translation = loadTranslation(text);
if (StringUtils.isBlank(translation)) {
return null;
} else {
return translation;
}
}
First approach clearly documents that the translation depends on where the text will be used and if a translation is available. But I have to introduce a helper variable to avoid code duplication and not recommendable assignments in conditions:
String translation;
if (textUsedForUi() && (translation = getNonEmptyTranslation(text)) != null)
I don’t want to rely on the previous value of result.getText(), so I can’t use this as a flag whether to use the fallback or not.
The second approach is clearly simpler (reduced cyclomatic complexity). But it hides the dependency to where the text is used in a sub routine. This reduces the self documenting nature of the code.
Which approach to prefer?
5
I think the answer depends on whether getNonEmptyTranslation or getTranslation would be more generally reusable, outside of this particular piece of code. If neither are used outside of this routine, I would consider inlining the function. Apart from that you can replace the useFallback variable with a return:
if (textUsedForUi()) {
final String translation = getNonEmptyTranslation(text);
if (translation != null && StringUtils.isBlank(translation)) {
result.setText(translation);
return;
}
}
result.setText(normalize(text));
1
The second version is worse because the getTranslation() method does not necessarily do what its name indicates, but switches its behavior due to a totally unrelated factor. Confusing and misleading code is much worse than a few points of CC.
The “nonEmpty” part of getNonEmptyTranslation() is pointless. One could check for empty just as easy as for null. Nor do you need a fallback variable. So, based on that, I’d suggest
String translation = "";
if (textUsedForUi()) {
translation = getTranslation(text); // return empty if none
if (transation.length() > 0)
result.setText(translation);
else
result.setText(normalize(text));
One could shorten the code even more with tertiary operators (? 🙂 but I think it’s clearer without them.
2