4
\$\begingroup\$
public String getDatePeriodtext(boolean userCanJoinAndLeave, Date startDate, Date endDate) {
    String text;

    if (!userCanJoinAndLeave) {
        text = getText(getLocale(), "no");
    } else if (startDate == null && endDate == null) {
        text = getText(getLocale(), "continuous");
    } else if (startDate == null && endDate != null) {
        text = getText(getLocale(), "to")
                + " "
                + formatDate(endDate);
    } else if (endDate == null) {
        text = getText(getLocale(), "from")
                + " "
                + formatDate(startDate);
    } else {
        text = formatDate(startDate)
                + " "
                + getText(getLocale(), "to")
                + " "
                + formatDate(endDate);
    }

    return text;
}

Some scenarios:

  • userCanJoinAndLeave = false ==> No
  • startDate = null and endDate = null ==> Continuous
  • startDate = 2016-10-31 and endDate = null ==> from 2016-10-31
  • startDate = null and endDate = 2016-11-01 ==> to 2016-11-01
  • startDate = 2016-10-31 and endDate = 2016-11-01 ==> 2016-10-31 to 2016-11-01
\$\endgroup\$
  • \$\begingroup\$ You have a trailing semi-colon in formatDate(startDate); (the final else). Also, is it normal that it doesn't have "from ..."? \$\endgroup\$ – Tunaki Oct 31 '16 at 11:21
  • \$\begingroup\$ @Tunaki yeah, my bad about the semi-colon. Yes, if it has a startDate and an endDate it doesn't need "from". \$\endgroup\$ – Tn Hn Oct 31 '16 at 13:20
  • \$\begingroup\$ This isn't a [design-patterns] question, it is a code-style question (for which we seem not to have a tag). \$\endgroup\$ – PellMel Oct 31 '16 at 13:53
  • \$\begingroup\$ Do you have unit tests testing this behaviour ? \$\endgroup\$ – Spotted Nov 1 '16 at 8:54
2
\$\begingroup\$

Concatenating Strings

I generally recommend avoiding forming strings by concatenating several smaller ones via the string-concatenation operator, at least in frequently-executed code. It used to be that doing so would definitely produce unneeded, short-lived temporary objects. That's only somewhat better now: more recent Java reduces it from a certainty to merely a possibility.

Too-narrow i18n

There's a bad code smell to internationalizing on a word-by-word basis ("to", "from", "no") instead of on a message-by-message basis. On one hand, you risk running into trouble when different messages that both use the same word (e.g. "from") in the default locale require different words in some other locale. More generally, you probably won't need to sample too many locales before discovering one where the message overall needs to take a different form, such that just substituting one word for another is insufficient.

You have five different forms of message you want to be able to present. I would recommend providing internationalized format strings for each whole message, which you then use with String.format() to produce the localized messages. Guessing a bit about how your getText() method is intended to work (or could be made to do), that might look something like this:

public String getDatePeriodtext(boolean userCanJoinAndLeave, Date startDate, Date endDate) {
    Locale locale = getLocale();
    String format;

    if (!userCanJoinAndLeave) {
        format = getText(locale, "date period none");
    } else if (startDate == null && endDate == null) {
        format = getText(locale, "date period continuous");
    } else if (startDate == null) {
        format = getText(locale, "date period closed end");
    } else if (endDate == null) {
        format = getText(locale, "date period closed start");
    } else {
        format = getText(locale, "date period closed both");
    }

    return String.format(locale, format, startDate, endDate);
}

(Or even better, use static final strings belonging to the class instead of string literals.)

The localized format strings on which that method relies will use argument specifiers to designate which argument to format, so as to be able to accept the same (some possibly unused) field values in every case. For example, in the en-US locale, some of them might be

"from %1$F, %1$T"
"%1$F, %1$T to %2$F, %2$T"

Code style

Whether you write an if / else chain with common actions afterward (my preference, and your original approach) or a series of separate if blocks, each of which returns, do put the body of each conditional block inside braces ({ ... }), contrary to the example presented in another answer. Omitting the braces creates a big maintenance booby trap, as last year's major Apple SSL vulnerability demonstrated.

\$\endgroup\$
3
\$\begingroup\$

I am not sure if you like this one better but I personally prefer returning directly (and therefore omitting the else-clauses). I do not know if there was a better way.

public String getDatePeriodtext(boolean userCanJoinAndLeave, Date startDate, Date endDate) {

  if (!userCanJoinAndLeave)
    return getText(getLocale(), "no");

  if (startDate == null && endDate == null)
    return getText(getLocale(), "continuous");

  if (startDate == null && endDate != null)
    return getText(getLocale(), "to")
            + " "
            + formatDate(endDate);

  if (endDate == null)
    return getText(getLocale(), "from")
            + " "
            + formatDate(startDate);

  return formatDate(startDate);
          + " "
          + getText(getLocale(), "to")
          + " "
          + formatDate(endDate);
}
\$\endgroup\$
  • \$\begingroup\$ That does make it more readable, I agree. I am, however, curious about different design patterns one can employ to avoid mulitple if statements but don't know if it is possible in this scenario. \$\endgroup\$ – Tn Hn Oct 31 '16 at 13:04
  • \$\begingroup\$ @TnHn You want 5 different and unrelated outputs. That means at the very least 4 different conditions. You're nearly in the "FizzBuzz" school-case. You could probably join cases together if you want like grouping the two where startDate == null. \$\endgroup\$ – Olivier Grégoire Oct 31 '16 at 15:11
2
\$\begingroup\$

There is only one thing I considered as recognizable: the dolphin among the fishes. I really recommend to handle the artefact "userCanJoinAndLeave" in the method that is calling the method "getDatePeriodtext". It seems semantically anorganic here. This relates to the single responsibility principle.

Be careful with multiple return statements as suggested. They hinder you to apply refactorings like "extract" method. But in this case it may be no problem at all.

I also recommend full (metal) brackets ({...}) in general. The code is trivial. So no real issue here.

After playing around I came up with a totally different structure with less null checks and less string redundancy. Do not take this too serious. But maybe you get the idea.

public String getDatePeriodtext1(Date startDate, Date endDate) {

    List<String> l = new ArrayList<>();

    if (startDate != null) {
        l.add(getText(getLocale(), "from"));
        l.add(formatDate(startDate));
    }

    if (endDate != null) {
        l.add(getText(getLocale(), "to"));
        l.add(formatDate(endDate));
    }

    if (l.size() == 4) {
        l.remove(0);
    }

    if (l.isEmpty()) {
        l.add(getText(getLocale(), "continuous"));
    }

    return l.stream().collect(Collectors.joining(" "));
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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