Welcome to the Treehouse Community
Want to collaborate on code errors? Have bugs you need feedback on? Looking for an extra set of eyes on your latest project? Get support with fellow developers, designers, and programmers of all backgrounds and skill levels here with the Treehouse Community! While you're at it, check out some resources Treehouse students have shared here.
Looking to learn something new?
Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and join thousands of Treehouse students and alumni in the community today.
Start your free trial
Evan Welch
1,815 PointsIn a battle ship game, how to compare a list of multiple fired on coordinates to the single coordinates of a ship?
I'm creating a battle ship game like the instructor recommended. I figure I should use a list to keep track of the fired-on coordinates so that I can add to it. So if I have a list of many fired-on coordinates, how do I compare them to the coordinates of the ships to determine when the ship has been hit? Any ideas? I'm done a bit of research into the Object.Equals method, but this is only for objects with matching types. Should I set the ship in a List which only includes one set of coordinates. I would appreciate a nudge in the right direction. Thanks!
7 Answers
Simon Coates
8,223 PointsI can't design classes to save my life, but I threw together something quick that might give you some ideas.
using System;
using BattleShip;
using System.Collections.Generic;
class Program
{
static void Main(string[] args)
{
Game g = new Game();
g.Play();
}
}
A Game class
using System;
using BattleShip;
using System.Collections.Generic;
namespace BattleShip
{
class Game
{
List<Point2d> guesses = new List<Point2d>();
List<Ship> ships = new List<Ship>();
public Game()
{
//do any set up work
ships.Add(new Ship( new Point2d( 1,2)));
}
public void Play()
{
Point2d guess =new Point2d(2,2); //or call a method to return a random point
foreach(Ship aShip in ships)
{
bool isHit= aShip.TakeHit(guess);
//for now
if(isHit)
{
System.Console.WriteLine("I guess i hit and ought to do something");
} else {
System.Console.WriteLine("I guess i didn't hit and ought to do nothing");
}
}
guesses.Add(guess);
}
}
}
A ship class:
namespace BattleShip
{
class Ship{
private Point2d location;
public Ship(Point2d location)
{
this.location = location;
}
public bool TakeHit(Point2d guess)
{
return location.Equals(guess);
}
}
}
a Point2d class (you might be able to find a prebuilt class.)
using System;
namespace BattleShip
{
class Point2d //should probably be a struct.
{
public int x;
public int y;
public Point2d(int x, int y)
{
this.x= x;
this.y = y;
}
public override bool Equals(Object obj)
{
if (obj == null || ! (obj is Point2d))
return false;
else {
Point2d casted = (Point2d) obj;
return this.x == casted.x &&
this.y == casted.y;
}
}
//I think this is necessary to override equals, but I don't recall how you're supposed to implement.
public override int GetHashCode()
{
return x+y;
}
}
}
i could have had the Ship return a point or list of coordiantes for the game loop to evaluate, but I figured I wanted ship to handle its own data. I apologize if I've defaulted into any java naming conventions. If there's any variable or field naming that looks weird it's probably just me writing bad code.
nb: Sorry, I should have explained it a little. you made reference the coordinates not all being in the same type and issues around testing equality, so I made the coords the same type and added an equality override (it's a little clumsy compared with some more advanced example I saw that used interfaces, generics and other more advanced concepts.)
Simon Coates
8,223 PointsIn reference to your final code, it wasn't my intent to over complicate things. I wanted to provide as full feedback as possible as you seemed to have an admirable attitude and aptitude. The random stuff was a detail. So I'll simplify. 1) If something doesn't need state, then maybe it can be static. 2) Composing the program into objects allows you to simplify. If the local variables were stored as fields (or properties) on an object, then you can split your code code into smallish methods that are easier to read and debug. For example, so if you had something like
if(Player1Turn)
{
takePlayerTurn();
} else {
takeComputerTurn();
}
it's easy to see (and verify) the high level picture and focus on the details necessary for a single action in the methods. (Sorry for responding weeks later. I had some fixed deadlines with a couple courses I'm taking elsewhere. I didn't like my earlier responses and it's something that nags at me.)
Evan Welch
1,815 PointsYeah I'm finding that organization is one of the hardest parts. A lot of small judgement calls are necessary all the time. On top of that is the actual logic and knowing the methods already available from .Net or Unity
Evan Welch
1,815 PointsOk! Thank you for the super high quality answer! Super helpful because I've already been working on this problem so I can see how you think to go about it.
I guess it is necessary to override the Equals method. I saw this in the Microsoft documentation, but I didn't understand it. Why does the point2d object need to be casted? And I'm sorry if some of these questions are obvious. I'm going to look into them so that I can understand and implement them into my own code, but I'm just writing this response in real time as I read your code.
So, the guess is point2d and the ship's location is point2d, that way they are the same type for the Equals method.
Looking at your code, I'm guessing it's convention to declare all fields at the top of the class, unless they're being used for a specific method, like isHit. Also, people prefer to have very little written in the Main method?
Will the foreach loop in the Play() Method, continue to loop until all of one player's ships are destroyed or does it loop once for every ship in the Ship list? Obviously, what you've written is very helpful, I just want to make sure I'm reading it right.
Simon Coates
8,223 Points1) I looked at example of .Equals and they defined themselves using a Object parameter. I wasn't sure if it was a matter of convention or necessity to use Object in the param. But if you use Object, then you have to tell it that it's a Point2d in order to be able to access fields and methods that don't exist on the base type. If C# knows it's a Point2d, then I can access the .x and .y. 2) isHit isn't a field. it's a local variable. Fields persist for the duration of the class and can be accessed across methods. A local variable is, if memory serves, scoped to the nearest {}. 3) I didn't write the main method to be short. I decided it made sense to have a Game object and it produced a nice short readable method. 4) I don't actually know how to play BattleShip offhand. I assumed each guess would need to be tested against all ships. But I assumed you'd need to code an outer loop.
Evan Welch
1,815 PointsThanks for the continued help Simon Coates. In case you want to see what I wrote https://github.com/evanwrdev/TreehouseC-CodingPracticeBattleShip . I'm sure that I didn't write clean code, and if you want to give me any pointers, I'd be all ears!
Simon Coates
8,223 PointsUnless I'm doing something wrong (very possible as my version control experience is mostly with software that preceded git), I'm only seeing git ignore and attribute files.
Evan Welch
1,815 PointsOk, I know this is a long wait time, and you might not remember this project, but I came back to it and decided I wanted to do it right and have this project of mine on GitHub. Still there is an issue that the game is constantly warning you of shooting where you have already shot because of the list I am using. But nonetheless, it should be on git and playable here https://github.com/evanwrdev/treehouseBattleShip
Simon Coates
8,223 PointsI'll take a look. I'll need some time. Offhand, you should probably alter Computer.cs. Microsoft says "Both to improve performance and to avoid inadvertently creating separate random number generators that generate identical numeric sequences, we recommend that you create one Random object to generate many random numbers over time, instead of creating new Random objects to generate one random number.". There's at least one copy and paste bug and a couple missing conditionals. Everything else is more complex.
Simon Coates
8,223 PointsRegarding
using System;
using System.Collections.Generic;
using System.Text;
namespace treehouseBattleShip
{
class Computer
{
public int ComputerXAxis;
public int ComputerYAxis;
public int ComputerXAxisToFire;
public int ComputerYAxisToFire;
public void ComputerCoordinates()
{
Random random = new Random();
ComputerXAxis = random.Next(6); //x coordinate value between 0 and 5
ComputerYAxis = random.Next(6);
}
public void GetComputerToFire()
{
Random random = new Random();
ComputerXAxisToFire = random.Next(6); //x coordinate value between 0 and 5
ComputerYAxisToFire = random.Next(6);
}
}
}
You currently access this with:
Computer computer = new Computer();
computer.ComputerCoordinates();
The problem is that the 2 methods are basically the same and that the values stores are only useful when accessed (ie. don't need to be stored). As such, you could replace it with something like
using System;
using System.Collections.Generic;
using System.Text;
namespace treehouseBattleShip
{
class Computer
{
private /* readonly */ Random random;
private int range;
public Computer(Random random, int range)
{
this.random = random;
this.range = range;
}
public Point2d getRandomCoordinate()
{
return new Point2d (random.Next(range), random.Next(range));
}
}
}
which can be accessed (or tested) with
Computer c = new Computer(new Random(), 6);
Point2d randomPoint = c.getRandomCoordinate();
Console.WriteLine($"{randomPoint.X}, {randomPoint.Y}");
Passing in the variables rather than creating them feels off, but it's really helpful for automated regression testing and provides benefits down the track (when you get to dependency injection, inversion of control).
Simon Coates
8,223 PointsWith CoordinateInput, the methods are mostly the same (see DRY) and the recursive method calls don't give you anything you couldn't get with a loop. Additionally, if there's no object state, then these methods should probably be static. Objects are bundles of data and the methods for acting on that data. If there's no data, I'm not sure you gain anything by using instance over static (at least not yet). So for now, I might opt for something like (see below). It's a little shorter. Access to the methods is slightly cleaner. And if you adjust the method to get the integer (to use try/catch or int16.tryparse), then you only have to modify the one method.
using System;
using System.Collections.Generic;
using System.Text;
namespace treehouseBattleShip
{
public class CoordinateInput
{
public static int GetCoordinate(int dimensions, string player, string axis, string otherPlayer)
{
return GetCoordinate(dimensions,
$"{player}, on a {dimensions} by {dimensions} grid, where would you like to place your battleship on the {axis}? {otherPlayer} DONT LOOK!");
}
public static int GetCoordinateVsComputer(int dimensions, string player, string axis)
{
return GetCoordinate(dimensions,
$"{player}, on a {dimensions} by {dimensions} grid, where would you like to place your battleship on the {axis}?");
}
public static int GetCoordinateToFire(int dimensions, string player, string axis)
{
return GetCoordinate(dimensions,
$"{player}, on a {dimensions} by {dimensions} grid, where would you like to fire on the {axis}?");
}
public static int GetCoordinate(int dimensions, String prompt)
{
while(true)
{
Console.WriteLine(prompt);
string userInputStr = Console.ReadLine();
int userInput = Convert.ToInt32(userInputStr);
if (CoordinateOnMap(userInput, dimensions))
{
return userInput;
}
Console.WriteLine("I'm sorry! Your choice was not on the board!");
}
}
public static bool CoordinateOnMap(int Coordinate, int dimensions)
{
return !(Coordinate > dimensions || Coordinate < 0);
}
}
}
Being static, access is slightly simpler and doesn't create any ambiguity as to whether there's any kind of underlying instance state that would require a single reference:
CoordinateInput.GetCoordinateToFire(6, "bob", "y")
(my opinions on code may be subjective. If you prefer your design and you're confident you understand the concepts, then feel free to ignore. I'm a bit of a tourist in C# anyway. )
Simon Coates
8,223 PointsRegarding Player, Unless you have a compelling reason (extra code, different data) to have separate classes for Player1 and Player2, they should likely be instances of the same class. For instance, with something like:
using System;
using System.Collections.Generic;
using System.Text;
namespace treehouseBattleShip
{
class Player
{
public Point2d ShipLocation {get; set;}
public String Name {get; set;}
public Player(String name, Point2d location)
{
this.Name = name;
this.ShipLocation = location;
}
}
}
I might create and compare players using code similar to
using System;
using System.Collections.Generic;
namespace treehouseBattleShip
{
class Program
{
static void Main(string[] args)
{
Player p = new Player("Bob", new Point2d(1,2));
Player p2 = new Player("Dave", new Point2d(3,2));
p2.ShipLocation = new Point2d(1,2);
Point2d shipAt = p2.ShipLocation;
Console.WriteLine($"Players are equal? {p.ShipLocation.Equals(p2.ShipLocation)}");
Console.WriteLine($"{p2.Name}'s x is {p2.ShipLocation.X}");
}
}
}
Evan Welch
1,815 PointsThank you! I see what you mean that having a second Random is not necessary for the computer class.
Evan Welch
1,815 PointsSimon Coates I appreciate the compliment! I've been struggling with coding in Unity, it's encouraging to get positive feedback!