より綺麗なコードにしたい。

import javafx.scene.canvas.GraphicsContext;
import javafx.scene.paint.Color;
import javafx.scene.text.Font;
import javafx.stage.Stage;

import java.util.Random;
 
public class RollDie extends Application 
{
    @Override
    public void start( Stage stage ) 
    {
       // set up window title and size
       GraphicsContext gc = JIGraphicsUtility.setUpGraphics( 
                  stage, "Roll a Die", 700, 400 );
      
       final int START_X = 300, START_Y = 150, ROLL_Y = 125;
       final int DIE_SIZE = 120, DOT_SIZE = 20;
       final int DOT_1 = DOT_SIZE / 2,
                 DOT_2 = DIE_SIZE / 2 - DOT_SIZE / 2,
                 DOT_3 = DIE_SIZE - DOT_SIZE / 2 - DOT_SIZE;
       Font largeFont = new Font( 20 );
       gc.setFont( largeFont );
       
       Random random = new Random();
       
       int roll1 = random.nextInt(6)+1;
       int roll2 = random.nextInt(6)+1;
       
       // draw a pink die
       gc.setFill( Color.PINK ); 
       gc.fillRect( START_X/2, START_Y, DIE_SIZE, DIE_SIZE );
       gc.fillRect( START_X+150, START_Y, DIE_SIZE, DIE_SIZE );
       
       // set dot color
       gc.setFill( Color.BLACK );
       
       switch ( roll1 )
       {
          case 5: // draw upper right and lower left dots
           gc.fillOval( START_X/2 + DOT_3, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X/2 + DOT_1, START_Y + DOT_3,
                         DOT_SIZE, DOT_SIZE ); 
          case 3: // draw upper left and lower right dots
           gc.fillOval( START_X/2 + DOT_1, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X/2 + DOT_3, START_Y + DOT_3,
                         DOT_SIZE, DOT_SIZE ); 
          case 1: // draw center dot
           gc.fillOval( START_X/2 + DOT_2, START_Y + DOT_2,
                        DOT_SIZE, DOT_SIZE );
           break; // stop executing the switch

          case 6: // draw middle left and right dots
           gc.fillOval( START_X/2 + DOT_1, START_Y + DOT_2,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X/2 + DOT_3, START_Y + DOT_2,
                         DOT_SIZE, DOT_SIZE ); 
          case 4: // draw upper right and lower left dots
           gc.fillOval( START_X/2 + DOT_3, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X/2 + DOT_1, START_Y + DOT_3,
                         DOT_SIZE, DOT_SIZE ); 
          case 2: // draw upper left and lower right dots
           gc.fillOval( START_X/2 + DOT_1, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X/2 + DOT_3, START_Y + DOT_3,
                        DOT_SIZE, DOT_SIZE );
           break; // stop executing the switch
                        
       } // end switch
       
       switch ( roll2 )
       {
          case 5: // draw upper right and lower left dots
           gc.fillOval( START_X+150 + DOT_3, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X+150 + DOT_1, START_Y + DOT_3,
                         DOT_SIZE, DOT_SIZE ); 
          case 3: // draw upper left and lower right dots
           gc.fillOval( START_X+150 + DOT_1, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X+150 + DOT_3, START_Y + DOT_3,
                         DOT_SIZE, DOT_SIZE ); 
          case 1: // draw center dot
           gc.fillOval( START_X+150 + DOT_2, START_Y + DOT_2,
                        DOT_SIZE, DOT_SIZE );
           break; // stop executing the switch

          case 6: // draw middle left and right dots
           gc.fillOval( START_X+150 + DOT_1, START_Y + DOT_2,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X+150 + DOT_3, START_Y + DOT_2,
                         DOT_SIZE, DOT_SIZE ); 
          case 4: // draw upper right and lower left dots
           gc.fillOval( START_X+150 + DOT_3, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X+150 + DOT_1, START_Y + DOT_3,
                         DOT_SIZE, DOT_SIZE ); 
          case 2: // draw upper left and lower right dots
           gc.fillOval( START_X+150 + DOT_1, START_Y + DOT_1,
                        DOT_SIZE, DOT_SIZE );
           gc.fillOval( START_X+150 + DOT_3, START_Y + DOT_3,
                        DOT_SIZE, DOT_SIZE );
           break; // stop executing the switch
                        
       } // end switch
       
       int t = roll1 + roll2;
       
       // display the roll number
       gc.fillText( "The roll is " + t, START_X, ROLL_Y );
                  
    }    
    
    public static void main( String [] args ) 
    {
        launch( args );
    }    
}
コード

