質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

新規登録して質問してみよう
ただいま回答率
85.34%
Java

Javaは、1995年にサン・マイクロシステムズが開発したプログラミング言語です。表記法はC言語に似ていますが、既存のプログラミング言語の短所を踏まえていちから設計されており、最初からオブジェクト指向性を備えてデザインされています。セキュリティ面が強力であることや、ネットワーク環境での利用に向いていることが特徴です。Javaで作られたソフトウェアは基本的にいかなるプラットフォームでも作動します。

意見交換

クローズ

3回答

1864閲覧

java 改善案求む。 読みやすいコードについて

shuoga

総合スコア2

Java

Javaは、1995年にサン・マイクロシステムズが開発したプログラミング言語です。表記法はC言語に似ていますが、既存のプログラミング言語の短所を踏まえていちから設計されており、最初からオブジェクト指向性を備えてデザインされています。セキュリティ面が強力であることや、ネットワーク環境での利用に向いていることが特徴です。Javaで作られたソフトウェアは基本的にいかなるプラットフォームでも作動します。

0グッド

1クリップ

投稿2024/12/12 06:04

0

1

テーマ、知りたいこと

java初心者です。
Mapを練習する過程で作ったコードが以下になります。
本職の方から見れば冗長だったり可読性が低いと思うので、改善案があれば教えて下さい。

背景、状況

chatGPTに出された問題の回答として作りました。以下問題文。

問題 4: 学生の年齢と成績を管理
問題: 学生IDをキーとして、名前と複数の成績を管理するプログラムを作成しなさい。

  1. 学生IDをキーとし、名前と複数の成績を持つクラス(オブジェクト)を値とする構造にする。
  2. 学生情報を追加する。
  3. 学生IDを指定してその名前と成績を表示する。
  4. すべての学生の情報を表示する。

コード

java

