Review Request: [PATCH 1/4] kcalendarsystem: Fix lengthOfWeek to 7

[Jon Severinsson]

> On Dec. 13, 2012, 8:48 a.m., John Layt wrote:
> > Please do not replace named variables or method calls with "Magic Numbers", this is bad coding practise as it makes the code harder to read and understand and maintain.  If you must optimise the code, please use a "#define DAYS_IN_WEEK 7" instead.  Please do not deprecate daysInWeek() as it is an api method in QDate in Qt5 and we want to maintain a close api match.

Actually, daysInWeek() does not exist in QDate or QDateTime in either Qt4 or Qt5. daysInYear(year) and daysInMonth(year, month) exists, but not daysInWeek(). I assume because it is supposed to be obvious, and not very "magical" at all ;)

Changing this isn't about optimizing the code, but about simplifying it for maintainability. I especially don't want to require code to supposedly work for calendar systems with a different number of days in a week, when that is currently useless and untestable.

If we keep the function, and make it non-inline, it should come with a big fat warning that changing it to return anything other than 7 is likely to break a whole lot of code, both in KCalendarSystem and elsewhere.


- Jon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107687/#review23388
-----------------------------------------------------------


On Dec. 13, 2012, 7:41 a.m., Jon Severinsson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107687/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2012, 7:41 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> -------
> 
> Week length depends on the week system (i.e. KLocale::WeekNumberSystem) used,
> not the calendar used, and is 7 for all week systems supported by KDE.
> 
> 
> Diffs
> -----
> 
>   kdecore/date/kcalendarsystem.h efddd08 
>   kdecore/date/kcalendarsystem.cpp 1aed791 
>   kdecore/date/kcalendarsystemcoptic.cpp 0a95dbe 
>   kdecore/date/kcalendarsystemcopticprivate_p.h 35907ff 
>   kdecore/date/kcalendarsystemgregorian.cpp 40db17a 
>   kdecore/date/kcalendarsystemgregorianprivate_p.h fb7a0dd 
>   kdecore/date/kcalendarsystemhebrew.cpp e3d1484 
>   kdecore/date/kcalendarsystemindiannational.cpp 7222722 
>   kdecore/date/kcalendarsystemislamiccivil.cpp eea98e2 
>   kdecore/date/kcalendarsystemjalali.cpp 889a060 
>   kdecore/date/kcalendarsystemjulian.cpp 9bfc5f9 
>   kdecore/date/kcalendarsystemprivate_p.h d935ead 
>   kdecore/date/kcalendarsystemqdate.cpp f233d219 
>   kdecore/date/kdatetimeparser.cpp a416808 
>   kdecore/date/klocalizeddate.h 1bfe261 
>   kdecore/localization/klocale.h c21bb37 
>   kdecore/localization/klocale_kde.cpp 5409610 
> 
> Diff: http://git.reviewboard.kde.org/r/107687/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jon Severinsson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20121213/b377276c/attachment-0001.html>