Review Request: Rewrite the anagram generation from scratch

Laszlo Papp lpapp at kde.org
Fri Jan 20 12:03:20 UTC 2012


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103747/
-----------------------------------------------------------

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


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20120120/f7de81bf/attachment.html>


More information about the kde-edu mailing list