2

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
  • Is it possible to return normalize(text) rather than null in the getNonEmptyTranslation(String) in the first approach?
    – SOFe
    Commented Jul 28, 2016 at 9:10
  • @PEMapModder: Both approaches are eligible. The question is about which is better of both. Having getNonEmptyTranslation(String) return untranslated text will worsen self documentation of code even further. Commented Jul 28, 2016 at 9:14
  • This might be a good question for Code Review
    – null
    Commented Jul 28, 2016 at 9:15
  • I think the first result.setText in the first approach is incorrect, it should set translation instead of text. Commented Jul 28, 2016 at 17:37
  • @MarkusUnterwaditzer: you're right. I fixed it. Commented Jul 29, 2016 at 14:10

3 Answers 3

4

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
  • That's a good approach. In the original code, where this problem came up, there's more code around the if block, so a return would break the code flow. But it can be refactored and extracted to a own method Commented Jul 29, 2016 at 14:15
7

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.

1

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
  • You substituted useFallback by translation but the code is still clean and readable. Nice solution. Commented Jul 29, 2016 at 14:20
  • Thanks. If you use this approach, you might want to rename translation to something like "validTranslation".
    – user949300
    Commented Jul 29, 2016 at 16:34

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.