[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-edu-devel
Subject: Re: Review Request: Rewrite the anagram generation from scratch
From: "Commit Hook" <null () kde ! org>
Date: 2012-01-20 15:55:15
Message-ID: 20120120155515.28793.53589 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103747/#review9971
-----------------------------------------------------------
This review has been submitted with commit 5de6d014e5d2b6718f770121671d46fd=
3c1508fa by Laszlo Papp to branch master.
- Commit Hook
On Jan. 20, 2012, 12:03 p.m., Laszlo Papp wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103747/
> -----------------------------------------------------------
> =
> (Updated Jan. 20, 2012, 12:03 p.m.)
> =
> =
> Review request for KDE Edu and Jeremy Paul Whiting.
> =
> =
> Description
> -------
> =
> 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 ope=
rating
> on the already truncated word.
> =
> 2) Storing the letters in a QStringList is suboptimal.
> =
> 3) Variable namings, like "insaneData" for the generated anagram, or "obj=
Chunk"
> for the random index are not that readable like in this patch
> =
> 4) Splitting the original word with "" rule is hard to read, understand a=
nd
> error-prone. I had to write a small workhorse where I realized this opera=
tion
> 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 o=
ne
> "count" variable.
> =
> 6. We do not need for loop anymore, just while. It means less statement a=
nd
> variable (no need for dealing with a loop variable, like 'i' in this spec=
ial
> 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; th=
at is
> what do/while is was designed for. Also, we spare some initialization with
> do/while.
> =
> =
> Diffs
> -----
> =
> src/engine/kanagramgame.cpp e7d3500 =
> =
> Diff: http://git.reviewboard.kde.org/r/103747/diff/diff
> =
> =
> Testing
> -------
> =
> Testing: I have just tested (build and runtime) the patch on my Archlinux
> laptop, and seems to work fine without any regressions.
> =
> =
> Thanks,
> =
> Laszlo Papp
> =
>
[Attachment #5 (text/html)]
<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>
_______________________________________________
kde-edu mailing list
kde-edu@mail.kde.org
https://mail.kde.org/mailman/listinfo/kde-edu
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic