Obrigado para compartilhar seu código.

O que me incomoda um pouco com esse código é que eu não uso interfaces, classes abstratas ou herança

Fazer OOP significa que você segue certos princípios que são (entre outros):

• ocultação / encapsulamento de informações
• responsabilidade única
• separação de preocupações
• BEIJO (mantenha-o simples (e) estúpido.)
• SECO (não se repita.)
• “Diga! Não pergunte.”
• Lei de demeter (“Não fale com estranhos!”)

Interfaces, abdominais classes de tratado, ou princípios de chapéu de suporte de herança e devem ser usados conforme necessário. Eles não “definem” OOP.

IMHO, a principal razão pela qual sua abordagem falha OOP é que seu “Modelo” é um array de um tipo primitivo char. Em última análise, isso leva a uma abordagem procedimental para a lógica do jogo.

Eu pensaria em uma interface como esta:

interface GameField{ char getIcon(); Result shootAt(); } 

onde Result seria um enum:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

E eu teria diferentes implementações da interface:

public class BorderField implements GameField{ private final char borderName; public BorderField(char borderName){ this.borderName = borderName; } @Override public char getIcon(){ return borderName; } @Override public Result shootAt(){ return Result.NO_HIT; } } 

public class WaterField implements GameField{ private boolean isThisFieldHit = false; @Override public char getIcon(){ return isThisFieldHit?"M": " "; } @Override public Result shootAt(){ return Result.NO_HIT; } } 

public class ShipField implements GameField{ private final Ship ship; private boolean isThisFieldHit = false; public ShipField(Ship ship){ this.ship = ship; } @Override public char getIcon(){ Result shipState = ship.getState(); switch(shipState){ case NO_HIT: return " "; case PARTIAL_HIT: return isThisFieldHit?"O":" "; case DESTROYED: return "#"; } @Override public Result shootAt(){ ship.hit(); return ship.getState(); } } 

Isso deve ser suficiente, espero que você pegue a ideia …

Questões formais

Nomenclatura

Encontrar bons nomes é a parte mais difícil na programação. Portanto, sempre pense nos nomes dos seus identificadores.

Pelo lado positivo, você segue as convenções de nomenclatura Java.

Mas você deve fazer com que os nomes dos métodos comecem com um verbo presente.Eg: shipWasHit() deve ser nomeado hit().
Ou distanceBetweenPoints() deve seja calculateDistanceBetween(). Aqui, os parâmetros revelam que a distância é entre os pontos, portanto, não há necessidade de colocar isso no nome do método.

Seja prolixo em seus nomes de variáveis. em vez de

 double x1 = from.getX(); double y1 = from.getY(); double x2 = to.getX(); double y2 = to.getY(); 

essas variáveis deveriam ser nomeado assim:

