業務でのシステムの話。
ログイン後、メニューが会社や人によって変わる条件を調査をしたときに、「クラス使った方が良くない?」と思ったことがあったので、考えをメモしておきます。
現状のコード
以下のような会社クラス、ユーザクラスが存在し、それらの情報を使うビューが実装されています。
ここでは、getter、setterは @Data
により自動的に実装されるものとします。
また、マジックナンバーは本来使うべきではありませんが、定数化を割愛しています。
会社クラス
/**
* 会社の情報を格納するクラス
*/
@Data
public class Company {
private String companyId; // 会社ID
private int option1Flg; // オプション1のフラグ(0 or 1)
private int option2Flg; // オプション2のフラグ(0 or 1)
}
ユーザクラス
/**
* ユーザの情報を格納するクラス
*/
@Data
public class User {
private String userId; // ユーザID
private int userType; // ユーザ種別(0:一般ユーザ、1:管理者)
}
メニュービュー
<% if (company.getOption1Flg() == 1) { %>
// メニュー1を表示
<% } %>
<% if (company.getOption2Flg() == 1) { %>
// メニュー2を表示
<% } %>
<% if (user.getUserType() == 1
&& (company.getOption1Flg() == 1 || company.getOption2Flg() == 1)) { %>
// 管理者メニューを表示
<% } %>
companyはCompanyクラスのオブジェクト、userはUserクラスのオブジェクトと考えてください。
基本的に、各オプションが1であれば、それぞれのメニューを表示するだけです。
管理者メニューだけ特別で、ユーザが管理者で、いずれかのオプションが1であれば表示されるものとなります。
割と見たようなことがあるコードではないでしょうか。
現状では一見問題ありません。
仕様変更後のコード(悪い例)
以下のように仕様を変更しないといけなくなりました。
- ユーザ種別にリーダーを追加する
- オプション3~5を追加する
- オプション3~5に対応するメニュー3~5は、リーダーのみが表示できる
- 管理者メニューは、ユーザが管理者で、オプション1~4のいずれかが1であれば表示(5は無関係)
これを再度コードにしますが、まずは元の形式を踏襲してみます。
会社クラス
/**
* 会社の情報を格納するクラス
*/
@Data
public class Company {
private String companyId; // 会社ID
private int option1Flg; // オプション1のフラグ(0 or 1)
private int option2Flg; // オプション2のフラグ(0 or 1)
private int option3Flg; // オプション3のフラグ(0 or 1)
private int option4Flg; // オプション4のフラグ(0 or 1)
private int option5Flg; // オプション5のフラグ(0 or 1)
}
ユーザクラス
/**
* ユーザの情報を格納するクラス
*/
@Data
public class User {
private String userId; // ユーザID
private int userType; // ユーザ種別(0:一般ユーザ、1:管理者、2:リーダー)
}
メニュービュー
<% if (company.getOption1Flg() == 1) { %>
// メニュー1を表示
<% } %>
<% if (company.getOption2Flg() == 1) { %>
// メニュー2を表示
<% } %>
<% if (user.getUserType() == 2 && company.getOption3Flg() == 3) { %>
// メニュー3を表示
<% } %>
<% if (user.getUserType() == 2 && company.getOption4Flg() == 4) { %>
// メニュー4を表示
<% } %>
<% if (user.getUserType() == 2 && company.getOption5Flg() == 5) { %>
// メニュー5を表示
<% } %>
<% if (user.getUserType() == 1
&& (company.getOption1Flg() == 1 || company.getOption2Flg() == 1)
|| company.getOption3Flg() == 1) || company.getOption4Flg() == 1)) { %>
// 管理者メニューを表示
<% } %>
メニュービューはこれでも一応判別はできますが、だいぶ面倒なコードになってきたのは分かりますよね。
今後、仕様変更が行われると、まずいなというのは感じると思います。
解決策:メニュークラスを追加する
解決策はいろいろとあると思いますが、メニュークラスを追加する設計にします。
併せて、既存クラスも状態判定メソッドを追加します。
会社クラス
/**
* 会社の情報を格納するクラス
*/
@Data
public class Company {
private String companyId; // 会社ID
private int option1Flg; // オプション1のフラグ(0 or 1)
private int option2Flg; // オプション2のフラグ(0 or 1)
private int option3Flg; // オプション3のフラグ(0 or 1)
private int option4Flg; // オプション4のフラグ(0 or 1)
private int option5Flg; // オプション5のフラグ(0 or 1)
public boolean isEnabledOption1() {
return option1Flg == 1;
}
public boolean isEnabledOption2() {
return option2Flg == 1;
}
public boolean isEnabledOption3() {
return option3Flg == 1;
}
public boolean isEnabledOption4() {
return option4Flg == 1;
}
public boolean isEnabledOption5() {
return option5Flg == 1;
}
public boolean isEnabledAdminOption() {
return isEnabledOption1()
|| isEnabledOption2()
|| isEnabledOption3()
|| isEnabledOption4();
}
}
ユーザクラス
/**
* ユーザの情報を格納するクラス
*/
@Data
public class User {
private String userId; // ユーザID
private int userType; // ユーザ種別(0:一般ユーザ、1:管理者、2:リーダー)
public boolean isNormalUser() {
return userType == 0;
}
public boolean isAdmin() {
return userType == 1;
}
public boolean isLeader() {
return userType == 2;
}
}
メニュークラス
/**
* メニュークラス
*/
@RequiredArgsConstructor
@Getter
public class Menu {
private final Company company;
private final User user;
public boolean isEnabledMenu1() {
return company.isEnabledOption1();
}
public boolean isEnabledMenu2() {
return company.isEnabledOption2();
}
public boolean isEnabledMenu3() {
return user.isLeader() && company.isEnabledOption3();
}
public boolean isEnabledMenu4() {
return user.isLeader() && company.isEnabledOption4();
}
public boolean isEnabledMenu5() {
return user.isLeader() && company.isEnabledOption5();
}
public boolean isEnabledAdminMenu() {
return user.isAdmin() && company.isEnabledAdminOption();
}
}
メニュービュー
<% if (menu.isEnabledMenu1()) { %>
// メニュー1を表示
<% } %>
<% if (menu.isEnabledMenu2()) { %>
// メニュー2を表示
<% } %>
<% if (menu.isEnabledMenu3()) { %>
// メニュー3を表示
<% } %>
<% if (menu.isEnabledMenu4()) { %>
// メニュー4を表示
<% } %>
<% if (menu.isEnabledMenu5()) { %>
// メニュー5を表示
<% } %>
<% if (menu.isEnabledAdminMenu()) { %>
// 管理者メニューを表示
<% } %>
※英語が怪しいのは大目に見てください
改善後は、メニューの表示条件や、会社やユーザの状態がとても分かり易くなったと思います。
実際のアプリケーションでは、MenuItemのような一つのメニューに該当するクラスを作り、そのクラスで判断させることも考えられますが、この記事ではここまでに留めておきます。
最後に:使用側で値を判断し始めたら要注意
今回の例では、メニュービューで会社やユーザの状態を直に判断し始めたことに問題がありました。
しかしながら、オブジェクト指向では、
求めるな、命じよ
が基本思想になります。
クラスの責務にもよりますが、値を持つ側に処理を任せましょう。
レガシープロジェクトでは、こういったコードをよく見かけることになります。
もはや手に負えないのがほとんどですが、大体はこういった小さな綻びから始まり、それをリファクタリングせずに残した結果です。
可能であれば、リファクタリングは積極的に行いましょう。