初心者です。上記のコードはサイコロを二つランダムに表示してその総数をテキストで表示するのみのプログラムです。見ての通り同じ操作が多く見受けられると思います。まずroll1とroll2で整数を取り込んでのですが上級者さん視点だとこんな感じのコードで良いのでしょうか。それともさらに簡略化出来るのでしょうか。メソッドでまとめてみたりしたのですが長くなってしまったので止めました。またswitchも二度も似たような操作をしています。これも同じくメッソドでまとめようとしたのですがこちらはどこからどこまでをまとめればいいか分からずに断念しました。というのは作ってみると、ほとんど最初から含まないと宣言した値が見つけられなくなってしまたり、どちらにせよstageの宣言が見つかなくなってしまいました。どうかどなたかご教授いただけないでしょうか。また皆様のコードの書き方に対する考え方など指摘も含めて教えていただけるとありがたいです。

jimbe

2022/06/04 16:08

OpenJFX 18.0.1 をダウンロードしてコンパイルを試みたのですが、 JIGraphicsUtility がエラーになります。これは何でしょうか。

回答3件

これも同じくメッソドでまとめようとしたのですがこちらはどこからどこまでをまとめればいいか分からずに断念しました。というのは作ってみると、ほとんど最初から含まないと宣言した値が見つけられなくなってしまたり、どちらにせよstageの宣言が見つかなくなってしまいました。

まず同じような2つの処理の、どこが違うのかを見極めます。
switchはroll（roll1・roll2）と、x（START_X / 2・START_X + 150）が可変だということがわかります。
少なくともこの2つは引数にする必要がありそうです。

ほかの必要な変数もすべて引数にすればメソッド化はできますが、ここでいったん落ち着いて考え直します。

なぜ同じようなswitchが並んだのでしょうか。
ダイスの点を２か所に書くためですね？

であればダイスを何に描くか（gc）・位置（x・y）・目（roll）の情報があれば、drawDiceメソッドとして分割できそうです。

よく見るとDIE_SIZE・DOT_SIZE・DOT_1等は、start内になくてもいいことがわかります。

Java
1import javafx.application.Application;
2import javafx.scene.Group;
3import javafx.scene.Scene;
4import javafx.scene.canvas.Canvas;
5import javafx.scene.canvas.GraphicsContext;
6import javafx.scene.paint.Color;
7import javafx.scene.text.Font;
8import javafx.stage.Stage;
9
10import java.util.Random;
11
12public class RollDie extends Application {
13    public static void main(String[] args) { launch(args); }
14
15    private final Random random = new Random();
16    private GraphicsContext gc;
17
18    @Override public void start(Stage stage) {
19        gc = JIGraphicsUtility.setUpGraphics(stage, "Roll a Die", 700, 400);
20        gc.setFont(new Font(20));
21
22        stage.getScene().setOnMouseClicked(e -> roll());
23        roll();
24    }
25
26    private void roll() {
27        final int START_X = 300;
28        final int START_Y = 150;
29        final int ROLL_Y = 125;
30
31        var roll1 = random.nextInt(6) + 1;
32        var roll2 = random.nextInt(6) + 1;
33
34        gc.clearRect(0, 0, gc.getCanvas().getWidth(), gc.getCanvas().getHeight());
35
36        drawDice(roll1, START_X / 2, START_Y);
37        drawDice(roll2, START_X + 150, START_Y);
38
39        gc.fillText("The roll is " + (roll1 + roll2), START_X, ROLL_Y);
40    }
41
42    private void drawDice(int roll, int x, int y) {
43        final int DIE_SIZE = 120;
44        final int DOT_SIZE = 20;
45        final int DOT_1 = DOT_SIZE / 2;
46        final int DOT_2 = DIE_SIZE / 2 - DOT_SIZE / 2;
47        final int DOT_3 = DIE_SIZE - DOT_SIZE / 2 - DOT_SIZE;
48
49        gc.setFill(Color.PINK);
50        gc.fillRect(x, y, DIE_SIZE, DIE_SIZE);
51
52        gc.setFill(Color.BLACK);
53        if (roll == 1 || roll == 3 || roll == 5) {
54            gc.fillOval(x + DOT_2, y + DOT_2, DOT_SIZE, DOT_SIZE); // center
55        }
56        if (roll == 2 || roll == 3 || roll == 4 || roll == 5 || roll == 6) {
57            gc.fillOval(x + DOT_1, y + DOT_1, DOT_SIZE, DOT_SIZE); // upper left
58            gc.fillOval(x + DOT_3, y + DOT_3, DOT_SIZE, DOT_SIZE); // lower right
59        }
60        if (roll == 4 || roll == 5 || roll == 6) {
61            gc.fillOval(x + DOT_3, y + DOT_1, DOT_SIZE, DOT_SIZE); // upper right
62            gc.fillOval(x + DOT_1, y + DOT_3, DOT_SIZE, DOT_SIZE); // lower left
63        }
64        if (roll == 6) {
65            gc.fillOval(x + DOT_1, y + DOT_2, DOT_SIZE, DOT_SIZE); // middle left
66            gc.fillOval(x + DOT_3, y + DOT_2, DOT_SIZE, DOT_SIZE); // middle right
67        }
68    }
69
70    
71    private static class JIGraphicsUtility {
72        public static GraphicsContext setUpGraphics(Stage stage, String title, int height, int width) {
73            stage.setTitle(title);
74            var canvas = new Canvas(height, width);
75            var gc = canvas.getGraphicsContext2D();
76            var root = new Group(canvas);
77            stage.setScene(new Scene(root));
78            stage.show();
79            return gc;
80        }
81    }
82}

