Battleship board game implementation in Turtle by sathwikmatsa · Pull Request #246 · sunjay/turtle

sathwikmatsa · 2021-07-24T14:47:34Z

This PR adds battleship board game implementation to turtle examples - #23

single player mode
multiplayer mode
arrow keys & enter based controls

enaut · 2021-07-24T18:22:14Z

Firstly - I don't have commit rights here!

I did not really read the whole code. But at a first glance, I think it would be nice to have some more documentation - as the examples are made to be read for learning. The audience is less experienced and is often very happy for explaining comments - so an overview in main.rs over the code might be helpful, as well as some function documentations for the less obvious things...

That said, the game itself looks great!

sathwikmatsa · 2021-07-27T12:27:26Z

Hey @enaut, Thanks for your feedback. I've added some documentation as per your suggestion. Let me know if you have any comments.

enaut

Sorry I have limited time. I skimmed most of it and read main.rs and ship.rs in more detail. I added comments and suggestions in the files.

Please do read my comments as suggestions for improvement. I highly value your PR and it works as it should I just added my opinion on some topics - feel free to have a different one and refuse it ;)

examples/battleship/main.rs

enaut · 2021-07-27T16:48:46Z

examples/battleship/ship.rs

+pub struct ShipPosition {
+    pub top_left: (u8, u8),
+    pub bottom_right: (u8, u8),
+}


Open for discussion: As you have the size as well as the orientation it might not be necessary to have 4 coordinates. It should be enough to have the top_left corner. If you have top_left and bottom_right that could be contradicting to what's specified in the orientation field. Or you could have like a 3x3 ship. When designing the structs it is good practise to disallow invalid states by typesystem... This could be intentional but is not part of the default battleship game. I think the cleanest solution would be to only keep top_left

enaut · 2021-07-27T17:30:36Z

examples/battleship/ship.rs

+#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub struct Ship {
+    pub kind: ShipKind,
+    pub position: ShipPosition,
+}


I see you only calculate the orientation when needed. I would have made that struct contain kind, position, orientation and calculate the area on the fly. But you may have good reasons to do it as you did - I'm just starting to go through the code.

That makes sense to me. I don't really need a separate struct ShipPosition. I've removed it and modified Ship struct to have kind, top_left and orientation fields.

enaut · 2021-07-27T17:40:05Z

examples/battleship/ship.rs

+    pub fn coordinates(&self) -> Vec<(u8, u8)> {
+        let orientation = self.orientation();
+        let x = self.position.top_left.0;
+        let y = self.position.top_left.1;
+
+        (0..self.kind.size())
+            .map(|i| match orientation {
+                Orientation::Horizontal => (x + i, y),
+                Orientation::Veritcal => (x, y + i),
+            })
+            .collect()
+    }


isn't that what you stored anyway in self.position? But I like that implementation - I'd name it bounding_box and let it return ShipBoundingBox

pub fn area(&self) -> ShipBoundingBox { ... } struct ShipBoundingBox { top_left: ShipCoordinate, bottom_right: ShipCoordinate } struct ShipCoordinate { x: u8, y: u8 }

Co-authored-by: Franz Dietrich <[email protected]>

enaut

Again some changes I would suggest. - still to review is the network and bot.

enaut · 2021-07-28T09:45:00Z

examples/battleship/battlestate.rs

+                    .map(|cell| match cell {
+                        Cell::Carrier => 'C',
+                        Cell::Battleship => 'B',
+                        Cell::Cruiser => 'R',
+                        Cell::Submarine => 'S',
+                        Cell::Destroyer => 'D',
+                        _ => '.',
+                    })


If you implement Display on Cell you can shorten this considerably:

Suggested change

.map(|cell| match cell {

Cell::Carrier => 'C',

Cell::Battleship => 'B',

Cell::Cruiser => 'R',

Cell::Submarine => 'S',

Cell::Destroyer => 'D',

_ => '.',

})

.map(Cell::to_string)

enaut · 2021-07-28T09:51:43Z

examples/battleship/battlestate.rs

+                    }
+                }
+            }
+            _ => unreachable!(),


having a default like that is nice and easy but you move a compile time error to a runtime error if you add a new Cell variant. So in general these should be avoided and rather list the remaining cases just as you did with the ship cases.

Suggested change

_ => unreachable!(),

Cell::Bombed | Cell::Unattacked | Cell::Missed | Cell::Destroyed => unreachable!(),

enaut · 2021-07-28T09:53:22Z

examples/battleship/battlestate.rs

+        match self.attack_grid.get(pos) {
+            Cell::Bombed | Cell::Destroyed | Cell::Missed => false,
+            Cell::Unattacked => true,
+            _ => unreachable!(),


enaut · 2021-07-28T10:03:07Z

examples/battleship/grid.rs

+pub enum Cell {
+    Carrier,
+    Battleship,
+    Cruiser,
+    Submarine,
+    Destroyer,
+    /// clear cell on ShipGrid
+    Empty,
+    /// clear cell on AttackGrid
+    Unattacked,
+    Missed,
+    Bombed,
+    /// Denotes a ship cell of a completely destroyed ship
+    Destroyed,
+}


I think it would make sense to nest ship kind here:

Suggested change

pub enum Cell {

Carrier,

Battleship,

Cruiser,

Submarine,

Destroyer,

/// clear cell on ShipGrid

Empty,

/// clear cell on AttackGrid

Unattacked,

Missed,

Bombed,

/// Denotes a ship cell of a completely destroyed ship

Destroyed,

}

pub enum Cell {

Ship(ShipKind),

/// clear cell on ShipGrid

Empty,

/// clear cell on AttackGrid

Unattacked,

Missed,

Bombed,

/// Denotes a ship cell of a completely destroyed ship

Destroyed,

}

That way you have one duplication less when adding a new ship. Of course this change needs some adjustments in the rest of the code - but it should be easy with rust-analyzer as guide :)

