On 2010/07/22 23:42:08, scottb wrote:
I just now took a look, and was curious about the rationale for some
of the API
changes. In particular, I'm trying to figure out the value of having
to do the
extra '.get()' at all call sites, vs. having static methods that do
the '.get()'
internally. I see some theoretical value in being able to get a
specific
instance of a SpeedTracerLogger, but 99% of the time, it doesn't seem
useful to
care.
I agree that going back to static methods could save a little bit of typing, but nevertheless, it is a common pattern for a logging class (j.u.logging, log4j). Also, if it is keystrokes that bother you, you could save a lot of typing in the invocation with the following pattern: SpeedTracerLogging stl = SpeedTracerLogger.get(). stl.start(...); ... stl.end(...); This is the pattern we actually used inside of Chrome, but that had mainly to do wit the fact that the logger can get turned on and off and .get() can return null.
As a practical matter, where do you the instanceness as useful outside
of maybe
SpeedTracerLoggerTest itself?
You are right, the sole rationale was to support testing. I don't have a second use case for it at the moment.
For example, a public newInstance() seems particularly silly, since if
you call
it twice and try to use each one independently, they'll both be trying
to write
to the same file.
I agree that doesn't make sense. Here are some possible ways I can think of to address your concerns: - Remove newInstance() or make it package protected - Refactor all of the SpeedTracerLogger.get() calls to use the pattern I mentioned above. - Go back to all static invocations and then have the test grab instances with new() or newInstance and use package protected methods with names that don't collide with start() and stop() like startImpl() stopImpl(). This doesn't feel right to me - it seems like the test is sneaking around under the covers of the API. It also guarantees a use of multple speedtracer loggers will require a major refactor. http://gwt-code-reviews.appspot.com/707802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
