[Kde-games-devel] Review Request 114845: [knavalbattle] utilties for the several ships patch

[Roney Gomes]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114845/#review47433
-----------------------------------------------------------



src/battlefield.cpp
<https://git.reviewboard.kde.org/r/114845/#comment33735>

    Would you mind to put this code in a private method? Preferably with some meaningful name like setUpSecondaryBoard() or something like this.



src/battlefield.cpp
<https://git.reviewboard.kde.org/r/114845/#comment33736>

    The two structures above should be also split into smaller methods.
    
    One method to take care of adding a ship to the secondary board and a second one responsible for adding the borders.



src/battlefield.cpp
<https://git.reviewboard.kde.org/r/114845/#comment33737>

    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?


- Roney Gomes


On Jan. 4, 2014, 9:57 a.m., Jaime Torres Amate wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114845/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2014, 9:57 a.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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20140115/87f2fcf1/attachment.html>