I assume you have run the tests including checkstyle?
http://gwt-code-reviews.appspot.com/338801/diff/1/9 File user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java (right): http://gwt-code-reviews.appspot.com/338801/diff/1/9#newcode25 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:25: private static AnyRtlDirectionEstimator estimator = On 2010/04/11 19:16:50, Tomer wrote:
On 2010/04/11 16:13:46, jat wrote: > Would it be worth moving estimator to DirectionEstimatorTestBase
(set in a
super > constructor call) and make each individual test just a set of
expected results
> for various strings?
I don't realize what it would save - we still should have all types of estimators, one for each subclass. On the other hand, it'd impair
readability. I was just thinking there is a lot of duplication among the different tests, and it could be slightly reduced by doing something like: assertEstimatedDirection(Direction.RTL, containsRtlChar); assertEstimatedDirectionHtml(Direction.LTR, noRtlCharsHtml); in the test methods, with assertEstimatedDirection* defined on the base class and estimator initialized there from a superclass constructor. To me, that seems more readable if the duplicated code is reduced. However, this isn't a big deal so if you feel strongly about it feel free to leave as-is. http://gwt-code-reviews.appspot.com/338801/diff/9001/10005 File user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java (right): http://gwt-code-reviews.appspot.com/338801/diff/9001/10005#newcode36 user/src/com/google/gwt/i18n/shared/HasDirectionEstimator.java:36: * DirectionEstimator object to {...@link DefaultDirectionEstimator}. In light of your comments about how different estimators will be used, is it appropriate to explicitly mention DefaultDirectionEstimator here? It seems like some widgets might have different defaults. Perhaps it would be better to say "...to a default DirectionEstimator instance". http://gwt-code-reviews.appspot.com/338801/diff/9001/10008 File user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java (right): http://gwt-code-reviews.appspot.com/338801/diff/9001/10008#newcode25 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:25: private static final AnyRtlDirectionEstimator ESTIMATOR = Sorry, I should have been more clear. Constants should be all-caps and underscore separated, but that doesn't mean that everything immutable is considered a constant. INSTANCE for singleton instances is somewhat of a special case, and it can be fuzzy on where to draw the line. Personally, I use all-caps for Enums, Strings, and primitives. So, I think estimator is fine here. http://gwt-code-reviews.appspot.com/338801/diff/9001/10008#newcode34 user/test/com/google/gwt/i18n/shared/AnyRtlDirectionEstimatorTest.java:34: assertEquals(ESTIMATOR.estimateDirection(containsRtlChar), Direction.RTL); Sorry I didn't notice this before -- you have expeced/actual backwards on the asserts. http://gwt-code-reviews.appspot.com/338801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors To unsubscribe, reply using "remove me" as the subject.
