ありがとうございますコードを共有してください。

このコードで少しイライラするのは、インターフェイス、抽象クラス、継承を使用しないことです。

OOPを実行するということは、（とりわけ）次の特定の原則に従うことを意味します。

• 情報の非表示/カプセル化
• 単一責任
• 関心の分離
• KISS（シンプルに（そして）愚かにしてください。）
• DRY（繰り返してはいけません。）
• 「教えてください！尋ねないでください。」
• 関心の分離の法則（「見知らぬ人と話さないでください！」）

インターフェース、absトラクトクラス、または継承は帽子の原則をサポートし、必要に応じて使用する必要があります。これらはOOPを「定義」しません。

私見では、アプローチがOOPに失敗する主な理由は、「モデル」がプリミティブ型charの配列であるためです。これは最終的にゲームロジックの手続き型アプローチにつながります。

次のようなインターフェースを考えます：

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

where 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にする必要があります。 。

コメント

コメントでコードがの理由を説明する必要があります。他のすべてのコメントを削除します。

コメントは、実装者が満たさなければならない契約が含まれているインターフェースまたは抽象メソッドでのみ使用する必要があります。

定数クラス

一緒に属するものをまとめます。それらを使用するクラスで定数を定義します。

コメント

• すべての有効なポイントであり、非常に役立ちます。ありがとうございました！

すでにいくつかの良い回答がありますが、私は立っていたもののいくつかを追加したいと思いましたコードを調べたときに私にわかりました。

現時点では、入力のソースはスキャナーからのユーザー入力のみです。これにより、ある種のコンピューターの対戦相手を追加する場合は非常に困難になります。対戦します。

Boardクラスには、Playerクラスに適したコードがあるようです。

具体的には、placeShipsOnBoard（）メソッドの内容です。

p>

このメソッドは、ユーザーからの入力を受け取り、位置を作成します。プレーヤー（人間またはコンピューター）が位置を作成できるように、これを再構築してみましょう。

インターフェイス

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

次に、メインボードクラスで、プレーヤーインターフェイスにプログラムします

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

すべてのコードが具体的な実装ではなくプレーヤー（現在はインターフェイス）を参照している場合、新しいタイプのコンピュータープレーヤーを簡単に追加できます。例えば新しいCheatingComputer（）、新しいHardComputer（）、新しいMediumComputer（）。それぞれが次に発砲する場所と次の船を配置する場所を異なる方法で決定するだけです。

そして、これがあれば、2人のコンピュータープレーヤーでゲームを作成し、それ自体をプレイさせることができます。エキサイティングな権利：D

変更できるもう1つの関連事項は、Gameのコンストラクターでは、常に2人の人間のプレイヤーがいることです。これにプレーヤーのリストを取得させることができるので、ゲームに任意の数のプレーヤーを含めることができます。実際のゲームの戦艦が2人のプレイヤーに制限されているからといって、あなたがそうしなければならないという意味ではありません。

グリッドサイズと任意のプレイヤー数を調整できるようにすることができます。突然、100x100グリッドと8のグリッドができました。プレイヤーはすべて無料です！

（コンピューターで制御できます）

船もボードクラスの静的ブロックで初期化されます。実際の船はすべて揃っています。戦艦からです。しかし、ここでも柔軟性を高めてみませんか。船がポイントのリストで構成されている場合はどうでしょうか。S字型の船を使用できれば、水平方向の垂直方向の配置に限定する必要はありません。 （これはやり過ぎかもしれませんが、「考えるのはクールなことだと思います！）

私にとって面白そうないくつかの小さなことで終わります

throw new ArrayIndexOutOfBoundsException(); 

Positionクラスから。これは、ここでスローするには不適切な例外のようです。 Positionクラスには配列がないため、これはBoardクラスの配列を参照している必要があります。より適切な例外タイプはIllegalArgumentExceptionであると思います。ArrayIndexOutofBoundsExceptionは（別のクラスの）実装の詳細です。また、例外をスローするとともに、適切なエラーメッセージを必ず提供してください。たとえば、「値はxとyの範囲内にある必要があります」

行

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

簡単に置き換えることができます

boolean isShipHit = ship != null; 

ここでは三項演算子は必要ありません。

targetHistoryの使用Playerクラスでwhile（targetHistory.get（point）！= null）

ここでは、要素が含まれているかどうかを確認することのみを目的としてマップを使用しています。これがまさにセットです。 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はすべての通常の算術演算を1つのクラスにグループ化します。

私を悩ませているのは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つの有効な座標を入力することを本当に信頼する必要がありますか？ユーザーが面白くしようとして、from =（1,1）to =（2,2）と入力した場合はどうなりますか？これを許可する必要がありますか？または、長さ5の船を入力して、from =（1,1）から=（1,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クラスとそのすべての参照を削除できます。これは、船が沈んだかどうかがわからなくなったことを意味します。

これを入力しているときに、@ TimothyTruckleがすでに回答を投稿していることに気付きました。ボードを表すためにcharの代わりに専用のFieldを使用するという彼のソリューションが本当に気に入っています。

so配置船の方法が次のように変更されます：

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も読んで、両方の回答の良い点を確認することをお勧めします。一見、私たちはお互いの答えに同意するか、単に補完し合うかのどちらかです。したがって、コードを改善する方法について、適切な指針が必要です:)

コメント

• 詳細なレビューをありがとうございました！皆さんは確かな意見を述べています。すべての回答がとても良かったので、’回答した人にチェックマークを付けます最初に。
• ahem “これはゲームに問題はありません。ゲームのコンセプトは非常にシンプルなので’これらのいずれも必要ありません。”-同意しない必要があります：’練習する方が良い最初にイージーモードで何でも。’シンプルなものに概念やパターンを適用できない場合は、’より複雑なものにそれらを適用しないでください。
• コメントに関する部分は特に重要です。冗長なコメントは、wrに時間とエネルギーを完全に浪費します。 ite、maintain、verify。自分自身を説明するコードと関数を取得できない場合にのみコメントしてください。

他の回答ではほとんどすべてが指摘されているので、1つだけ追加します。 どういうわけかフロー制御を実行するために例外を使用していると私には思います。 例外は制御フローメカニズムではありません。

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の位置をチェックします。 div id = “7108ca8522”>