はじめに
こんにちは。
みなさんご機嫌いかがですか？
文系出身・IT系未経験の新卒エンジニアです。
先日初めてpaizaの問題のコードレビューをしていただいたので、そのときの感想や学びなどを共有いたします。
コードレビューに参加する際の注意事項
-
- レビューする側は、受け手に対する人格否定は避ける
-
- 受け手の側も、自身ではなくコードが評価の対象であることを認識する
-
- レビューをする側は、自分ができていなくても棚に上げて発言してOK
人格否定って何事...？と思っていましたが、がんばって書いたコードを批評の場に出すのは勇気がいるもの。レビューを行う側は言葉の遣い方に気を配る必要があります。他方、受け手側も必要以上に重く受け止める必要はないということですね。双方の心掛けが大切です。
コードレビューでのアクションについては、こちらの記事で詳しく説明されています！
コードレビューで嫌われる人の特徴7選
問題：クラス・構造体メニュー 静的メンバ (paizaランク B 相当)
https://paiza.jp/works/mondai
paizaのレベルアップ問題集については「ユーザー同士で解答を教え合ったり、コードを公開したりするのは自由」ということでしたので公開します。
/*問題：（表示範囲に合わせて改行を追加）
居酒屋で働きながらクラスの勉強をしていたあなたは、お客さんをクラスに見立てることで
勤務時間中の店内の人数や注文の情報を管理できることに気付きました。
全てのお客さんは、ソフトドリンクと食事を頼むことができます。
加えて 20 歳以上のお客さんはお酒を頼むことができます。
20 歳未満のお客さんがお酒を頼もうとした場合はその注文は取り消されます。
また、お酒（ビールを含む）を頼んだ場合、以降の全ての食事の注文 が毎回 200 円引きになります。
今回、この居酒屋でビールフェスをやることになり、ビールの注文が相次いだため、
いちいちビールの値段である 500 円を書くのをやめ、
注文の種類と値段を書く代わりに 0 とだけを書くことになりました。
勤務時間の初めに店内にいるお客さんの人数と与えられる入力の回数、
各注文をしたお客さんの番号とその内容、または退店したお客さんの番号が与えられます。
お客さんが退店する場合はそのお客さんの会計を出力してください。
勤務時間中に退店した全てのお客さんの会計を出力したのち、勤務時間中に退店した客の人数を出力してください。*/
// 自分の得意な言語で
// Let's チャレンジ！！
import java.util.*;
//アルコールを頼んだかどうか・成年かどうかで処理が変わる
class Customer { //未成年のお客さんのクラス
public int old;
public int price;
public boolean isInshu; //true,falseだけを示してくれる
public Customer(int customerOld){
old = customerOld;
price = 0;
isInshu = false;
}
public void orderSoftdrink(int cost){
price += cost; //普通に加算
}
public void orderAlcohol(int cost){//他のものと呼び方をそろえたいのでcost
//何もせず返す
}
public void orderFood(int cost){
price += cost; //普通に加算
}
public void orderBeer(){
//何もせず返す
}
public int getPrice() {
return price; //会計を返す
}
}
class CustomerOtona extends Customer{//できること：アルコールの注文・それ以降の食事が全て200円引き
public CustomerOtona(int customerOld){
super(customerOld);//コンストラクタを親クラスのものに
}
public void orderAlcohol(int cost){//他のものと呼び方をそろえたいのでcost
price += cost;
isInshu = true;
}
public void orderFood(int cost){
price += cost;
if(isInshu){
price -= 200; //アルコールを頼んでいた場合、以降の食事の注文は200円引き
}
}
public void orderBeer(){
price += 500;
isInshu = true;
}
public int getPrice() {
return price;
}
}
public class Main {
public static void main(String[] args) {
Scanner sc = new Scanner(System.in); //標準入力を初期化
int num = sc.nextInt();
int time = sc.nextInt();
int a =0; //退店した人数を入れる
ArrayList<Customer> customerArray = new ArrayList<Customer>();
for(int i=0; i<num; i++){
int old = sc.nextInt();//人数分
if(old<20){//それぞれのクラスのオブジェクトを作ってあげる
Customer customer = new Customer(old);//オブジェクトをつくる
customerArray.add(customer); //配列に加える
}else{
CustomerOtona customer = new CustomerOtona(old); //オブジェクトをつくる
customerArray.add(customer); //配列に加える
}
}
for(int i=0; i<time; i++){
int bango = sc.nextInt();
String type = sc.next();
if(type.equals("0")){
customerArray.get(bango-1).orderBeer();
} else if(type.equals("A")){//退店のとき
System.out.println(customerArray.get(bango-1).getPrice());//会計を出力
a += 1;//退店した人数をプラス
} else {
int cost = sc.nextInt(); //標準入力から値を取ってきて渡してあげる
if(type.equals("softdrink")){
customerArray.get(bango-1).orderSoftdrink(cost);
} else if (type.equals("alcohol")){
customerArray.get(bango-1).orderAlcohol(cost);
} else if (type.equals("food")){
customerArray.get(bango-1).orderFood(cost);
}
}
}
System.out.println(a);//退店した人数を出力
}
}
もらったご意見
-
- コメントの記載方法に注意
-
- クラスの中の変数のアクセス修飾子はprivateが好ましい
あるクラスにおいて、他のクラスの変数に値を代入することは忌避されることだそうです。クラス内の変数にprivateを付けることで、他クラスにおいて勝手に書き換わってしまうことを防ぎます。
それでは、他のクラスからはアクセスできないprivateの変数をどういじればよいのかというと、値を取得するときにはgetter、値を変更するときにはsetterを使うとよいそうです。
このとき、getter、setterはpublicで記述する必要があります。
getter,setterについてはこちらの記事で詳しく解説されています！
【Java】getter・setterのメモ
-
- 変数名等は極力英語を使うように
日本語話者ではない方が自分のコードを見る可能性も考慮して、ということでした。
今回、お酒を注文したかどうかをboolean isInshuで区別したのですが、英語でdrunkの方がよかったようです。
お客さんのクラスも、未成年の場合はCustomer、成年の場合はCustomerOtonaにしていたのですが、分かりやすくChildrenとAdultsなどの名前にすべきでした。
-
- マジックナンバーは避け、定数で定義するとよい
今回で言うと、「お酒を注文した場合、以降の食事は毎回200円引き」の200円です。何度も使う数字を具体的な数値として書いてしまうと、後から数値を変更するときに手間がかかるからということでした。
-
- メソッドが増えすぎると煩雑。ただ、分けた方がバグの箇所が分かりやすくなる
今回は、注文の種類ごとにメソッドを作ってみました。このコードでは5個程度だったのでそこまで苦ではありませんでしたが、注文の種類が100, 200となったときには対応できないかもしれません。
一方で、同じメソッドを何回も使いまわすとバグの発見が難しくなるというデメリットがあるようです。
-
- importの部分は、ArrayListなどいちいち書いた方がよい
とっても便利なimport java.util.*;ですが、読みやすさを意識するなら「何をimportしたのか」を逐一書いた方がいいそうです。
-
- 未成年のisInshuは不要
未成年はお酒を頼めない（頼んだとしても何の処理もされない）ので不要だそうです。
-
- Mainと他のクラスのどちらを上に書くか？
こちらは流派が分かれる話で、Mainを上にして「何の処理をしたいか」の趣旨を明確にするのもいいし、他のクラスを上にして、使うメソッド等をそろえてからMainに入る、という方法でもよいようです。今回は一つのエリアにすべてのコードを記載したのですが、実業務ではクラスファイルごとに分けることが多いので、その場合はあまり気にしなくてもいいということでした。
修正後のコード
getter,setterの部分までは修正できなかったのですが、可能な限りご意見を踏まえてコードを修正しました。
修正箇所はコメントアウトで記しています。
//importを一つ一つ記載
import java.util.ArrayList;
import java.util.Scanner;
//アルコールを頼んだかどうか・成年かどうかで処理が変わる
class Customer { //未成年のお客さんのクラス
int old;
int price;
//drunk(飲酒したかどうか)はAdultクラスのみに記載
public Customer(int customerOld){
old = customerOld;
price = 0;
}
public void orderSoftdrink(int cost){
price += cost; //普通に加算
}
public void orderAlcohol(int cost){//他のものと呼び方をそろえたいのでcost
//何もせず返す
}
public void orderFood(int cost){
price += cost; //普通に加算
}
public void orderBeer(){
//何もせず返す
}
public int getPrice() {
return price; //会計を返す
}
}
class Adult extends Customer{ //名称を変更
int discount = 200; // 値引きの数値を定数に
int beer = 500; // ビールの価格を定数に
public Adult(int customerOld){
super(customerOld);//コンストラクタを親クラスのものに
}
boolean drunk; //true,falseだけを示してくれる
public void orderAlcohol(int cost){//他のものと呼び方をそろえたいのでcost
price += cost;
drunk = true;
}
public void orderFood(int cost){
price += cost;
if(drunk){
price -= discount; //アルコールを頼んでいた場合、以降の食事の注文は200円引き
}
}
public void orderBeer(){
price += beer;
drunk = true;
}
public int getPrice() {
return price;
}
}
public class Main {
public static void main(String[] args) {
Scanner sc = new Scanner(System.in); //標準入力を初期化
int num = sc.nextInt();
int time = sc.nextInt();
int a =0; //退店した人数を示す
ArrayList<Customer> customerArray = new ArrayList<Customer>();
for(int i=0; i<num; i++){
int old = sc.nextInt();//人数分
if(old<20){//それぞれのクラスのオブジェクトを作ってあげる
Customer customer = new Customer(old);//オブジェクトをつくる
customerArray.add(customer); //配列に加える
}else{
Adult customer = new Adult(old); //オブジェクトをつくる
customerArray.add(customer); //配列に加える
}
}
for(int i=0; i<time; i++){
int bango = sc.nextInt();
String type = sc.next();
if(type.equals("0")){
customerArray.get(bango-1).orderBeer();
} else if(type.equals("A")){//退店のとき
System.out.println(customerArray.get(bango-1).getPrice());//会計を出力
a += 1;//退店した人数をプラス
} else {
int cost = sc.nextInt(); //標準入力から値を取ってきて渡してあげる
if(type.equals("softdrink")){
customerArray.get(bango-1).orderSoftdrink(cost);
} else if (type.equals("alcohol")){
customerArray.get(bango-1).orderAlcohol(cost);
} else if (type.equals("food")){
customerArray.get(bango-1).orderFood(cost);
}
}
}
System.out.println(a);//退店した人数を出力
}
}
おわりに―学んだこと・考えたこと・今後の展望
読みやすいコード＝短いコード ではないということ
初学者からすると長いコードを一目見ただけで身構えてしまうものですが、だからといって長いコード＝わかりにくい、短いコード＝わかりやすい、というわけではないことを学びました。
「importの部分はいちいち書いた方がよい」を例にとりましょう。importしたものをいちいち書いたり読んだりするのは時間がかかります。しかし、そこで時間をかけると、はじめに何をimportしたのかを把握しやすく、「読みやすい」コードになるのだそうです。
こうした積み重ねが、コードの意図を読み手に伝えたり、エラーが発生した際に修正個所を特定したりすることを容易にし、結果的に
「意図が伝わるコードを書く手間 ＜ コードの意図が伝わらないために余計にかかる手間」
となるということでした。
ここで、自分が提出したコードの中には、不要だと指摘されたものもたくさんあったなということが頭をよぎります。自分なりに丁寧にわかりやすくしようと思ったことであっても、かえって可読性を低下させてしまうことがあるようです。重要なのはコードの長さではなく、「過不足なく意図を伝える」ということなのでした。どういったコードであれば、「過不足なく意図を伝える」ことができるのか、優れた先人のコードをたくさん読んで目を養う必要があると感じました。
paizaから一歩進んで、実業務の視座で考えること
paizaラーニングでは、一回限りの問題を解き、テストに合格すれば終了という取り組み方をしていました。合格をもらうだけでも精いっぱいという感じだったのですが、残念ながらそれでは不十分なようです。
今回の問題では居酒屋でのビールフェスが想定されています。実業務として考えると、別のイベントを開催することもあるでしょうし、それぞれの商品の価格や値引きの条件も変わることが予想できます。そのような場合にも対応できる保守性の高いコードが求められているということを学びました。修正前の私のコードでは、マジックナンバーを多用してしまい、後から変更することが難しい仕様になっていました。
柔軟性が必要な箇所、絶対に変えてはいけない箇所を見分け、それぞれに適合した仕様を実現することが必要なのであれば、様々な業界で働く人々のニーズを理解することが重要だと思います。
コードを書く際には、機能を使うユーザーのユースケースを細かく想定する習慣をつけようと思いました。
以上です。ありがとうございました。