> 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
