Re: Another JUnit scenario
Lew wrote:
You never answered my question about why you wanted to use a static factory
method, let alone a factory class, in the first place. The question was
neither arbitrary nor insignificant.
Rhino wrote:
Actually, I did answer it a few hours ago on one of the other threads.
You answered it over two and half hours after I posted the above, in "Design
Questions about static factory classes". I didn't have my USB crystal ball
attached when I wrote that you hadn't. So sorry.
You answered, essentially, that you didn't know why you were using a static
factory and were just applying the principle by rote, cargo-cult style.
That's not programming.
Remember, the only hard and fast rule in programming is that there are no hard
and fast rules in programming. You cannot just go apply a hammer to
everything hoping that you're hitting a nail. Sometimes you're hitting a
crystal vase, with bad results.
You must THINK.
I'm darned if I know which one at this point; there are so many
threads going now with so many subthreads that I am absolutely dizzy
Rhino, it only took me five minutes of digging around to find the particular
post. I guess I'm just not very lazy.
keeping up wiht them and figuring out which ones I have
"answered" (affirmed that I understood or asked a followup or
clarifying question) and which ones I've missed.
You should fix that.
===================================================================
public class LocalizationUtils2 {
static final String CLASS_NAME = "LocalizationUtils";
Interesting that you label something 'CLASS_NAME' that does not match the name
of the class in which it appears. Careless.
private final static String MSG_PREFIX =
"ca.maximal.common.utilities.Resources.LocalizationUtilsMsg";
public static final String HORIZONTAL_SEPARATOR = "\t";
Why is this 'public'?
StringUtils stringUtils = null;
Why do you redundantly set the member to 'null'? Why is it package-private
("default" access)?
private ResourceBundle locMsg = null;
private MessageFormat msgFmt = new MessageFormat("");
private Locale locale = null;
private LocalizationUtils2() {
locale = Locale.getDefault();
It's so weird. You explicitly set 'locale' to 'null', its default value
anyway, when you have no use for and no intention of ever having a use for
that value.
locMsg = getResources(locale, MSG_PREFIX);
Ditto.
msgFmt.setLocale(locale);
}
public static LocalizationUtils2 getInstance() {
return new LocalizationUtils2();
Usually when a class is a "utility" class, it has only static members and is
never instantiated.
}
public String greeting() {
Object[] msgArgs = {};
msgFmt.applyPattern(locMsg.getString("msg012"));
return (msgFmt.format(msgArgs));
}
public SortedMap<String, Locale> getLocales() {
SortedMap<String, Locale> sortedLocales = new TreeMap<String,
Locale>();
for (Locale oneLocale : Locale.getAvailableLocales()) {
sortedLocales.put(oneLocale.getDisplayName(locale), oneLocale);
}
return sortedLocales;
}
public void writeLocales(PrintStream out) {
SortedMap<String, Locale> locales = getLocales();
int longestLocaleName = 0;
for (String oneLocale : locales.keySet()) {
if (oneLocale.length()> longestLocaleName) {
longestLocaleName = oneLocale.length();
}
}
stringUtils = StringUtils.getInstance();
This looks wrong. You have defined 'stringUtils' to be part of the object's
state, i.e., a member variable, but you only use it like a local variable.
You should declare it as a local variable, not a member variable.
for (String oneLocale : locales.keySet()) {
out.println(stringUtils.pad(oneLocale, ' ', 'T',
longestLocaleName) + HORIZONTAL_SEPARATOR +
locales.get(oneLocale));
On the other hand, you use 'StringUtils' like a utility class. The methods
should be 'static' and you should not instantiate it.
That means no 'stringUtils' variable at all.
}
out.flush();
}
public void writeLocales(PrintWriter out) {
SortedMap<String, Locale> locales = getLocales();
Is the list of 'Locale's likely to change often, because you build it every
time you refer to it. That implies you expect the value to change between calls.
int longestLocaleName = 0;
for (String oneLocale : locales.keySet()) {
if (oneLocale.length()> longestLocaleName) {
longestLocaleName = oneLocale.length();
}
}
stringUtils = StringUtils.getInstance();
for (String oneLocale : locales.keySet()) {
out.println(stringUtils.pad(oneLocale, ' ', 'T', longestLocaleName) +
HORIZONTAL_SEPARATOR + locales.get(oneLocale));
}
out.flush();
}
public ResourceBundle getResources(Locale currentLocale, String
listBase) {
final String METHOD_NAME = "getResources()";
ResourceBundle locList = null;
Redundant assignment of 'null'.
try {
locList = ResourceBundle.getBundle(listBase, currentLocale);
} catch (MissingResourceException mr_excp) {
Object[] msgArgs = {CLASS_NAME, METHOD_NAME, listBase,
currentLocale.toString(), mr_excp};
msgFmt.applyPattern(locMsg.getString("msg011"));
throw new IllegalArgumentException(msgFmt.format(msgArgs));
}
return (locList);
}
}
===================================================================
...
Getting expert reactions to this aspect of the class is my main issue
at this point.
I started to address those matters (that I elided here) for you, but realized
at this point that you have been overwhelmed. You're posting to quickly and
too frequently; you clearly have not had time to practice with and assimilate
the information you've already obtained. I, for one, am loathe to barrage you
with more wisdom until you've achieved a plateau with what people have already
told you.
Go and practice.
Consider NOT calling the class by a name that includes "Util" or "Utils" if
it's instantiable - conventionally such a name implies a class with no state
and no instance members.
Now go, practice, think, assimilate and master what you've already been given.
Would it help or confuse things if I replicated this post on the OTHER
threads that are discussing these issues, particularly the Design
Questions for Static Factory Methods one?
Gods! Don't do that!
--
Lew