감사합니다. 귀하의 코드를 공유해 주셔서 감사합니다.

이 코드로 인해 조금 짜증나는 것은 인터페이스, 추상 클래스 또는 상속을 사용하지 않는다는 것입니다.

OOP를한다는 것은 다음과 같은 특정 원칙을 따른다는 것을 의미합니다.

• 정보 숨김 / 캡슐화
• 단일 책임
• 문제 분리
• KISS (단순하고 어리석게 유지)
• DRY (반복하지 마십시오.)
• “말하세요! 묻지 마세요.”
• 데메테르의 법칙 ( “낯선 사람과 이야기하지 마세요!”)

인터페이스, 복근 전도지 클래스 또는 상속은 모자 원칙을 지원하며 필요에 따라 사용해야합니다. OOP를 “정의”하지 않습니다.

IMHO 접근 방식이 OOP에 실패하는 주된 이유는 “모델”이 기본 유형 char의 배열이기 때문입니다. 이는 궁극적으로 게임 로직에 대한 절차 적 접근 방식으로 이어집니다.

다음과 같은 인터페이스를 생각합니다.

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

여기서 Result는 열거 형입니다.

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

그리고 다른 인터페이스 구현을 갖게됩니다.

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(); } } 

이 정도면 충분합니다. 아이디어를 얻으세요 …

형식적 문제

이름 지정

좋은 이름을 찾는 것은 프로그래밍에서 가장 어려운 부분입니다. 따라서 항상 식별자 이름에 대해 생각할 시간을 가지십시오.

밝은면에서는 Java 명명 규칙을 따릅니다.

하지만 메서드 이름은 동사로 시작해야합니다. 현재 시제. 예 : shipWasHit() 이름은 hit()이어야합니다.
또는 distanceBetweenPoints()는 calculateDistanceBetween()이어야합니다. 여기서 매개 변수는 거리가 점 사이에 있음을 나타내므로이를 메서드 이름에 넣을 필요가 없습니다.

변수 이름을 자세하게 작성하십시오. 대신

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

이 변수는 이름은 다음과 같이 지정됩니다.

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

기술 솔루션이 아닌 문제 도메인에서 이름을 가져옵니다. 예 : SHIP_ICON는 Ship 클래스 내에 다른 상수가없는 경우에만 SHIP 여야합니다. .

댓글

댓글은 코드가 왜 인지 설명해야합니다. . 다른 모든 주석을 제거하십시오.

주석은 구현자가 이행해야하는 계약 을 포함하는 인터페이스 또는 추상 메소드에서만 사용해야합니다.

상수 클래스

함께 속한 것들을 모으십시오. 상수를 사용하는 클래스에서 상수를 정의하십시오.

댓글

• 모든 유효한 포인트, 정말 유용합니다. 감사합니다!

이미 좋은 답변이 몇 가지 있지만 서있는 것을 몇 가지 추가 할 것이라고 생각했습니다. 코드를 살펴보면 나에게 알려졌습니다.

현재 유일한 입력 소스는 스캐너로부터의 사용자 입력입니다. 이로 인해 일종의 컴퓨터 상대를 추가하려는 경우 매우 어렵습니다. Play against.

Board 클래스에 Player 클래스에 더 적합한 코드가있는 것 같습니다.

특히 placeShipsOnBoard () 메서드의 내용입니다.

p>

이 방법은 사용자의 입력을 받아 Position을 생성합니다. 플레이어 (인간 또는 컴퓨터)가 Position을 생성 할 수 있도록이를 재구성 해 보겠습니다.

Let “s make an 인터페이스

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

우리는 이미 인간의 구현을 가지고 있습니다.

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 } } 

그리고 기본 컴퓨터 플레이어

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

그런 다음 메인 Board 클래스에서 플레이어 인터페이스에 프로그래밍

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. } } } 

모든 코드가 구체적인 구현이 아닌 Player (현재 인터페이스)를 참조하는 경우 새로운 유형의 컴퓨터 플레이어를 쉽게 추가 할 수 있습니다. . 예 : 새로운 CheatingComputer (), 새로운 HardComputer (), 새로운 MediumComputer (). 각각은 다음에 발사 할 위치와 다음 배를 배치 할 위치를 다른 방식으로 결정할 것입니다.

그리고 만약 우리가 이것을 가지고 있다면, 우리는 2 명의 컴퓨터 플레이어로 게임을 만들어 스스로 플레이하게 할 수 있습니다! 흥미로운 권리 : D

우리가 변경할 수있는 또 다른 관련 사항은 게임 생성자에 항상 2 명의 인간 플레이어가 있다는 것입니다. 플레이어 목록을 가져 와서 게임에 원하는 수의 플레이어를 가질 수 있습니다. 실제 게임 Battleship이 2 명으로 제한되어 있다고해서 “당신이 그렇게 할 필요는 없습니다.

