> 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
> 
>

_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Reply via email to