<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107687/">http://git.reviewboard.kde.org/r/107687/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 13th, 2012, 8:48 a.m., <b>John Layt</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Jon</p>
<br />
<p>On December 13th, 2012, 7:41 a.m., Jon Severinsson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Jon Severinsson.</div>
<p style="color: grey;"><i>Updated Dec. 13, 2012, 7:41 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdecore/date/kcalendarsystem.h <span style="color: grey">(efddd08)</span></li>
<li>kdecore/date/kcalendarsystem.cpp <span style="color: grey">(1aed791)</span></li>
<li>kdecore/date/kcalendarsystemcoptic.cpp <span style="color: grey">(0a95dbe)</span></li>
<li>kdecore/date/kcalendarsystemcopticprivate_p.h <span style="color: grey">(35907ff)</span></li>
<li>kdecore/date/kcalendarsystemgregorian.cpp <span style="color: grey">(40db17a)</span></li>
<li>kdecore/date/kcalendarsystemgregorianprivate_p.h <span style="color: grey">(fb7a0dd)</span></li>
<li>kdecore/date/kcalendarsystemhebrew.cpp <span style="color: grey">(e3d1484)</span></li>
<li>kdecore/date/kcalendarsystemindiannational.cpp <span style="color: grey">(7222722)</span></li>
<li>kdecore/date/kcalendarsystemislamiccivil.cpp <span style="color: grey">(eea98e2)</span></li>
<li>kdecore/date/kcalendarsystemjalali.cpp <span style="color: grey">(889a060)</span></li>
<li>kdecore/date/kcalendarsystemjulian.cpp <span style="color: grey">(9bfc5f9)</span></li>
<li>kdecore/date/kcalendarsystemprivate_p.h <span style="color: grey">(d935ead)</span></li>
<li>kdecore/date/kcalendarsystemqdate.cpp <span style="color: grey">(f233d219)</span></li>
<li>kdecore/date/kdatetimeparser.cpp <span style="color: grey">(a416808)</span></li>
<li>kdecore/date/klocalizeddate.h <span style="color: grey">(1bfe261)</span></li>
<li>kdecore/localization/klocale.h <span style="color: grey">(c21bb37)</span></li>
<li>kdecore/localization/klocale_kde.cpp <span style="color: grey">(5409610)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107687/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>