Enum's toString method and Java good practice

Question

I'd like to ask about good coding practice in Java. I want to create a enumeratoin of some properties and override toString() to use it in the following way (JSF 1.2 is used in order to retrieve localized message):

package ua.com.winforce.casino.email.util;

import java.text.MessageFormat;
import java.util.Locale;
import java.util.MissingResourceException;
import java.util.ResourceBundle;

import javax.faces.context.FacesContext;

public enum StatisticItems {
    MESSAGES,
    VIEWS;

    private static String BUNDLE_NAME = "messages";

    public String toString(){
        switch (this) {
        case MESSAGES:
            return getLocalizedMsg("messages.title");
        case VIEWS:
            return getLocalizedMsg("views.title");
        default:
            return null;
        }
    }

    private static String getLocalizedMsg(String key, Object... arguments) {
        Locale locale = FacesContext.getCurrentInstance().getViewRoot().getLocale();
        String resourceString;
        try {
            ResourceBundle bundle = ResourceBundle.getBundle(BUNDLE_NAME, locale);
            resourceString = bundle.getString(key);
        } catch (MissingResourceException e) {
            return key;
        }

        if (arguments == null) {
            return resourceString;
        }
        MessageFormat format = new MessageFormat(resourceString, locale);
        return format.format(arguments);
    }
}

My question is about good practice. Is it considered good to put all such methods within a enum definition? If not, I'd like to understand why and of course how to do it better.

Seems like a bad idea to have a `toString()` method complicated enough that it might throw an exception when you are trying to print a debugging message. — khelwood, Feb 24 '15 at 10:45
In the future JSF-related questions, you'd better not tag [java]. No one of the answerers took `FacesContext` into account, rendering the both answers technically wrong in JSF context. And, you'd better post code review requests at codereview.se, not at SO. Or at least eliminate subjectiveness/argumentativeness (including words like "best practice") from the question. For the real answer, do a search here using the three simple keywords "jsf", "enum" "localization". I've answered this kind of question before. — BalusC, Feb 24 '15 at 11:18

score 3 · Accepted Answer · edited May 23 '17 at 11:50

3

There are two points to be made here:

If the default case (returning null in your code) is a runtime error, then using a switch is kind of error prone. There are two better alternatives in my opinion:
1. Use a field localizationKey, initialized in the constructor of the enum instances, and refer to this key in the toString method
2. Or, (for more complicated cases) make the toString abstract and force each instance to override with proper implementation. See this question for example.
Many people argue that toString is intended either for really obvious implementations, otherwise only for debugging. (See this question for a good elaboration.) My recommendation: Come up with a more descriptive method name and don't reuse toString just for convenience.

Update: Zooming out from Java semantics a bit: This logic belongs to the view and not the model, as pointed out by BalusC in the comments below.

edited May 23 '17 at 11:50

Community

1
1

answered Feb 24 '15 at 10:43

aioobe

413,195
112
811
826

Overriding on a per-value basis isn't needed here - it would be cleaner just to private the message name in a constructor, store it in a field, then use it in a single implementation. – Jon Skeet Feb 24 '15 at 10:44
1. What do you mean I can override toString for each case? Don't you mean toString(StatisticItems), do you? – user3663882 Feb 24 '15 at 10:45
Thanks @JonSkeet. Answer updated. user3663882, see link on item 1.2. – aioobe Feb 24 '15 at 10:48
1

More elegant would be to completely move the localization from the model to the view. This is very definitely not model's responsibility. – BalusC Feb 24 '15 at 11:24
1

@BalusC, I didn't think of that aspect, but I agree to 100%. Added a sentence at the bottom of the answer. – aioobe Feb 24 '15 at 11:29

score 0 · Answer 2 · answered Feb 24 '15 at 10:56

0

I would ensure all of that complex logic is done once only at initialise time.

public enum StatisticItems {

    MESSAGES("messages"),
    VIEWS("views");
    final String asString;

    StatisticItems(String localised) {
        asString = getLocalizedMsg(localised + ".title");
    }

    @Override
    public String toString () {
        return asString;
    }

    private static String BUNDLE_NAME = "messages";
    private static final Locale locale = FacesContext.getCurrentInstance().getViewRoot().getLocale();
    private static final ResourceBundle bundle = ResourceBundle.getBundle(BUNDLE_NAME, locale);

    private static String getLocalizedMsg(String key) {
        String resourceString;
        try {
            resourceString = bundle.getString(key);
        } catch (MissingResourceException e) {
            return key;
        }

        if (arguments == null) {
            return resourceString;
        }
        MessageFormat format = new MessageFormat(resourceString, locale);
        return format.format(arguments);
    }
}

answered Feb 24 '15 at 10:56

OldCurmudgeon

64,482
16
119
213

I'm not sure it's desirable to put all the complex logic which in this case includes loading ResourceBundles in costructors (let alone enum constructors, considering they are executed upon class loading). See for instance [Is doing a lot in constructors bad?](http://stackoverflow.com/questions/7048515/is-doing-a-lot-in-constructors-bad). – aioobe Feb 24 '15 at 10:58
@aioobe - If that seems a little too heavyweight there are various mechanisms to do it later such as [Multiton](http://stackoverflow.com/a/18149547/823393)s. – OldCurmudgeon Feb 24 '15 at 11:02
Oh please no. Locale is in this particular context HTTP request / JSF view based, which is very definitely not application wide. In the future, please actually test the code yourself before doing off as an answer. – BalusC Feb 24 '15 at 11:22
@BalusC - That code was copied from OP. All I did was make it `static final`. But I agree that locale should come from context. – OldCurmudgeon Feb 24 '15 at 11:26
Uh? OP did it right on a threadlocal basis, yet you made it a constant and introduced a potential NPE. – BalusC Feb 24 '15 at 11:27

Enum's toString method and Java good practice

2 Answers2