1import java.util.*; 2 3class Front{ 4 private StudentsScore studentsScore; 5 private Scanner sc; 6 7 Front(){ 8 studentsScore = new StudentsScore(); 9 sc = new Scanner(System.in); 10 } 11 12 public void add(){ 13 System.out.println("Put name : "); 14 String name = sc.nextLine(); 15 int id = makeID(); 16 17 while(studentsScore.getStudentsScore().containsKey(id)){ 18 id = makeID(); 19 } 20 21 System.out.println("The name is " + name + ". \nYour ID is " + id +". \nHow many subject do you add? "); 22 23 int numOfScore = 0; 24 try{ 25 numOfScore = sc.nextInt(); 26 String blank = sc.nextLine(); 27 }catch(Exception e){ 28 System.out.println("Invalid number"); 29 } 30 31 StudentScore studentScore = new StudentScore(name); 32 for(int i = 0; i < numOfScore; i++){ 33 System.out.println("what's subject?"); 34 String sub = sc.nextLine(); 35 System.out.println("what's score ?"); 36 int score = -1; 37 try{ 38 score = sc.nextInt(); 39 }catch(Exception e){ 40 System.out.println("Invalid number"); 41 } 42 43 if(score >= 0) { 44 studentScore.setScore(sub,score); 45 System.out.println(studentScore.getScore()); 46 String blank = sc.nextLine(); 47 } 48 49 else{ 50 System.out.println("try again? y/n"); 51 String ans = sc.next(); 52 String blank = sc.nextLine(); 53 54 if(ans.equals("y")) numOfScore++; 55 else numOfScore = 0; 56 } 57 } 58 if(!studentScore.getScore().isEmpty()) studentsScore.makeStudent(id,studentScore); 59 } 60 61 public void delete(){ 62 System.out.println("type id: "); 63 int id = -1; 64 try{ 65 id = sc.nextInt(); 66 String blank = sc.nextLine(); 67 }catch(Exception e){ 68 System.out.println("Invalid num"); 69 } 70 71 if(id >= 0){ 72 StudentScore studentScore = studentsScore.getStudentsScore().get(id); 73 System.out.println( "id is " + id + ",\nthis student is " + studentScore.getName() + ". \ndo you delete this info? y/n"); 74 String ans = sc.next(); 75 if(ans.equals("y")){ 76 studentsScore.deleteStudent(id); 77 System.out.println("Successfully deleted."); 78 }else{ 79 System.out.println("you said no -> back to menu."); 80 } 81 } 82 83 84 } 85 86 public void find(){ 87 System.out.println("type id: "); 88 int id = -1; 89 try{ 90 id = sc.nextInt(); 91 String blank = sc.nextLine(); 92 }catch(Exception e){ 93 System.out.println("Invalid num"); 94 } 95 96 if(studentsScore.getStudentsScore().containsKey(id)) { 97 StudentScore studentScore = studentsScore.getStudentsScore().get(id); 98 System.out.println("you are " + studentScore.getName() + ". \nand your id is " + id + ".\nand your score :"); 99 for(Map.Entry<String,Integer> score : studentScore.getScore().entrySet()){ 100 System.out.println(" -" + score.getKey() + " : " + score.getValue()); 101 } 102 } 103 } 104 105 public void list(){ 106 System.out.println("this is the list : "); 107 for(Map.Entry<Integer,StudentScore> score : studentsScore.getStudentsScore().entrySet()){ 108 System.out.println(" - " + score.getValue().getName() + " : " + score.getKey()); 109 } 110 } 111 112 private int makeID(){ 113 double random = Math.random() * 10000; 114 return (int) random; 115 } 116} 117 118 119class StudentsScore{ 120 private Map<Integer,StudentScore> studentsScore; 121 StudentsScore(){ 122 // instance StudentScore? 123 studentsScore = new HashMap<>(); 124 } 125 126 public void makeStudent(Integer iD,StudentScore score){ 127 studentsScore.put(iD,score); 128 } 129 130 public Map<Integer,StudentScore> getStudentsScore(){ 131 return studentsScore; 132 } 133 134 public void deleteStudent(int id){ 135 if(studentsScore.containsKey(id)) studentsScore.remove(id); 136 } 137} 138 139 class StudentScore{ 140 private final String name; 141 private final Map<String,Integer> score; 142 143 StudentScore(String name){ 144 this.name = name; 145 score = new HashMap<>(); 146 } 147 148 public String getName(){ 149 return this.name; 150 } 151 152 public void setScore(String subject,Integer score){ 153 this.score.put(subject,score); 154 155 } 156 157 public Map<String,Integer> getScore(){ 158 return score; 159 } 160 161} 162 163public class Main{ 164 public static void main(String[] args) { 165 Front front = new Front(); 166 Scanner sc = new Scanner(System.in); 167 boolean notLeave = true; 168 169 while (notLeave){ 170 System.out.println("---------------------------------\n-MENU \n -1: add student and their score \n -2: delete student info \n -3: find student based on id \n -4: list all student \n -any: end"); 171 String ans = sc.next(); 172 173 switch(ans){ 174 case "1": front.add();break; 175 case "2": front.delete();break; 176 case "3": front.find();break; 177 case "4": front.list();break; 178 default: notLeave = false; 179 } 180 } 181 } 182} 183

気になる質問をクリップする

クリップした質問は、後からいつでもMYページで確認できます。

またクリップした質問に回答があった際、通知やメールを受け取ることができます。

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

回答3

#1

pecmm

総合スコア745

投稿2024/12/12 09:41

とりあえず読んで思いついたことを箇条書きします
(javaのバージョンが不明なので 21 ぐらいとします)

全般

  • フォーマッタかけた方がよい(特にswitchの所)
    IDE の機能を活かしたほうがよい。

もっと細かくメソッドを使った方がよい

  • addメソッドの中にID生成処理のループが露出している。
    • 仮に他の箇所でもID生成したくなったら、そこでも同様にループを埋め込むか?
      →そういうメソッドとして抽出しておくとよい
    • ※同じ処理は何度か繰り返されてから初めて共通化した方がよい……という考えもあるけど、
      この場合はaddメソッドの見通しが悪くなるので最初からメソッド抽出がよい
  • 数値をユーザに入力させる処理
    • 同じ様なロジックがあちこちにある。共通化した方がよい
    • Invalid number の後で処理を続けるのはよくない(再入力を求めるか、すぐにreturnするか、場合によっては例外throwも検討)
      →共通化の際に、戻り値を OptionalInt とかにするとよい
  • 文字列入力
    • deleteStudent で nextLine してない箇所がありますが、無くても大丈夫?
    • ※こういうのを防ぐためにも同様の処理は共通化すると安心

add メソッド

  • numOfScore++ とか numOfScore = 0 はロジックの組み立て方としておかしい。
    普通は入力成功数をカウントして、numOfScore に達したら終了……とか
    • こういうのを防ぐために final が付けられると安心。
      IDE によっては「final に出来る変数は全部 final 付ける」という超強力な入力支援機能があったりします。
      みなさん使いましょう。使いなさい。

未使用変数blank

  • String blank = sc.nextLine(); の変数宣言が不要。
    改行を読み捨てるのなら sc.nextLine(); だけでよい。

StudentsScore クラス

  • StudentsScoreが薄すぎるラッパーなので、もっとロジックを詰め込んでStudentScoreに関するできるだけの責務を持たせるか
    そうでないなら、Front内のただの1データとして、直でMap(HashMap)を持ってしまってもよいぐらい。
  • studentsScore.getStudentsScore() とか、変数を直接保持してるのと全く同じ。
    なのに makeStudent とか deleteStudent のような処理を移譲してるだけの処理もあってどっちつかず。

……と急に言われてもよくわからないかも知れませんが。その場合は以下のどれかをおすすめします

  1. StudentsScore クラスを捨てて、直接変数として持たせる
    (おそらく現状コードとさほど変わらないというかちょっとだけコードが短くなる。)
  2. StudentsScore クラスを最低限残す。
    studentsScore.getStudentsScore() という呼び出しをなくして、直接 studentsScore のメソッドで解決できるようにする
  3. さらに上記に加えて、例えば重複しないID生成処理なんかもこのクラスに持たせる
    StudentScoreのID管理

Map, HashMap のAPIドキュメントをちゃんと読んだほうがよい。

  • deleteStudent で containsKey 呼び出しは本当に必要?

findメソッドの打ち切り

  • (数値入力処理と同じ)id入力で"Invalid num"なら、処理を継続せず明示的にreturnとかしたほうがよい
    ※現状は、ID=-1の生徒がたまたま存在しないので幸運にも入力異常時でも処理異常が起こらない……みたいな偶発的プログラミングっぽさがあります

テキストブロック

  • 複数行Stringリテラルは、テキストブロックを使った方がよい

    java:ビフォー

    1 System.out.println("---------------------------------\n-MENU \n -1: add student and their score \n -2: delete student info \n -3: find student based on id \n -4: list all student \n -any: end");

    java:アフター

    1 System.out.println(""" 2 --------------------------------- 3 -MENU 4 -1: add student and their score 5 -2: delete student info 6 -3: find student based on id 7 -4: list all student 8 -any: end""");
  • 数値を埋め込みたい場合もok

    java:ビフォー

    1 System.out.println("you are " + studentScore.getName() + ". \nand your id is " + id + ".\nand your score :");

    java:アフター

    1 System.out.print(""" 2 you are %s. 3 and your id is %d. 4 and your score : 5 """.formatted(studentScore.getName(), id));

main のループ

  • notLeave変数は要らない
    後続処理が無いのでwhile(true)してdefaultでreturnでよい

入力プロンプトについて

あと、コードの読みやすさじゃないですが…

コンソールアプリでユーザ入力を求める場合は、プロンプトで改行せずに

java

1System.out.print("Put name: ");

とするのが割と一般的だと感じます。

Put name: 【←ここでユーザが入力することになる】

コレのほうが、今入力を求められていることが分かりやすい & 何を入力すべきかがちょっと分かりやすい
ような気がします。

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

#2

hiroki-o

総合スコア1145

投稿2024/12/12 11:10

コメントを入れましょう。
何週間か後、何カ月か後の自分自身のためでもあります。

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

#3

jimbe

総合スコア13230

投稿2024/12/12 12:08

編集2024/12/14 01:24

年齢はどうなったんでしょうか。

一番の問題は冗長でも可読性でもなくカプセル化が出来ていないことに思います。
折角フィールドを private にしているのにメソッドでまんま参照が得られてしまっては public フィールドと変わりません。

それと、 Scanner の nextInt が InputMismatchException を発した時のその後の Scanner の処理は大丈夫でしょうか?
ついでに、同じ元から複数の Scanner を作るのは止めたほうが良いです。

読み易さ的には、クラス名も Main とか Front のような何なのか役割が分からないとか、 StudentScore と StudentsScore みたいに 1 文字しか違わないパッと見間違えそうなのは止めたほうが良いと思います。

まぁ兎にも角にも私好みにザーっと(過不足もあり)

java

1import java.util.*; 2import java.util.function.BiConsumer; 3 4public class StudentsScorer { 5 public static void main(String[] args) { 6 new StudentsScorer(new Scanner(System.in)).execute(); 7 } 8 9 private static BiConsumer<String,Integer> SCORE_PRINTER = 10 (subject,score) -> System.out.println(" -" + subject + " : " + score); 11 private static BiConsumer<Integer,String> LIST_PRINTER = 12 (id,name) -> System.out.println(" - " + name + " : " + id); 13 14 private Scorer scorer = new Scorer(); 15 private Scanner sc; 16 17 StudentsScorer(Scanner sc) { 18 this.sc = sc; 19 } 20 21 void execute() { 22 while(true) { 23 String ans = inputString( 24 "---------------------------------\n" + 25 "MENU\n" + 26 " 1: add student and their score\n" + 27 " 2: delete student info\n" + 28 " 3: find student based on id\n" + 29 " 4: list all student\n" + 30 "any: end"); 31 switch (ans) { 32 case "1": 33 add(); 34 break; 35 case "2": 36 delete(); 37 break; 38 case "3": 39 find(); 40 break; 41 case "4": 42 list(); 43 break; 44 default: 45 return; 46 } 47 } 48 } 49 50 private void add() { 51 String name = inputString("Put name"); 52 System.out.println("The name is " + name + "."); 53 Student student = new Student(name); 54 55 int id = scorer.regist(student); 56 System.out.println("Your ID is " + id + "."); 57 58 while (true) { 59 try { 60 String subject = inputString("what's subject?"); 61 int score = inputInt("what's score?"); 62 student.putScore(subject, score); 63 } catch (NumberFormatException e) { 64 System.out.println("** Invalid score **"); 65 } 66 67 student.putScoresTo(SCORE_PRINTER); 68 String ans = inputString("continue? y/n"); 69 if (!ans.equals("y")) break; 70 } 71 72 if (student.countScores() == 0) { 73 scorer.delete(id); 74 } 75 } 76 77 private void delete() { 78 try { 79 int id = inputInt("type ID"); 80 Student student = scorer.find(id); 81 82 System.out.println("ID is " + id + ","); 83 System.out.println("this student is " + student.name + "."); 84 85 String ans = inputString("do you delete this info? y/n"); 86 if (ans.equals("y")) { 87 scorer.delete(id); 88 System.out.println("Successfully deleted."); 89 } else { 90 System.out.println("you said no -> back to menu."); 91 } 92 93 } catch (NumberFormatException|NotFoundException e) { 94 System.out.println("** Invalid ID **"); 95 } 96 } 97 98 private void find() { 99 try { 100 int id = inputInt("type ID"); 101 Student student = scorer.find(id); 102 103 System.out.println("you are " + student.name + "."); 104 System.out.println("your ID is " + id + "."); 105 System.out.println("and your score :"); 106 student.putScoresTo(SCORE_PRINTER); 107 108 } catch (NumberFormatException|NotFoundException e) { 109 System.out.println("** Invalid ID **"); 110 } 111 } 112 113 private void list() { 114 System.out.println("this is the list :"); 115 scorer.putListTo(LIST_PRINTER); 116 } 117 118 private String inputString(String prompt) { 119 System.out.print(prompt + ": "); 120 return sc.nextLine(); 121 } 122 123 private int inputInt(String prompt) throws NumberFormatException { 124 System.out.print(prompt + ": "); 125 return Integer.parseInt(sc.nextLine()); 126 } 127} 128 129class NotFoundException extends RuntimeException { 130 public NotFoundException(String message) { 131 super(message); 132 } 133} 134 135class Scorer { 136 private final Map<Integer,Student> studentMap = new HashMap<>(); 137 private final Random random = new Random(); 138 139 int regist(Student student) { 140 int id = generateId(); 141 studentMap.put(id, student); 142 return id; 143 } 144 145 Student find(int id) throws NotFoundException { 146 if (!studentMap.containsKey(id)) throw new NotFoundException("id=" + id); 147 return studentMap.get(id); 148 } 149 150 void delete(int id) { 151 studentMap.remove(id); 152 } 153 154 void putListTo(BiConsumer<Integer,String> consumer) { 155 studentMap.entrySet().stream().forEach(e -> consumer.accept(e.getKey(), e.getValue().name)); 156 } 157 158 private int generateId() { 159 while (true) { 160 int id = random.nextInt(10000); 161 if (!studentMap.containsKey(id)) return id; 162 } 163 } 164} 165 166class Student { 167 final String name; 168 private final Map<String,Integer> scoreMap = new HashMap<>(); 169 170 Student(String name) { 171 this.name = name; 172 } 173 174 void putScore(String subject, int score) { 175 scoreMap.put(subject, score); 176 } 177 178 int countScores() { 179 return scoreMap.size(); 180 } 181 182 void putScoresTo(BiConsumer<String,Integer> consumer) { 183 scoreMap.entrySet().stream().forEach(e -> consumer.accept(e.getKey(), e.getValue())); 184 } 185}

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

最新の回答から1ヶ月経過したため この意見交換はクローズされました

意見をやりとりしたい話題がある場合は質問してみましょう!

質問する

関連した質問