Another JUnit scenario
in [Java Programming]
|
Prev: Design question - methods calling methods
Next: JavaSE/EE + Tomcat 6 + Windows service on 64 bit
From: Arne Vajhøj on 23 May 2010 19:08 On 23-05-2010 15:02, Patricia Shanahan wrote: > Rhino wrote: > ... >> I was looking at the source code for the Properties class, >> specifically the list() methods (I finally took a minute to find out >> where the source code in the Java API is again) and I didn't see any >> contructors for writers or streams in there. Is Properties not a good >> class to imitate for my situation then? > ... > > In general, you should not imitate classes, such as Properties, that > were designed very early in the life of Java. Many of them suffer from > at least one of two problems: > > 1. The initial Java API writers may have been very smart, but the > collective learning of the Java community over the years has often led > to better techniques. > > In particular, they tend to depend too much on inheritance, rather than > composition. Making Properties extend Hashtable creates problems. > > 2. They are limited in their use of features added to the language since > they were defined. > > Even if one were to expose the map aspects of Properties, it would > have been defined to implement Map<String,String>, rather than extend > Hashtable, if the Map interface and generics had existed when it was > defined. The Map optional methods that are undesirable for Properties, > put and putAll, would have thrown UnsupportedOperationException. The Properties class has many unusual features - the parsing of strings are also unusual. But it is a very practical class. It is easy to use. Any beginner can use it. Those that want something more sophistcated can use one of the XML API's or even the dreaded preferences API (or develop something themselves for pre-1.4). This type of easy is what makes many developers like the MS stuff. MS have a ton of practical but not super-OO stuff. Arne
From: Rhino on 23 May 2010 19:12 On May 23, 3:53 pm, Lew <no...(a)lewscanon.com> wrote: > Lew wrote: > >> A (usually - always the disclaimer) better pattern is to include the > >> Locale as an argument to the factory method. Static state is an > >> antipattern. > > >> public class LocalizationUtils > >> { > >> /** Don't forget the private constructor! */ > >> private LocalizationUtils(){} > > >> /** > >> * Foo factory with default Locale. > >> * @return Foo instance with default Locale. > >> */ > >> public static Foo getFooInstance() > >> { > >> return new SubtypeOfFoo(); > >> } > > >> /** > >> * Foo factory with custom Locale. > >> * @param locale custom Locale. > >> * @return Foo instance with custom Locale. > >> */ > >> public static Foo getFooInstance( Locale locale ) > >> { > >> return new SubtypeOfFoo( locale ); > >> } > >> } > > >> Remember, the only hard and fast rule of programming is that there are > >> no hard and fast rules of programming. > Rhino wrote: > > I'm REALLY getting confused now. Do you mean for Foo to be the > > constructor for the stream or writer that Daniel suggested? And why would > > No. I mean for some subtype of 'Foo', possibly 'Foo' itself, to be the actual > type of the constructed instance, regardless of whether you specify the > 'Stream' or 'Locale' or any other attribute. Sorry, that's just a jumble of words to me. I don't mean that you are being unclear but that I'm afraid I don't do very well when everything is put in very general terms like 'Foo', especially if I'm not clear on what kind of thing Foo is supposed to be. And the fact that we've got all these threads going with umpteen different considerations expressed, some of which I only half understand, is definitely complicating things.... And now my mail reader is refusing to connect! Arrrggghhhh!! Okay, switching to Google Groups..... > > > a non-default Locale call for a subtype of Foo, rather than Foo itself?? > > 'Locale' or 'Stream' or any other attribute for which the factory is > responsible, the principles are the same. > > What makes you think that the non-default 'Locale' setting has anything to do > with using a subtype? I showed use of a subtype in both cases. > > If you are only constructing 'Foo' instances and never different implementors > of 'Foo', you probably don't need a factory class and should just use 'Foo' > constructors. > > One of the main purposes of a factory class is to provide a hidden > implementation of the constructed type. The returned type in such cases is > nearly always an interface. > > 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. > Actually, I did answer it a few hours ago on one of the other threads. 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 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. Rather than tracking it down right now or risking paraphrasing my remarks inaccurately and causing further confusion, it would be helpful to _me_ to simply post LocalizationUils and/or LocalizationUtils2 right now. They are quite brief and they would give me some concrete reference points in asking questions. Basically, I want to ask if I have done things correctly now and identify any things that still need work. LocalizationUtils is satisfactory, I should then be able to make equivalent changes on the other utility classes. I've replaced displayLocales(), which wrote to the console, with writeLocales(PrintWriter) and writeLocales(PrintStream). By the way, I have the JUnit tests doing all (or most?) of what various people suggested. Those tests all give green checkmarks so I _think_ I've proven that my functionality all works. With regards to the writeXXX methods that write the Locales list, my test cases write them to a file, then read them back from the file and parse them sufficiently to verify that all 152 Locales are present and that en_CA, en_US, fr_FR and de_DE are all there. I'm just not sure if the overall design in right and I don't think I'm understanding the right place to create the Stream or Writer that I need. Okay, let's just dive in. Here is LocalizationUtils2, with all the imports, comments and javadocs stripped out for brevity: =================================================================== public class LocalizationUtils2 { static final String CLASS_NAME = "LocalizationUtils"; private final static String MSG_PREFIX = "ca.maximal.common.utilities.Resources.LocalizationUtilsMsg"; public static final String HORIZONTAL_SEPARATOR = "\t"; StringUtils stringUtils = null; private ResourceBundle locMsg = null; private MessageFormat msgFmt = new MessageFormat(""); private Locale locale = null; private LocalizationUtils2() { locale = Locale.getDefault(); locMsg = getResources(locale, MSG_PREFIX); msgFmt.setLocale(locale); } public static LocalizationUtils2 getInstance() { return new LocalizationUtils2(); } 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(); for (String oneLocale : locales.keySet()) { out.println(stringUtils.pad(oneLocale, ' ', 'T', longestLocaleName) + HORIZONTAL_SEPARATOR + locales.get(oneLocale)); } out.flush(); } public void writeLocales(PrintWriter out) { SortedMap<String, Locale> locales = getLocales(); 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; 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); } } =================================================================== I initially had two private constructors, the one you see here which takes no Locale parameter, and a second one which did. I initially had two getInstance() methods, one for each constructor, so one had no parameter and one took Locale as its parameter. I also had a getLocale() and setLocale(). This was the "everything INCLUDING the kitchen sink" version and allowed people to invoke the class with the default Locale or a specified one and then to change Locales anytime they wanted. Some of the discussion here persuaded me that this was too much so I stripped out the constructor and getInstance() that used the Locale parameter and removed getLocale() and setLocale(). (And yes, I have an open mind on this and could be persuaded to restore them; I've kept the previous version of the class around just in case I want to do this.) I know now that the user will get messages according to his user.language and user.country settings. (I created the frivolous greeting() method and made ResourceBundles just to demonstrate this. I made ResourceBundles for English, French and German containing a single greeting (Merry Christmas) in each of those languages with the key of msg012. I changed my user.language and user.country settings in the JVM via the run profile in Eclipse and verified that, depending on which values those settings have, the user will get the greeting in that language.) Therefore, I am supporting users that speak various languages and can easily add more. Users can have whichever of the supported languages they want simply by setting user.language and user.country. Users who want to switch on the fly from English to German and then to French can't do that; they are out of luck. But the ability to switch on the fly is probably overbuilding anyway. That's my (tentative) executive decision on that :-) Getting expert reactions to this aspect of the class is my main issue at this point. The lesser issue is the whole business about where the Writer or Stream used by writeLocales() should be done. At the moment, I create the Writer or Stream in my JUnit test case. It seems to me that anyone writing a class calling my utility class would expect to create the Writer or Stream there, not that I would do it for them. But I'm definitely willing to keep an open mind on that! I suppose my biggest reservation about creating the Writer or Stream in my class is the fear that it limits what the caller gets. For instance, if I create a Stream that writes to a physical file but the caller wants to redirect the output to the console, aren't I limiting his options unnecessarily? If I should create the Writer or Stream in LocalizationUtils2, what characteristics should I give it? Since it seems we've decided against using Properties as a model, can someone suggest a better model that does things the way the suggester (Daniel?) meant? Lastly, I would be happy to post the test cases for the class here to get people's comments if they'd like to see them. I don't expect that they'd be controversial but there might well be things that I could have done more efficiently. For instance, I test the writeLocales() methods by sending the output to a flat file, then read them back and parse them to be sure that the right number of Locales was written and that four specific Locales were present. That code is a bit bulkier than I've had to do in other tests so maybe there is a better way..... 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? -- Rhino
From: Lew on 23 May 2010 19:44 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
From: Lew on 23 May 2010 19:58 Lew wrote: >> A (usually - always the disclaimer) better pattern is to include the >> Locale as an argument to the factory method. Static state is an >> antipattern. >> >> public class LocalizationUtils >> { >> /** Don't forget the private constructor! */ >> private LocalizationUtils(){} >> >> /** >> * Foo factory with default Locale. >> * @return Foo instance with default Locale. >> */ >> public static Foo getFooInstance() >> { >> return new SubtypeOfFoo(); >> } >> >> /** >> * Foo factory with custom Locale. >> * @param locale custom Locale. >> * @return Foo instance with custom Locale. >> */ >> public static Foo getFooInstance( Locale locale ) >> { >> return new SubtypeOfFoo( locale ); >> } >> } >> >> Remember, the only hard and fast rule of programming is that there are >> no hard and fast rules of programming. Arne Vajhøj wrote: > I would prefer a non static field + getter + setter in the factory > over the arguments to the get instance methods. As long as you don't need the thing to be thread safe that's fine, too. Even better, usually, is a non-static final field set in the factory's constructor with only a getter. You instantiate a new instance of the factory each time you need a different 'Locale' (or whatever the attribute(s) is/are). > IF you need to get many objects different places in the code > then configuring the factory once may require less code and > may work better if the factory is retrieved via something > like Spring. I have a hard time grokking Spring. -- Lew
From: Arne Vajhøj on 23 May 2010 21:04
On 23-05-2010 19:58, Lew wrote: > Lew wrote: >>> A (usually - always the disclaimer) better pattern is to include the >>> Locale as an argument to the factory method. Static state is an >>> antipattern. >>> >>> public class LocalizationUtils >>> { >>> /** Don't forget the private constructor! */ >>> private LocalizationUtils(){} >>> >>> /** >>> * Foo factory with default Locale. >>> * @return Foo instance with default Locale. >>> */ >>> public static Foo getFooInstance() >>> { >>> return new SubtypeOfFoo(); >>> } >>> >>> /** >>> * Foo factory with custom Locale. >>> * @param locale custom Locale. >>> * @return Foo instance with custom Locale. >>> */ >>> public static Foo getFooInstance( Locale locale ) >>> { >>> return new SubtypeOfFoo( locale ); >>> } >>> } >>> >>> Remember, the only hard and fast rule of programming is that there are >>> no hard and fast rules of programming. > > Arne Vajhøj wrote: >> I would prefer a non static field + getter + setter in the factory >> over the arguments to the get instance methods. > > As long as you don't need the thing to be thread safe that's fine, too. > Even better, usually, is a non-static final field set in the factory's > constructor with only a getter. You instantiate a new instance of the > factory each time you need a different 'Locale' (or whatever the > attribute(s) is/are). I would leave the thread safeness to threads having different factory. Constructor or setter does not differ much. Just because in a few contexts no arg constructor and setter is better I have a tendency to use that everywhere. >> IF you need to get many objects different places in the code >> then configuring the factory once may require less code and >> may work better if the factory is retrieved via something >> like Spring. > > I have a hard time grokking Spring. It is both overhyped and highly misused. But I prefer it over homegrown pick class by config frameworks. Arne |