前記事のうなぎ屋のテーブルのオブジェクト指向にリファクタリングしたものです。
目次
はじめに
この記事は自分で書いた実装を、オブジェクト指向を考慮して書き直そうという目論見です。
また、書いたコードをpaizaに提出して確認する事は致しません。
実装の正しさを確認するのはテストであると思っているからです。
提出しない理由としてはもう1点あります。
現在[2018/11/24]のpaizaの提出方法ですと、以下により難しい為です。
- 複数のクラス宣言をした場合の実行に対応していない
- 提出の際に本質とはあまり関係のない、入力関係の実装(javaだとScanner)が入る
多くの人が学習初期に、main関数に実装したい全ての処理を書き続けるような事をした事はあると思います。
結果として、提出時にはそんなmain関数に詰め込んだような処理を提出する事になります。
※paizaが悪いと言いたいわけではありません。
※決められたルールの上では、表現方法に制限がある事をお伝えしたいです。
前バージョンの問題点洗い出し
だいたいこんな感じかと思います。
最初期コードでは、そもそもChairsクラスすらなかったのでまだマシな状態です。
しかし、もっと責任分割は可能だと思います。
また、実際にやってみると分かるのですがテストが作りづらいです。
実際の実施手順
リファクタリングする前に、テストを確認する
まず、テストの実装を確認していきます。
@Test
public void test_6人テーブルに3名と1名と2名がそれぞれ席を指定した場合は最初の2グループだけ全員が座れる() {
// paizaの例1
int table = 6;
int[][] groupA = new int[][] { { 3, 2 }, { 1, 6 }, { 2, 5 } };
int result = UnagiStore.execute(table, groupA);
assertThat(result, is(4));
}
ローカル変数groupAの配列宣言が、全く直観的でないです。
連想配列のどっちが人数で、どっちが指定した席だったのかが分かりません。
そもそも、このコードを見る限りではその判断はできません。
このコードを数か月後に見た時、groupAはどのようにすれば説明できるでしょうか？
この様に、直観的にテストが書きづらい・分かりづらいというのは致命的です。
実際の業務ロジックはより複雑なケースが多いです。
複雑な実装の証明を行う手段のテストが、そもそも設計しづらいのは問題があります。
何をやっているのか分からない。
何が起きているのか分からない。
といったテストコードは、メンテナンス性にも乏しい為崩壊しやすいです。
テストを書き直す
テストがしやすいコードは実装もシンプルになりやすいです。
ですので、テストコードから先に作成し確認したい事を予め確定しておきます。
その上で、実装が正しい処理であればよいと思っています。
今回の実装で必要な実装は大きく3つです。
- 椅子クラス
- グループ（客）クラス
- うなぎ屋クラス
まず椅子クラスの作成と、グループクラスの作成を目標にテストを書き直します。
再定義：椅子クラス
画像の手順で定義すると、エラーとなりました。
元々の定義では、ChairsのクラスはUnagiStoreの内部クラスでした。
この定義を実現するには、別クラスとして定義しなおす必要があります。
Chairsクラスをsrc.main.refactoringに作成しました。
出来上がったChairsクラスはまだ空の状態です。
package src.main.refactoring;
public class Chairs {
}
その後、インポートを実行します。
しかし、テストクラスはまだエラーとなっています。
引数を持ったコンストラクタが必要なので、引数intのコンストラクタを作成します。
これを、椅子の最大席数として保持しておくつもりです。
できあがったコンストラクタは以下になります。
package src.main.refactoring;
public class Chairs {
public Chairs(int chairNum) {
}
}
その後、UnagiStore.Chairsの実装コードを全て持ってきます。
まだこの時点では、UnagiStoreにもChairsクラスは存在しています。
package src.main.refactoring;
/**
* 座席管理クラス
*
* 指定した座席に客が座れるかどうかを管理し、着席可能な場合のみ席を確保する.
* 指定値を超えた場合でも、エラーとはならずに単純に着席を行わない
* @author Naoto Wada
*
*/
public class Chairs {
private boolean[] chairs;
public Chairs(int capacity) {
this.chairs = new boolean[capacity];
}
/**
* 座席の着席数をカウントする.
*
* 全てが空席だった場合は[0]を返却する.
* @return 着席数
*/
public int countOccupied() {
int occupied = 0;
for (boolean sut : chairs) {
if (sut) {
occupied++;
}
}
return occupied;
}
/**
* 指定された座席から顧客が着席できるかを判定した後着席させる.
*
* 指定した座席が全席数を超えている場合は着席させない.
* 例えば、席数：4, 客数x, 指定席5
*
* 顧客が連番で座れない場合は着席させない.
* 例えば、席数：4, グループ1[客2, 指定席1], グループ2[客2, 指定席2]
* @param customers 客数
* @param position 指定席
*/
public void occupyIfAvailable(int customers, int position) {
if (hasAlreadyOccupied(customers, position)) {
return;
}
occupySeat(customers, position);
}
private boolean hasAlreadyOccupied(int customers, int position) {
int posit = position - 1;
for (int i = 0; i < customers; i++) {
if (chairs[posit % chairs.length]) {
return true;
}
posit++;
}
return false;
}
private void occupySeat(int customers, int position) {
int posit = position - 1;
for (int i = 0; i < customers; i++) {
chairs[posit % chairs.length] = true;
posit++;
}
}
}
クラスが生成出来たら、createChairsを使ってChairsを生成します。
その際、当然ですが設定する席数の変更は行いません。
また。作った定義(chair)がテーブルのint値と被ってしまっています。
そのため、execute()の引数であるint tableを、思い切ってChairsに変更します。
すると、executeではコンパイルエラーが発生します。
Chairsについては、いったん終了です。
次にexecuteを修正します。
再定義：うなぎ屋クラス
UnagiStore.executeの引数を、テストで定義したChairsの引数に変更しました。
すると、引数と同一名称のローカル変数があるためコンパイルエラーになります。（前図）
役割としてはまったく同じなので、UnagiStore.execute内のnew宣言を削除します。
また、UnagiStore.main内のコンパイルエラーもあるため同様に変更しておきます。
その後、UnagiStoreで定義していたChairsクラスを一括削除します。
出来上がったUnagiStoreはこうなります。
package src.main.refactoring;
import java.util.Scanner;
/**
* paizaの長テーブルのうなぎ屋の解法
*
* クラス名を[Main]に変更すれば、そのまま提出可能
* @author Naoto Wada
*
*/
public class UnagiStore {
/**
* paiza用のメイン関数
* @param args
*/
public static void main(String[] args) {
int result = 0;
try (Scanner sc = new Scanner(System.in)) {
String tableAndParties = sc.nextLine();
int capacity = Integer.parseInt(tableAndParties.split(" ")[0]);
Chairs chair = new Chairs(capacity);
int parties = Integer.parseInt(tableAndParties.split(" ")[1]);
int[][] groups = createGroups(sc, parties);
result = execute(chair, groups);
}
System.out.println(result);
}
private static int[][] createGroups(Scanner sc, int parties) {
int[][] groups = new int[parties][2];
for (int i = 0; i < parties; i++) {
String[] groupAndPosition = sc.nextLine().split(" ");
groups[i][0] = Integer.parseInt(groupAndPosition[0]);
groups[i][1] = Integer.parseInt(groupAndPosition[1]);
}
return groups;
}
/**
* 実行関数.
*
* 座席数と入力グループの情報から顧客を着席させる.
*
* @param chair 座席数
* @param groups 入力グループ情報
* @return 着席数
*/
public static int execute(Chairs chair, int[][] groups) {
for (int grpCnt = 0; grpCnt < groups.length; grpCnt++) {
int customers = groups[grpCnt][0];
int position = groups[grpCnt][1];
chair.occupyIfAvailable(customers, position);
}
return chair.countOccupied();
}
}
テスト再実行
この段階で、テストを再実行してください。
内部のふるまいは変えていない為、問題なく通るはずです。
再定義：グループ（客）クラス
Chairsの定義が完了し、それに対してUnagiStoreを変更しました。
また、テスト実行して現段階で問題がない事も確認済みです。
次に、直観的でなかった連想配列部分をグループクラスとして定義します。
Chairsで行った手順とほぼ変わりません。
違うのは、連想配列と同じ結果となるようグループは複数インスタンス生成が必要な所です。
また、Chairsと違い新規クラス作成する為フィールドの定義等が別途必要です。
グループクラスは以下の様に定義しました。
package src.main.refactoring;
public class Group {
private int memberNum;
private int position;
public Group(int memberNum, int position) {
this.memberNum = memberNum;
this.position =position;
}
}
定義後、ヘルパーを整理したテストクラスは以下のような状況です。
Chairsと同様に、GroupもUnagiStore.executeに渡すようにしています。
この時のコンパイルエラーも、Chairsとほぼ同様の対処となります。
intの連想配列で受け取っている部分をGroupに置き換えるだけです。
置き換え後、UnagiStore.executeも修正します。
UnagiStore.executeのfor文は拡張for文へ置き換えできそうです。
また、GroupをそのままChairs.occupyIfAvailableへ渡します。
これにより、見通しがかなり良くなります。（一部問題が発生します）
上記を実行すると、結果的に以下の様にChairsの定義が変更されます。
本来は、Groupの内部情報をgetterにより晒すべきではありません。
これは、Chairsクラスを含むすべてのクラスがGroupの情報を知る事ができ、
状態変更される可能性を含むからです。
この設計だと、いずれ問題になる可能性を含んでいますがいまいち良い解決方法が見いだせなかったので、ひとまずこの実装でいきます（技術負債）。
UnagiStore.executeの修正後、まだUnagiStore.mainでコンパイルエラーが出ています。
intの連想配列を作っていた部分(UnagiStore.createGroups)に変更が必要です。
以下の様に変更します。
new Group()の引数が直観的ではありません。
なので、以下の様に説明変数を使っても良いかもしれません。
これで、少し理解がし易くなります。
これで、グループクラスの生成と、うなぎ屋クラスの修正は完了です。
ドキュメント修正
クラスの新規作成や、publicメソッド修正行いました。
都度修正しておけば良いですが、現在はjavadocが合わない状態となっています。
ですので、javadocを最新化しておきます。
ドキュメントコメントの修正は後にすると忘れがちなので、都度修正したほうが良さそうです
テスト再実行
この段階で、既にコンパイルエラーは全て解消している段階だと思います。
全てのテストクラスを再度書き直して、再実行します。
気持ちよく、全てGreenな状態です。
リファクタリング後の完了形
まず、元々1つのクラスで管理していたのが以下の3つに分離しました。
- UnagiStore
- Chairs
- Group
package src.main.refactoring;
/**
* 座席管理クラス
*
* 指定した座席に客が座れるかどうかを管理し、着席可能な場合のみ席を確保する.
* 指定値を超えた場合でも、エラーとはならずに単純に着席を行わない
* @author Naoto Wada
*
*/
public class Chairs {
private boolean[] chairs;
public Chairs(int capacity) {
this.chairs = new boolean[capacity];
}
/**
* 座席の着席数をカウントする.
*
* 全てが空席だった場合は[0]を返却する.
* @return 着席数
*/
public int countOccupied() {
int occupied = 0;
for (boolean sut : chairs) {
if (sut) {
occupied++;
}
}
return occupied;
}
/**
* 指定された座席から顧客が着席できるかを判定した後着席させる.
*
* 指定した座席が全席数を超えている場合は着席させない.
* 例えば、席数：4, 客数x, 指定席5
*
* 顧客が連番で座れない場合は着席させない.
* 例えば、席数：4, グループ1[客2, 指定席1], グループ2[客2, 指定席2]
*
* @param group 指定席情報と顧客数が入ったグループ情報
*/
public void occupyIfAvailable(Group group) {
if (hasAlreadyOccupied(group)) {
return;
}
occupySeat(group);
}
private boolean hasAlreadyOccupied(Group group) {
int posit = group.getPosition() - 1;
for (int i = 0; i < group.getNumber(); i++) {
if (chairs[posit % chairs.length]) {
return true;
}
posit++;
}
return false;
}
private void occupySeat(Group group) {
int posit = group.getPosition() - 1;
for (int i = 0; i < group.getNumber(); i++) {
chairs[posit % chairs.length] = true;
posit++;
}
}
}
package src.main.refactoring;
/**
* グループ情報クラス.
*
* 以下の状態を保持し、公開する.
* ・int:グループ人数
* ・int:座りたい席の位置情報
*
* @author Naoto Wada
*
*/
public class Group {
private int memberNum;
private int position;
/**
* コンストラクタ
* @param memberNum グループ人数
* @param position 座りたい席の位置情報
*/
public Group(int memberNum, int position) {
this.memberNum = memberNum;
this.position = position;
}
/**
* グループ人数を取得する.
*
* @return グループ人数
*/
public int getNumber() {
//でききれば外部に公開したくない
return this.memberNum;
}
/**
* 座りたい席の位置情報を取得する.
*
* @return 座りたい席の位置情報
*/
public int getPosition() {
//でききれば外部に公開したくない
return this.position;
}
}
package src.main.refactoring;
import java.util.Scanner;
/**
* paizaの長テーブルのうなぎ屋の解法
*
* クラス名を[Main]に変更すれば、そのまま提出可能
* @author Naoto Wada
*
*/
public class UnagiStore {
/**
* paiza用のメイン関数
* @param args
*/
public static void main(String[] args) {
int result = 0;
try (Scanner sc = new Scanner(System.in)) {
String tableAndParties = sc.nextLine();
int capacity = convertInt(tableAndParties.split(" ")[0]);
int parties = convertInt(tableAndParties.split(" ")[1]);
Chairs chair = new Chairs(capacity);
Group[] groups = createGroups(sc, parties);
result = execute(chair, groups);
}
System.out.println(result);
}
private static Group[] createGroups(Scanner sc, int parties) {
Group[] groups = new Group[parties];
for (int i = 0; i < parties; i++) {
String[] groupAndPosition = sc.nextLine().split(" ");
int member = convertInt(groupAndPosition[0]);
int position = convertInt(groupAndPosition[1]);
groups[i] = new Group(member, position);
}
return groups;
}
private static int convertInt(String strNum) {
return Integer.parseInt(strNum);
}
/**
* 実行関数.
*
* 座席数と入力グループの情報から顧客を着席させる.
* グループ毎に毎回判定を行った後、席に座れるようなら着席させる
*
* @param chair 座席情報
* @param groups グループ情報配列
* @return
*/
public static int execute(Chairs chair, Group[] groups) {
for (Group group : groups) {
chair.occupyIfAvailable(group);
}
return chair.countOccupied();
}
}
リファクタリング後に残った問題点
以下のような問題は依然として残っています。
これらの修正を行う場合は、そもそもクラス設計を変更する必要がありそうです。
- Groupが内部情報である、人数や指定する場所をgetterで公開している。
- Chairsの実装が、うなぎ屋に依存する。
getterの公開や、汎用性のなさについては以下の記事が参考になります。
リファクタリング後のメリット
主に、テストの書きやすさが違っています。
つまり、問題の検知が簡単に行えるという事です。
例えば以下の問題に対しても、容易にテストが可能です。
- 指定席が席の最大数を超えた場合、着席できてしまう
- 最大席数を超えた人数を渡すと、全員座れる。
しかし、これらの問題は、元々の要件に無い為勝手に設定する事は難しいです。
(実装上決めうって実装する事は勿論可能です）
テスト後に検知した問題事象に対して
現実的に捉えるとする場合
例えば、以下のようなシチュエーションです。
店員「何名様ですか？」
客 「10人なんですけど、入れますか？」
客 「厨房でご飯食べたいんですけど、いいですか？」
実際こんなお客様がいたら困ってちょっとビックリしますね。
しかし、それでエラーとなって店員さんが完全に固まったら困ります。
現実的には着席させない様にするか、もう一度入力を促す等が考えられます。
店員「今6名までしかご案内ができないので、少々お待ちいただけますか？」
店員「お食事を厨房で召し上がる事はできません。」
システム的に捉えるとする場合
システムとしては、より厳密な実装が必要です。
以下のような対処が検討できます。
- エラーは発生する場合スローして、上位層で再度入力を実行してもらう。
- 入力の段階で防ぐバリデーションチェックを実装する
実装の際には、業務要件上どのように振舞うかによって変わります。
個人的にはバリデーションエラーで防いだ上で、ユーザーに再入力を促すのが好みです。
意図しない入力に対して対応できずに、結果フリーズするシステムは使う側として困ります。
また、意図しない入力に対して応答がないシステムも同様に困ります。
まとめ
長くなりましたが、リファクタリングするといくつか利点が得られることが分かりました。
- テストの書きやすさの向上
- 実装関係の理解の向上
- 後で実装を見直した時、理解がしやすい。
本来であれば、書き始めの段階である程度意識して書くのが良いです。
とはいえ、コードを生み出す時に答えが分かっている事なんてまずありません。
大事なのは、ダメだと分かった時に都度治していく姿勢だと感じます。