On Mon, Sep 29, 2008 at 2:44 PM, John LaBanca <[EMAIL PROTECTED]> wrote:
> Overall, this widget looks very cool. <ake sure you apply the GWT check
> styles as they show a few problems, such as missing periods, unnecessary
> casts, unused @param tags, and unresolved types used in JavaDoc. There are
> a lot of methods with partial JavaDoc as well.
>
Yep. Sorry, I should have been clearer -- the code is in its final state
other then the changes we will make due to the review. The java doc will go
through a polishing stage after the code is settled.
>
> CalendarModel:
> Most of the static methods in CalendarModel seem like implementation
> details that should not be exposed.
>
I think some of those are really critical for people creating alternative
month selectors and calendar views, but we can move them into a
CalendarUtil, as that will make the distinction clearer.
>
> - computeYearBeforeMonth() should be renamed, removed, or made package
> protected since it is really an implementation detail. I vote for removing
> it.
>
It's useful for people who need to separate the month and year in order to
create drop downs, but as we're not using it yet, so we can probably remove
it for now and re-introduce it after we have a month selector with a year
shortcut.
>
> - copy() could be renamed to copyDate()
>
yep, I like that name change.
>
> - diffDays could be renamed to calculateInterval or something else. Should
> specify if start and/or end is inclusive.
>
Not crazy about any of those names, will try to think of something better...
>
> - createKeyFromDate seems like an implementation detail. Also, it says it
> will create a unique string key for the date, but only if the dates are
> unique by day. If two dates have the same days (even if they have different
> times), the key will be the same.
>
Yep, that is actually the point, as we need someway at an abstract level to
associate information with a specific date (as opposed to a date/time).
However, as currently only DatePicker requires it, it can safely be moved
there.
>
> DatePickerComponent
> - Needs JavaDoc
>
> DatePicker
> 324: I don't think you should fire the showRangeEvent retroactively, its
> not consistent with other handlers and doesn't really make sense logically
> that the event is fired when you add a handler.
>
To borrow from our "and sink" on raw handlers, adding a
addShowRangeHandlerAndFire method to support the use case..
>
> 426: why not just use a while loop instead of a for loop?
>
for the sake of avoiding "while" while programming of course ;-). Changing
to a "while" loop.
> 446: when you set the allowable date range, you should probably check that
> the current date is within that range
>
Waiting on a pending patch on the setAllowableDate range machinery. Will go
ahead and write my own if none is forthcoming soon.
Thanks for the review!
Emily
> Thanks,
> John LaBanca
> [EMAIL PROTECTED]
>
>
>
> On Mon, Sep 29, 2008 at 1:27 PM, Emily Crutcher <[EMAIL PROTECTED]> wrote:
>
>> Could you review the gen 2 date picker code? The code is in
>> com.google.gwt.gen2.datepicker. This code is going to be moved over to gwt
>> after the handler code is moved, so nitpicks would be very welcome.
>>
>> thanks!
>>
>> Emily
>>
>> --
>> "There are only 10 types of people in the world: Those who understand
>> binary, and those who don't"
>>
>
>
--
"There are only 10 types of people in the world: Those who understand
binary, and those who don't"
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