조정 가능한 그리드 크기와 임의의 플레이어 수를 허용 할 수 있습니다. 갑자기 우리는 8 명이있는 100×100 그리드를 갖게됩니다. 플레이어는 모두 무료입니다!

(컴퓨터로 제어 할 수있는 양에 관계없이)

함선은 보드 클래스의 정적 블록에서도 초기화됩니다. 모든 실제 함선이 있습니다. 하지만 다시 한 번, 여기서 더 많은 유연성을 허용하지 않는 이유는 무엇입니까? 군함이 포인트 목록으로 구성되어 있다면 어떨까요? S 자형 군함을 가질 수 있지만 수직 정렬의 수평으로 제한 할 필요가 없습니다. (정말 이상 할 수도 있지만 “생각해 볼만한 멋진 일입니다!)

저에게 재미있어 보이는 사소한 일로 마무리하겠습니다.

throw new ArrayIndexOutOfBoundsException(); 

이것은 여기에 던질 부적절한 예외처럼 보입니다. Position 클래스에는 배열이 없으므로 Board 클래스의 배열을 참조해야합니다. 더 적합한 Exception 유형은 IllegalArgumentException이고 ArrayIndexOutofBoundsException은 구현 세부 사항 (다른 클래스의!)입니다. 또한 예외 발생과 함께 적절한 오류 메시지를 제공해야합니다. 예 : “값은 x 및 y 범위 내에 있어야합니다.”

행

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

단순히 대체 할 수 있습니다.

boolean isShipHit = ship != null; 

여기에는 삼항 연산자가 필요하지 않습니다.

targetHistory 사용 Player 클래스에서 while (targetHistory.get (point)! = null)

여기서는 요소가 있는지 확인하기위한 목적으로 만 맵을 사용하고 있습니다. 이것이 바로 Set입니다. for!

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

댓글

• 통찰 해 주셔서 감사합니다.이 모든 답변은 각각을 보완합니다. 기타 정말 좋습니다. ‘ 버전 2.0 작업을 시작하겠습니다. 이 모든 것을 염두에두고 있습니다.
• 문제 없습니다. ‘이 기능이 유용하다는 것을 알게되어 기쁩니다. 2.0에서 행운을 빕니다!

이 코드에 대해 조금은 “인터페이스, 추상 클래스 또는 상속을 사용하지 않지만 여기에서 좋은 사용 사례를 찾을 수 없습니다.

게임에 문제가 없습니다. 게임 개념은 너무 단순해서 그런 것들이 필요하지 않습니다. 작은 장난감 프로그램의 문제는 일반적으로 큰 디자인 솔루션을 적용 할 필요가 없다는 것입니다. 일반적인 SOLID 설계 원칙 을 따르고 있는지 확인하기 만하면됩니다.

이제 정리해 보았습니다. 개선해야 할 코드의 세부 사항을 살펴보십시오.

첫 번째는 매우 분명합니다. 주석을 작성하기 위해 주석을 작성하지 마십시오. 일부 교사는 모든 것에 대해 javadoc 주석을 작성하도록 강요합니다. 다른 사람들이 사용해야하는 일종의 유틸리티 패키지를 작성하는 경우를 제외하고는 실제로 이것에 완전히 반대합니다. 일반적으로 말하는 코드는 자체 문서화 여야합니다. 그리고 여러분의 코드는 그 작업을 정말 잘 수행합니다. 따라서 주석을 제거하십시오. 기본적으로 변수 / 함수 / …의 잘 선택된 이름을 반복합니다.

예 :

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

해당 주석은 어떤 값을 추가합니까? 기능에?나는 이것이 또한 더 읽기 쉽게 만들기 위해 변경할 유일한 방법이라고 생각합니다. 위치가 from 및 to 지점으로 구성되어 있는지 확실하지 않기 때문에 point는 그 사이에 있습니다.

메서드에이 서명이 있다면 어떻게됩니까?

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

그렇지 않을 것입니다. 좀 더 이해가 되시나요?

일반적으로 댓글에 반대하지 않는다고 여기에 추가해야합니다.하지만 댓글은 왜 그런 식으로 수행되는지 설명해야합니다. 구현 방법이 아닙니다. 구현 방법을 알고 싶습니다. 코드를 살펴봤을 것입니다.

다음 요점은 Util 클래스를 사용하는 것입니다. 일부 순수 주의자들과 달리 Util 클래스의 개념에 반하는 것이 없습니다. . Util 클래스는 유사한 함수를 조합하는 데 유용 할 수 있습니다. 예를 들어 모든 일반적인 산술 연산을 하나의 클래스로 그룹화하는 java.lang.Math.

당신의 Util 클래스는 그것이 존재하는 데 정말 좋은 이유가 없다는 것입니다. 거기에있는 2 개의 함수는 Board 클래스 내에서만 사용됩니다. 그래서 그들은 “그 클래스 내에서 private static 도우미 함수일 수도있었습니다.