That's neat!

enaut · 2021-07-28T10:42:20Z

examples/battleship/grid.rs

+    pub fn to_cell(self) -> Cell {
+        match self {
+            ShipKind::Carrier => Cell::Carrier,
+            ShipKind::Battleship => Cell::Battleship,
+            ShipKind::Cruiser => Cell::Cruiser,
+            ShipKind::Submarine => Cell::Submarine,
+            ShipKind::Destroyer => Cell::Destroyer,
+        }
+    }


With the above change this could be simplified like so:

Suggested change

pub fn to_cell(self) -> Cell {

match self {

ShipKind::Carrier => Cell::Carrier,

ShipKind::Battleship => Cell::Battleship,

ShipKind::Cruiser => Cell::Cruiser,

ShipKind::Submarine => Cell::Submarine,

ShipKind::Destroyer => Cell::Destroyer,

}

}

pub fn to_cell(self) -> Cell {

Cell::Ship(self)

}

enaut · 2021-07-28T10:52:56Z

examples/battleship/game.rs

+                        RightArrow => crosshair.move_right(self),
+                        UpArrow => crosshair.move_up(self),
+                        DownArrow => crosshair.move_down(self),
+                        Return => {


Suggested change

Return => {

Return | Space => {

At least that is what I tried intuitively - note the import above has to be adjusted too. Also the documentation in main.

enaut · 2021-07-29T05:43:37Z

examples/battleship/config.rs

+            Cell::Ship(ShipKind::Carrier) => Self::CARRIER_COLOR.into(),
+            Cell::Ship(ShipKind::Battleship) => Self::BATTLESHIP_COLOR.into(),
+            Cell::Ship(ShipKind::Cruiser) => Self::CRUISER_COLOR.into(),
+            Cell::Ship(ShipKind::Submarine) => Self::SUBMARINE_COLOR.into(),
+            Cell::Ship(ShipKind::Destroyer) => Self::DESTROYER_COLOR.into(),


Not really necessary but sometimes it is more readable to nest matches in that case:

Cell::Ship(kind) => match kind { ShipKind::Carrier => Self::CARRIER_COLOR.into(), ShipKind::Battleship => Self::BATTLESHIP_COLOR.into(), ShipKind::Cruiser => Self::CRUISER_COLOR.into(), ShipKind::Submarine => Self::SUBMARINE_COLOR.into(), ShipKind::Destroyer => Self::DESTROYER_COLOR.into() }

But as I said in this case its perfectly readable so no need to change.

enaut

I don't have anymore issues with your code. Thank you a lot for your time and effort!

Now @sunjay has to add his opinion and review the changes as I do not have the ability to commit.

Of course there are still things like if one opponent closes his window in a multiplayer setup the other window just crashes. But I think those are out of scope for an example program.

sunjay · 2021-08-04T23:11:55Z

Thanks for taking the time to contribute this example! Appreciate the first pass at reviewing @enaut. I have been swamped with things at work lately but hopefully I will have time to review this in the coming week. If I don't get to it by next weekend, please ping me. Sorry for the delay!

sathwikmatsa added 14 commits July 15, 2021 12:56

Add destroy to Drawing to close window

9791ef7

undo rustfmt

20e18ec

Mark Drawing::destroy() as unstable

7eb6895

update runtest.rs example with Drawing::destroy()

7e3bbb9

Battleship logic

51ded43

Merge branch 'sunjay:master' into master

258591e

Add Config, Channel, Game structs

098b5b0

Add Game loop, user input event loop logic

92128b1

Listen on all interfaces

bd9e640

Add single player, improve UX

8974d8d

Refactor

4a36f02

reduce CPU usage

dcd8c53

Merge branch 'master' of https://github.com/sathwikmatsa/turtle

658d185

revert changes in square.rs

dcdf18b

Add documentation, refactor

3a53c82

enaut suggested changes Jul 27, 2021

View reviewed changes

sathwikmatsa and others added 2 commits July 28, 2021 12:32

refactor ship.rs

d3c5d3c

Apply suggestions from code review

5acbc11

Co-authored-by: Franz Dietrich <[email protected]>

enaut suggested changes Jul 28, 2021

View reviewed changes

Apply suggestions from code review

2451f94

enaut reviewed Jul 29, 2021

View reviewed changes

enaut approved these changes Jul 30, 2021

View reviewed changes

	_ => unreachable!(),
	Cell::Bombed \| Cell::Unattacked \| Cell::Missed \| Cell::Destroyed => unreachable!(),

	_ => unreachable!(),
	Cell::Carrier \| Cell::Battleship \| Cell::Cruiser \| Cell::Submarine \| Cell::Destroyer \| Cell::Empty => unreachable!(),

Conversation

sathwikmatsa commented Jul 24, 2021

Uh oh!

enaut commented Jul 24, 2021

Uh oh!

sathwikmatsa commented Jul 27, 2021

Uh oh!

enaut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enaut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enaut Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enaut left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunjay commented Aug 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

enaut Jul 29, 2021 •

edited

Loading

enaut left a comment •

edited

Loading