クリックで振りなおせるように、rollメソッドも作りました。
random・gcは、フィールド（メンバー変数）にしました。
もしstageをどのメソッドからでも使いたければ、同様にすればよいのです。

switchからifに変えてしまいましたが、ケチをつけてるわけではありません（趣味の問題です^^;

投稿2022/06/05 06:35

TN8001

総合スコア8281

https://github.com/jbill2000/CSC-1043-Intro-to-Programming
誰かの解答のようです。

コメントするつもりが回答してしまいました。
JIGraphicsUtilityが見つかります。

投稿2022/06/04 20:40

編集2022/06/04 23:12

xebme

総合スコア1008

回答へのコメント

xebme

2022/06/04 23:29

MVCパターンの観点から・入出力(CV) - GUIと、モデル(M)（サイコロ）を分離する　モデルの機能、モデルの集合機能などを考える・アプリケーションのシナリオを分離する　main()でシナリオを実行するか　シナリオオブジェクトに状態遷移させるかなどを考える　(シナリオがなければ考えません)

少なくとも(?) 二つの switch はメソッド化すれば 1 つになりますし、"サイコロ" という単位でクラス化することは(例え行数が多少増えても)オブジェクト指向としては意味があると思います。

※以下のコードは実行確認していません。

java
1import java.util.Random;
2
3import javafx.application.Application;
4import javafx.scene.canvas.GraphicsContext;
5import javafx.scene.paint.Color;
6import javafx.scene.text.Font;
7import javafx.stage.Stage;
8
9public class RollDice extends Application {
10  static final int START_X = 300, START_Y = 150, ROLL_Y = 125;
11  static final int[] XS = { START_X / 2, START_X + 150 };
12
13  @Override
14  public void start(Stage stage) {
15    GraphicsContext gc = JIGraphicsUtility.setUpGraphics(stage, "Roll a Dice", 700, 400);
16
17    Random random = new Random();
18    Dice[] dices = Dice.rolls(XS.length, random);
19
20    int total = 0;
21    for(int i=0; i<XS.length; i++) {
22      dices[i].draw(gc, XS[i], START_Y);
23      total += dices[i].value;
24    }
25
26    Font largeFont = new Font(20);
27    gc.setFont(largeFont);
28    gc.fillText("The roll is " + total, START_X, ROLL_Y);
29  }
30
31  public static void main(String[] args) {
32    launch(args);
33  }
34}
35
36class Dice {
37  private static final int SIZE = 120, DOT_SIZE = 20;
38  private static final int DOT_1 = DOT_SIZE / 2;
39  private static final int DOT_2 = SIZE / 2 - DOT_SIZE / 2;
40  private static final int DOT_3 = SIZE - DOT_SIZE - DOT_SIZE / 2;
41  //dx,dy
42  private static final int[][] POINTS = {
43      { DOT_2, DOT_2 },
44      { DOT_1, DOT_1 }, { DOT_3, DOT_3 },
45      { DOT_1, DOT_3 }, { DOT_3, DOT_1 },
46      { DOT_1, DOT_2 }, { DOT_3, DOT_2 }
47  };
48
49  static Dice[] rolls(int n, Random random) {
50    if(n <= 0) throw new IllegalArgumentException("n="+n);
51    Dice[] dices = new Dice[n];
52    for(int i=0; i<n; i++) dices[i] = new Dice(random.nextInt(6) + 1);
53    return dices;
54  }
55
56  final int value;
57  private Dice(int value) {
58    this.value = value;
59  }
60
61  void draw(GraphicsContext gc, int x, int y) {
62    gc.setFill(Color.PINK);
63    gc.fillRect(x, y, SIZE, SIZE);
64
65    gc.setFill(Color.BLACK);
66    for(int i=1-value%2, end=i+value; i<end; i++) {
67      gc.fillOval(x + POINTS[i][0], y + POINTS[i][1], DOT_SIZE, DOT_SIZE);      
68    }     
69  }
70}