<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/101816/">http://git.reviewboard.kde.org/r/101816/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 1st, 2011, 3:43 p.m., <b>Arjen Hiemstra</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;">Category's function seems relatively logical. However, to me, NewGame sounds more like a method on some other class. I understand that it is async so you need to implement the signal handlers, but it seems to me that it is better suited for a more generic GameManager or something, that can also handle existing games and such, which would then include a newGame() method.</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;">Ok, though I think its much more cleaner to separate different actions into different classes (not generally, only in this case. Here, the classes are more like jobs (so one object per request), so if you combine two classes, you create a GameManager object even when what you required was just what NewGame provided, this wasting mem as some private vars aren't used in NewGame but only in EditGame. You will only set gameName and gameCategory, all other vars are useless).
Stil, I've attached an updated diff to show what it looks like.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 1st, 2011, 3:43 p.m., <b>Arjen Hiemstra</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/101816/diff/1/?file=25763#file25763line58" style="color: black; font-weight: bold; text-decoration: underline;">player/lib/category.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace GluonPlayer</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">58</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">private</span> <span class="n">Q_SLOTS</span><span class="o">:</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Personally, I prefer the following order in general:
Public
Public Slots
Signals
Protected
Protected Slots
Private
Private Slots
Makes it easier to simply read the header to find out what methods it contains and makes it easier to skip over the irrelevant ones. (i.e., private always and protected when you're not inheriting.)
Also, wouldn't these be better served by using Q_PRIVATE_SLOT ?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I have read concerns about Q_PRIVATE_SLOT not being documented and hence they should be avoided. (Refer http://lists.trolltech.com/qt-interest/2008-04/msg00348.html, first comment from Thiago). Is it ok to be used?</pre>
<br />
<p>- Shantanu Tushar</p>
<br />
<p>On July 1st, 2011, 2:40 p.m., Shantanu Tushar Jha 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 Gluon.</div>
<div>By Shantanu Tushar Jha.</div>
<p style="color: grey;"><i>Updated July 1, 2011, 2:40 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;">Category's job is to fetch list of categories from the server.
NewGame takes a game name and a category, and then creates a new game on the server, returning the ID of newly created game.</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;">Both classes function as expected when used.</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>player/lib/CMakeLists.txt <span style="color: grey">(53c0f0a)</span></li>
<li>player/lib/category.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/lib/category.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/lib/newgame.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/lib/newgame.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101816/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>