<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/103747/">http://git.reviewboard.kde.org/r/103747/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This review has been submitted with commit 5de6d014e5d2b6718f770121671d46fd3c1508fa by Laszlo Papp to branch master.</pre>
<br />
<p>- Commit</p>
<br />
<p>On January 20th, 2012, 12:03 p.m., Laszlo Papp 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 Edu and Jeremy Paul Whiting.</div>
<div>By Laszlo Papp.</div>
<p style="color: grey;"><i>Updated Jan. 20, 2012, 12:03 p.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;">The current way of generating an anagram from a QString coming from the
dictionary is a bit hard to read, maintain and not completely working.
1) If the generated anagram is equal to the original, the re-calculation does
not work because the original object is not set back again; it starts operating
on the already truncated word.
2) Storing the letters in a QStringList is suboptimal.
3) Variable namings, like "insaneData" for the generated anagram, or "objChunk"
for the random index are not that readable like in this patch
4) Splitting the original word with "" rule is hard to read, understand and
error-prone. I had to write a small workhorse where I realized this operation
actually prepends and appends a "" at the beginning and end. This is more than
what we need on the other hand (ie. we just need the letters).
5) objData.count() as the condition of the loop is not that good and easy to
read than simple !letters.isEmpty(). Also with this patch, we can spare one
"count" variable.
6. We do not need for loop anymore, just while. It means less statement and
variable (no need for dealing with a loop variable, like 'i' in this special
case).
7) The whole method is now shorter and cleaner. Thereby, easier to read,
understand and maintain.
I was wondering whether to use do/while or recursive call, but I decided to keep
do/while since we need to try, at least once, to generate and anagram; that is
what do/while is was designed for. Also, we spare some initialization with
do/while.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Testing: I have just tested (build and runtime) the patch on my Archlinux
laptop, and seems to work fine without any regressions.</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>src/engine/kanagramgame.cpp <span style="color: grey">(e7d3500)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103747/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>