사실, 우리는 서명을 무엇으로 변경 한 후에 조금 더 잘할 수 있습니다. 앞서 제안했습니다. Position div 대신 Position 클래스 안에 containsPoint(Position position, Point point) {를 넣으면 어떨까요? >을 메소드의 매개 변수로 사용 하시겠습니까? 그러면 다음과 같이 사용할 수 있습니다.

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

정말 잘 맞지 않나요?

Positions 수업에 대해 이야기합니다. 나는 당신의 코드를 살펴 보는 동안 이것에 대해 이상한 느낌을 받았습니다. 처음에는 보드를 대표 할 something[][]가 없다고 생각했습니다. 전체 코드베이스에서 모든 것을 포인트로 표시했다고 생각했습니다.이 방법은 작동 할 수는 있지만 보드를 인쇄하는 것이 어색합니다. Board 클래스 내부에 char[][]가있는 것으로 나타났습니다.하지만 즉시 내부에 배를 넣는 것이 이치에 맞지 않을까요? 중간 Position 클래스가없는 그리드?

placeShipsOnBoard()에 대해 또 다른 위험한 점을 발견했습니다. 사용자가 2 개의 유효한 좌표를 입력하는 것을 정말로 신뢰해야합니까? 사용자가 재미있게 시도하고 = (1,1)에서 = (2,2)까지 입력하면 어떻게됩니까? 이것을 허용해야합니까? 또는 길이가 5 인 배를 입력하고 from = (1,1) to = (1,1)을 입력하여 기본적으로 배를 단일 사각형으로 축소 (5 번 쳐야합니다! 5 명의 생명이 있습니다). 그가 이렇게 속이는 것을 막아야하지 않나요?

배를 배치하는 다른 방법을 살펴 보겠습니다. 첫째, 사용자가 배를 수평 또는 수직으로 배치 할 것인지 선택할 수 있습니다. 그런 다음 배의 상단 / 왼쪽 좌표를 입력하도록합니다. 나머지 포인트는 직접 계산합니다.

실제 메소드 구현은 다음과 같습니다.

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; } } 

이제 배를 보드에 직접 배치하면 Position 클래스와 모든 참조를 제거 할 수 있습니다. 이것은 우리가 더 이상 배가 침몰했는지 여부를 알 수 없음을 의미합니다.

이를 입력하는 동안 @Timothy Truckle도 이미 답변을 게시했음을 알았습니다. 저는 char “대신 전용 Field를 사용하여 이사회를 대표하는 그의 솔루션을 정말 좋아합니다.

그래서 장소 함선 방법이 다음과 같이 변경됩니다.

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); } } 

그러면 .

이제이 답변을 계속하는 대신 @ Timothy s도 읽고 두 답변의 장점을 살펴 보시기 바랍니다. 언뜻보기에 우리는 서로의 대답에 동의하거나 단순히 보완합니다. 따라서 코드를 개선하는 방법에 대한 적절한 조언이 있어야합니다. 🙂

댓글

• 자세한 검토를 해주셔서 대단히 감사합니다. 여러분 모두 확실한 포인트를 제시합니다. 모든 답변이 너무 좋았으므로 답변 한 사람에게 ‘ 체크 표시를하겠습니다. 먼저.
• ahem ” 게임에 문제가 없습니다. 게임 개념이 너무 간단해서 할 수 없습니다. ‘ 그 중 어느 것도 필요하지 않습니다. “-동의하지 않습니다. ‘ 연습하는 것이 더 좋습니다. 쉬운 모드에서 먼저 무엇이든 합니다. ‘ 간단한 것에 개념과 패턴을 적용 할 수 없다면 ‘ 더 복잡한 것에 적용하지 마세요.
• 양념에 대한 부분은 특히 중요합니다. 중복 된 댓글은 작성하는 데 시간과 에너지를 완전히 낭비하는 것입니다. 사이트, 유지 및 확인. 자신을 설명 할 코드와 함수를 얻을 수없는 경우에만 주석을 다십시오.

다른 답변에서 거의 모든 것을 지적 했으므로 한 가지만 추가하겠습니다. 저에게는 “예외를 사용하여 어떻게 든 흐름 제어를 수행하고 있습니다.” 예외는 제어 흐름 메커니즘이 아닙니다 .

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; } 

그런 다음

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

Position를 만들기 전에 주어진 좌표가 보드 내부에 있는지 확인해야한다고 생각합니다. 이것은 사용자 입력이며이를 수행하는 것이 합리적입니다. 이미 ship.getSize() != Utils.distanceBetweenPoints(from, to)의 유효성을 검사하고 있습니다. Board 개체 자체에서도 Board 개체 자체에서 Constants.BOARD_SIZE.