 double startPointX = from.getX(); double startPointY = from.getY(); double endPointX = to.getX(); double endPointY = to.getY(); 

Tire seus nomes do domínio do problema, não da solução técnica. Ex .: SHIP_ICON deve ser SHIP apenas a menos que você tenha outra constante na classe Ship .

Comentários

Os comentários devem explicar por que o código é como é . Remova todos os outros comentários.

os comentários devem ser usados apenas em métodos de interface ou abstratos onde contenham o contrato que o implementador deve cumprir.

Classe de constantes

Junte as coisas que devem estar juntas. Defina constantes na classe que as usa.

Comentários

• Todos os pontos válidos e realmente úteis. Obrigado!

Já existem algumas respostas boas, mas pensei em adicionar algumas das coisas que permaneceram para mim quando verifiquei o código.

No momento, a única fonte de entrada é a entrada do usuário de um Scanner. Isso tornaria muito difícil se você quisesse adicionar algum tipo de adversário de computador ao jogar contra.

Parece que há algum código na classe Board que pode ser mais adequado na classe Player.

Especificamente, o conteúdo do método placeShipsOnBoard ().

Este método recebe a entrada do usuário e cria uma posição. Vamos tentar reestruturar isso para que um jogador (humano ou computador) possa criar uma posição.

Vamos fazer uma interface

public interface Player { Position placeNextShip(); void fireAt(Position target); } 

Já temos implementações de um humano,

public class HumanPlayer implements Player { // variables @Override public Position placeNextShip(){ // uses Scanner instance to create a Position } @Override public void fireAt(Position target){ // code from your attack method } } 

E sobre um reprodutor básico de computador

public class DumbComputer implements Player { @Override public Position placeNextShip(){ // keep choosing random locations } @Override public void fireAt(Position target){ // keep firing at random positions } } 

Em seguida, na classe do Quadro principal, programamos para a interface do reprodutor

while(!isShipPlacementLegal){ for(Player player : players){ // some list of players in the game // either scanner input or randomly generated Position Position shipPlacement = player.placeNextShip(); boolean validPosition = validatePos(shipPlacement); if(validPostion){ // good to go! Place ship and continue to next player } else { // prompt again, whatever else you need to do here. } } } 

Se todo o código se referir a um jogador (agora uma interface) em vez de uma implementação concreta, podemos facilmente adicionar novos tipos de jogadores de computador . por exemplo. novo CheatingComputer (), novo HardComputer (), novo MediumComputer (). Cada um apenas determinaria onde atirar em seguida e onde colocar a próxima nave de uma maneira diferente.

E, se tivéssemos isso, poderíamos fazer um jogo com 2 jogadores de computador e deixá-lo jogar sozinho! Emocionante certo: D

Outra coisa relacionada que poderíamos mudar é que em seu construtor para Game, você sempre terá 2 jogadores Humanos. Poderíamos fazer isso levar uma Lista de jogadores, para que você pudesse ter qualquer número de jogadores em seu jogo. Só porque o jogo real Battleship é limitado a 2 jogadores, não significa que o seu tenha que ser.

Poderíamos permitir um tamanho de grade ajustável e um número de jogador arbitrário. De repente, temos uma grade de 100×100 com um 8 jogador de graça para todos!

(qualquer quantidade pode ser controlada pelo computador).

Suas naves também são inicializadas em um bloco estático em sua classe de tabuleiro. do navio de guerra. Mas, novamente, por que não permitir mais flexibilidade aqui. E se sua nave consistisse em uma lista de pontos? Você poderia ter naves em forma de S, elas não precisariam ser limitadas ao alinhamento horizontal ou vertical. (Isso pode ser exagero, mas acho que é uma coisa legal para se pensar!)

Vou terminar com algumas pequenas coisas que me pareceram engraçadas

throw new ArrayIndexOutOfBoundsException(); 

da classe Position. Esta parece ser uma exceção inadequada para lançar aqui. Não há array na classe Position, então isso deve estar se referindo ao array na classe Board. Acho que um tipo de Exception mais adequado seria IllegalArgumentException, ArrayIndexOutofBoundsException é um detalhe de implementação (de uma classe diferente!). Você deve também certifique-se de fornecer uma mensagem de erro apropriada juntamente com o lançamento da Exceção. Por exemplo, “o valor deve estar dentro do intervalo x e y”

A linha

boolean isShipHit = (ship != null) ? true : false; 

poderia simplesmente ser substituído por

boolean isShipHit = ship != null; 

Não há necessidade do operador ternário aqui.

O uso de targetHistory na classe Player while (targetHistory.get (point)! = null)

Aqui, você está usando um mapa com o único propósito de ver se um elemento está nele. Isso é exatamente o que um conjunto é para!

targetHistory = new HashSet<>(); while(targetHistory.contains(point)){ // re-prompt } 

Comentários

• Muito obrigado pelo insight! Todas essas respostas se complementam outro muito bem! Vou ‘ começar a trabalhar na versão 2.0 com tudo isso em mente.
• Sem problemas, ‘ Fico feliz por você ter achado isso útil! Boa sorte com 2.0!

O que incomoda um pouco com este código é que não uso interfaces, classes abstratas ou herança, mas não consegui encontrar um bom caso de uso para eles aqui.

Não há nada de errado com isso em seu jogo. O conceito do jogo é tão simples que você não precisa de nenhum deles. O problema com os pequenos programas de brinquedos é que normalmente você não precisa aplicar grandes soluções de design. Você só precisa ter certeza de seguir os princípios de design SÓLIDOS habituais.

Agora que esclarecemos isso, vamos veja alguns detalhes do seu código que devem ser melhorados.

O primeiro é bastante óbvio. Não escreva comentários apenas para escrever comentários. Alguns professores gostam de forçar você a escrever um comentário javadoc sobre tudo. Na verdade, sou totalmente contra isso, exceto ao escrever algum tipo de pacote de utilitário que todo mundo deve usar. Normalmente, o código deve ser autodocumentado. E seu código faz um trabalho muito bom nisso. Portanto, apenas remova o comentário que “s basicamente repetindo o nome bem escolhido da variável / função / …

Por exemplo:

/** * Is point between boolean. * * @param point the point * @param position the position * @return the boolean */ public static boolean isPointBetween(Point point, Position position) { 

Que valor esse comentário adiciona para a função?Acho que esse também é o único método que eu mudaria para torná-lo mais legível. Porque não é óbvio que a posição seja composta de um from e to ponto para o qual verificamos se nosso point fica entre eles.

E se o método tivesse esta assinatura:

public static boolean containsPoint(Position position, Point point) { 

Não seria isso faz um pouco mais de sentido?

Devo acrescentar aqui que não sou contra comentários em geral. Mas um comentário deve explicar POR QUE algo é feito dessa maneira. Não como é implementado. Se eu quisesse saber como é implementado, eu teria acabado de olhar o código.

O próximo ponto é ter essa classe Util … Ao contrário de alguns puristas, não tenho nada contra o conceito de classes Util . As classes util podem ser úteis para reunir funções semelhantes. Por exemplo java.lang.Math que agrupa todas as operações aritméticas usuais em uma classe.

O que me preocupa com sua classe Util é que não há realmente uma boa razão para ela existir. As 2 funções que você tem lá são usadas apenas dentro da classe Board. Então, eles poderiam “ter sido private static funções auxiliares dentro dessa classe.

Na verdade, podemos até fazer um pouco melhor depois de alterar a assinatura para o que Eu sugeri anteriormente. E se colocarmos containsPoint(Position position, Point point) { dentro da classe Position em vez de ter a Position como um parâmetro para o método? Então podemos usá-lo assim:

Position shipPosition = //some ship"s position if(shipPosition.contains(targetPoint)) { //handle ship hit } 

Ele se encaixa muito bem aí, não é?

Falando da classe Positions. Tive um pressentimento estranho sobre isso enquanto olhava seu código. A princípio, pensei que você não tinha um something[][] para representar o tabuleiro. Achei que você tivesse representado tudo como pontos em toda a base de código. Isso poderia funcionar, mas torna a impressão da placa difícil. E então percebi que tem um char[][] dentro de sua Board classe. Mas não faria mais sentido colocar os navios imediatamente dentro essa grade sem ter uma classe de posição intermediária?

Eu também percebi outra coisa perigosa sobre o placeShipsOnBoard(). Você realmente deve confiar que seu usuário irá inserir 2 coordenadas válidas? E se o usuário tentar ser engraçado e inserir de = (1,1) a = (2,2)? Devemos permitir isso? Ou se você quiser que ele insira um navio de comprimento 5 e ele insere de = (1,1) a = (1,1), essencialmente encolhendo o navio a um único quadrado (que você tem que bater 5 vezes! Desde o navio tem 5 vidas). Não deveríamos impedi-lo de trapacear assim?

Vejamos uma maneira alternativa de lidar com a colocação dos navios. Primeiro, deixe o usuário escolher se deseja posicionar o navio horizontal ou verticalmente. Em seguida, peça que ele insira a coordenada superior / esquerda do navio. Nós calcularemos nós mesmos os pontos restantes.

Aqui está como a implementação do método real poderia ser:

private Scanner scanner = new Scanner(System.in); private void placeShipsOnBoard() { System.out.printf("%nAlright - Time to place out your ships%n%n"); for(Ship ship : ships) { //awesome for-each loop is better here boolean horizontal = askValidShipDirection(); Point startingPoint = askValidStartingPoint(ship, horizontal); placeValidShip(ship, startingPoint, boolean horizontal); } } private boolean askValidShipDirection() { do { System.out.println("Do you want to place the ship horizontally (H) or vertically (V)?"); String direction = Scanner.nextLine().trim(); while ( !"H".equals(direction) && !"V".equals(direction); return "H".equals(direction); //note here: use "constant".equals(variable) so nullpointer is impossible. //probably not needed, but it"s best practice in general. } private Point askValidStartingPoint(Ship ship, boolean horizontal) { do { //note: do-while more useful here System.out.printf("%nEnter position of %s (length %d): ", ship.getName(), ship.getSize()); Point from = new Point(scanner.nextInt(), scanner.nextInt()); while(!isValidStartingPoint(from, ship.getLength(), horizontal)); return from; } private boolean isValidStartingPoint(Point from, int length, boolean horizontal) { int xDiff = 0; int yDiff = 0; if(horizontal) { xDiff = 1; } else { yDiff = 1; } for(int i = 0; i < lenth; i++) { if(!isInsideBoard(i,from.getY()) { return false; } if(!Constants.BOARD_ICON.equals(board[from.getY()+i*yDiff][from.getX()+i*xDiff])){ return false; } } return true; } private boolean isInsideBoard(int x, int y){ return x <= Constants.BOARD_SIZE && x >= 0 && y <= Constants.BOARD_SIZE && y >= 0 && x <= Constants.BOARD_SIZE && x >= 0 && y <= Constants.BOARD_SIZE && y >= 0; } private void placeValidShip(Ship ship, Point startingPoint, boolean horizontal) { int xDiff = 0; int yDiff = 0; if(horizontal) { xDiff = 1; } else { yDiff = 1; } for(int i = 0; i < ship.getLenth() ; i++) { board[ship.getY() + i*yDiff][ship.getX()+i*xDiff] = Constants.SHIP_ICON; } } 

Agora que acabamos coloque os navios a bordo diretamente, podemos remover a classe Position e todas as suas referências. Isso significa que não sabemos mais se um navio afundou ou não.

Enquanto escrevia isso, percebi que @Timothy Truckle também postou uma resposta. Eu realmente adoro sua solução de usar Field s em vez de char “s para representar o conselho. nosso método de navio local muda para:

private void placeValidShip(Ship ship, Point startingPoint, boolean horizontal) { int xDiff = 0; int yDiff = 0; if(horizontal) { xDiff = 1; } else { yDiff = 1; } for(int i = 0; i < ship.getLenth() ; i++) { board[ship.getY() + i*yDiff][ship.getX()+i*xDiff] = new ShipField(ship); } } 

Dessa forma, podemos verificar se um navio foi destruído completamente ou apenas parcialmente atingido ao atacar um Field.

Agora, em vez de continuar com esta resposta, sugiro que você leia @Timothy” s também e analise os pontos positivos de ambas as respostas. À primeira vista, concordamos ou simplesmente complementamos a resposta um do outro. Portanto, você deve ter algumas dicas decentes sobre como melhorar seu código 🙂

Comentários

• Muito obrigado por uma revisão detalhada! Todos vocês têm pontos sólidos. Como todas as respostas foram tão boas, ‘ marcarei a pessoa que respondeu primeiro.
• ahem ” Não há nada de errado com isso em seu jogo. O conceito do jogo é tão simples que você don ‘ não preciso de nenhum deles. ” – Devo discordar: é ‘ melhor praticar qualquer coisa no modo fácil primeiro. Se você pode ‘ aplicar conceitos e padrões em algo simples, com certeza você pode ‘ t aplicá-los em algo mais complexo.
• A parte sobre os comentários é especialmente importante. Comentários redundantes são uma completa perda de tempo e energia para escrever ite, mantenha e verifique. Apenas comente se não conseguir que o código e as funções se expliquem.

Outras respostas apontaram quase tudo, então adicionarei apenas uma coisa. Parece para mim que você está usando exceções para de alguma forma realizar o controle de fluxo. As exceções não são mecanismos de fluxo de controle .

public Position(Point from, Point to) { if (from.getX() > Constants.BOARD_SIZE || from.getX() < 0 || from.getY() > Constants.BOARD_SIZE || from.getY() < 0 || to.getX() > Constants.BOARD_SIZE || to.getX() < 0 || to.getY() > Constants.BOARD_SIZE || to.getY() < 0) { throw new ArrayIndexOutOfBoundsException(); } this.from = from; this.to = to; } 

E então

} catch (IndexOutOfBoundsException e) { System.out.println("Invalid coordinates - Outside board"); } 

Acho que você deve validar se as coordenadas fornecidas estão dentro do tabuleiro antes de tentar criar o Position. Esta é uma entrada do usuário e é perfeitamente razoável fazê-lo. Você já está validando esse ship.getSize() != Utils.distanceBetweenPoints(from, to). Você estaria fazendo isso no próprio objeto Board, em vez de ter a verificação de posição para Constants.BOARD_SIZE.