[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-games-devel
Subject: Re: [Kde-games-devel] Review Request 114845: [knavalbattle] utilties for the several ships patch
From: "Roney Gomes" <roney477 () gmail ! com>
Date: 2014-01-16 12:20:36
Message-ID: 20140116122036.2618.57633 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Jan. 15, 2014, 11:52 a.m., Roney Gomes wrote:
> > src/battlefield.cpp, line 213
> > <https://git.reviewboard.kde.org/r/114845/diff/2/?file=229803#file229803line213>
> >
> > I see here that you're testing for both horizontal and vertical availability, \
> > which -- once again -- could also be split into smaller methods. One for each \
> > case.
> > You also decided to check horizontal availability first. Any reason for this or \
> > the order doesn't matter at all?
> > How the AI would know in which direction it should place the new ship?
>
> Jaime Torres Amate wrote:
> Q: Any reason for this or the order doesn't matter at all?
> A: the order doesn't matter at all. Both horizontal and vertical must be checked. A \
> ship can not be placed if there is not enough horizontal nor vertical space.
> The method, as it is now, can be divided in two smaller methods (one for vertical \
> and one for horizontal) reading two times the matrix. Or it can be done in one pass \
> with an additional column(or row) array. The second way should be faster.
> Roney Gomes wrote:
> Although it is faster, the matrix isn't so big to cause any sensible variation in \
> performance. The code is OK, I'm not asking you to change it. I'm just pointing out \
> that I prefer code that's easy to read, and thus maintain, than optimized code \
> whose performance improvements are not significant.
> Jaime Torres Amate wrote:
> I'll change it. You are right. Even with a 50x50 board, the board fits in a 1Kib \
> cpu cache. Also I missed the delete[] (using different programming languages so \
> often) But I need some git help. There will be (in any case) two commits in the \
> multipleShips branch for this patch. I cherry-pick those 2 commits into master (I \
> know how to do it)? Merge the two commits into master (more or less, reading Albert \
> instructions)? Create one patch and commit it directly into master (I know how to \
> do it)?
>
The simpler approach is to commit to the new branch then merge with master \
(considering the current state of things, it'll be a fast-forward merge). However, I \
ask you to merge only when the multiple ships feature is ready.
Work on your branch as usual, commit and push as you progress but merge only when \
everything is done. I may take care of merging case you find any problems.
- Roney
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114845/#review47433
-----------------------------------------------------------
On Jan. 15, 2014, 6:49 p.m., Jaime Torres Amate wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114845/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2014, 6:49 p.m.)
>
>
> Review request for KDE Games and Roney Gomes.
>
>
> Repository: knavalbattle
>
>
> Description
> -------
>
> These are the tools I use to check if a ship can be placed in the board or if the \
> user/AI should start again. Also, it serves me to split a big patch into smaller \
> pieces. How it is done:
> * Pass the parameter allow adjacent ships also to the BattleField.
> * Implement a bool canAddShipOfSize(size). When a ship of size size can not be \
> placed, then the AI or the player should start again (what I'm working on) or an \
> Undo.
> * Implement a clear() to be able to start again
> * In the battleField also add another hidden board with the cells having only two \
> status (free, busy) to be able to know the availability of the cells. It is easier \
> for me in this way than the other way (sending a signal to clear the ship borders \
> when the shooting starts).
>
> Diffs
> -----
>
> src/battlefield.h b2f30bc
> src/battlefield.cpp 6467d8c
> src/sea.h 4af50b7
> src/sea.cpp 38ab8b5
>
> Diff: https://git.reviewboard.kde.org/r/114845/diff/
>
>
> Testing
> -------
>
> It detects the blocked boards (played much more games than I wanted to implement \
> the restart placing ships).
>
> Thanks,
>
> Jaime Torres Amate
>
>
[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="https://git.reviewboard.kde.org/r/114845/">https://git.reviewboard.kde.org/r/114845/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On January 15th, 2014, 11:52 a.m. UTC, <b>Roney \
Gomes</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="https://git.reviewboard.kde.org/r/114845/diff/2/?file=229803#file229803line213" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/battlefield.cpp</a> <span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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">206</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">I see here that \
you're testing for both horizontal and vertical availability, which -- once again \
-- could also be split into smaller methods. One for each case.
You also decided to check horizontal availability first. Any reason for this or the \
order doesn't matter at all?
How the AI would know in which direction it should place the new ship?</pre>
</blockquote>
<p>On January 15th, 2014, 6:42 p.m. UTC, <b>Jaime Torres Amate</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;">Q: Any reason for this \
or the order doesn't matter at all?
A: the order doesn't matter at all. Both horizontal and vertical must be checked. \
A ship can not be placed if there is not enough horizontal nor vertical space.
The method, as it is now, can be divided in two smaller methods (one for vertical and \
one for horizontal) reading two times the matrix. Or it can be done in one pass with \
an additional column(or row) array. The second way should be faster.</pre> \
</blockquote>
<p>On January 16th, 2014, 12:22 a.m. UTC, <b>Roney Gomes</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;">Although it is faster, \
the matrix isn't so big to cause any sensible variation in performance. The code \
is OK, I'm not asking you to change it. I'm just pointing out that I prefer \
code that's easy to read, and thus maintain, than optimized code whose \
performance improvements are not significant.</pre> </blockquote>
<p>On January 16th, 2014, 9:39 a.m. UTC, <b>Jaime Torres Amate</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;">I'll change it. You \
are right. Even with a 50x50 board, the board fits in a 1Kib cpu cache. Also I missed \
the delete[] (using different programming languages so often) But I need some git \
help. There will be (in any case) two commits in the multipleShips branch for this \
patch. I cherry-pick those 2 commits into master (I know how to do it)? Merge the two \
commits into master (more or less, reading Albert instructions)? Create one patch and \
commit it directly into master (I know how to do it)?
</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;">The simpler \
approach is to commit to the new branch then merge with master (considering the \
current state of things, it'll be a fast-forward merge). However, I ask you to \
merge only when the multiple ships feature is ready.
Work on your branch as usual, commit and push as you progress but merge only when \
everything is done. I may take care of merging case you find any problems.</pre> <br \
/>
<p>- Roney</p>
<br />
<p>On January 15th, 2014, 6:49 p.m. UTC, Jaime Torres Amate wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KDE Games and Roney Gomes.</div>
<div>By Jaime Torres Amate.</div>
<p style="color: grey;"><i>Updated Jan. 15, 2014, 6:49 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
knavalbattle
</div>
<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;">These are the tools I use to check if a ship can be placed in the board \
or if the user/AI should start again. Also, it serves me to split a big patch into \
smaller pieces. How it is done:
* Pass the parameter allow adjacent ships also to the BattleField.
* Implement a bool canAddShipOfSize(size). When a ship of size size can not be \
placed, then the AI or the player should start again (what I'm working on) or an \
Undo.
* Implement a clear() to be able to start again
* In the battleField also add another hidden board with the cells having only two \
status (free, busy) to be able to know the availability of the cells. It is easier \
for me in this way than the other way (sending a signal to clear the ship borders \
when the shooting starts).
</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;">It detects the blocked boards (played much more games than I wanted to \
implement the restart placing ships).</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/battlefield.h <span style="color: grey">(b2f30bc)</span></li>
<li>src/battlefield.cpp <span style="color: grey">(6467d8c)</span></li>
<li>src/sea.h <span style="color: grey">(4af50b7)</span></li>
<li>src/sea.cpp <span style="color: grey">(38ab8b5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/114845/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
kde-games-devel mailing list
kde-games-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-games-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic