はじめに
ほぼタイトルの通りですが、「良いコード/悪いコードで学ぶ設計入門 ―保守しやすい 成長し続けるコードの書き方」という技術書を読んだので、学んだ知識のアウトプットとして以前作成したポートフォリオを一部リファクタリングしてみました。
リファクタリング(エンティティ)
AccountInfo.java
public class AccountInfo {
private String userName;
private String nickName;
private String profile;
private int status;
private int gender;
private int age;
private float height;
private float weight;
private LocalDateTime createAt;
private LocalDateTime updateAt;
public AccountInfo() {
}
public AccountInfo(String userName, String nickName, String profile, int status, int gender, int age,
float height, float weight, LocalDateTime createAt, LocalDateTime updateAt) {
this.userName = userName;
this.nickName = nickName;
this.profile = profile;
this.status = status;
this.gender = gender;
this.age = age;
this.height = height;
this.weight = weight;
this.createAt = createAt;
this.updateAt = updateAt;
}
//以下、プロパティのgetterとsetter
AccountInfoは以下のServiceクラスで使われています。
RegistrationService.java
@Service
public class RegistrationService {
private final RegistrationMapper registrationMapper;
private final PasswordEncoder passwordEncoder;
public RegistrationService(RegistrationMapper registrationMapper,
PasswordEncoder passwordEncoder) {
this.registrationMapper = registrationMapper;
this.passwordEncoder = passwordEncoder;
}
@Transactional(readOnly = false)
public void insertAccount(RegisterForm form) {
LocalDateTime datetime = LocalDateTime.now();
form.setPassword(passwordEncoder.encode(form.getPassword()));
/*～アカウント、パスワード履歴テーブルへinsertする処理～*/
AccountInfo info = new AccountInfo(form.getUserName(),form.getUserName(),null,0,0,0,
165,60,datetime,datetime);
registrationMapper.insertAccountInfo(info);
}
}
AccountInfoService.java
@Service
public class AccountInfoService {
private final AccountInfoMapper accountInfoMapper;
private final PasswordEncoder passwordEncoder;
public AccountInfoService(AccountInfoMapper accountInfoMapper,
PasswordEncoder passwordEncoder) {
this.accountInfoMapper = accountInfoMapper;
this.passwordEncoder = passwordEncoder;
}
@Transactional(readOnly = false)
public void updateAccountInfo(AccountInfoForm form) {
AccountInfo info = new AccountInfo(form.getUserName(),form.getNickName(),
form.getProfile(),form.getStatus(),
form.getGender(),form.getAge(),
form.getHeight(),form.getWeight(),
null,LocalDateTime.now());
accountInfoMapper.updateAccountInfo(info);
}
}
RegistrationServiceクラスはユーザ登録時に、デフォルト値をデータベースへinsertします。また、データ更新時には対応するFormクラスから値を取得して、コンストラクタに渡しています。
まず、コンストラクタの引数が多すぎますね。これに関しては、STSの自動生成機能を使ってしまったために発生しました。後のメンテナンス等を何も考えずに自動生成したばっかりにこんなことになってます。。。
また、RegistrationService内ではコンストラクタで身長、体重などの初期値を設定しているようですが、これも良くありません。初期値をベタ書きしてしまうと、「他のクラスでも初期化したオブジェクトを使いたいとき、重複コードを書くことになる」「初期値の修正が必要な場合、あちこちにベタ書きしたコードを一つ一つ直さなければならない」 といったデメリットが発生してしまいます。
多すぎる引数
引数が多い場合は、「渡す引数をインスタンス変数として持つクラス」を渡すように設計します。ちょうど内部プロパティが同じFormクラスがあるので、引数にFormクラスを渡すように変更します。
また、初期値を設定したインスタンスを返すファクトリメソッドも用意します。初期値をエンティティクラス内で用意することによって、他のクラスに初期化ロジックを書かずに済みますし、初期化ロジックの修正も1つのクラスで完結します。
AccountInfo.java
public class AccountInfo {
//プロパティは省略
private AccountInfo() {
}
static AccountInfo init(String name) {
LocalDateTime datetime = LocalDateTime.now();
AccountInfo info = new AccountInfo();
info.userName = name;
info.nickName = name;
info.profile = null;
info.status = 0;
info.gender = 0;
info.age = 25;
info.height = 165;
info.weight = 60;
info.createAt = datetime;
info.updateAt = datetime;
return info;
}
static AccountInfo of(AccountInfoForm form) {
AccountInfo info = new AccountInfo();
info.userName = form.getUserName();
info.nickName = form.getNickName();
info.profile = form.getProfile();
info.status = form.getStatus();
info.gender = form.getGender();
info.age = form.getAge();
info.height = form.getHeight();
info.weight = form.getWeight();
info.createAt = null;
info.updateAt = LocalDateTime.now();
return info;
}
}
リファクタリング(バリデータクラス)
NotReusedPasswordValidator.java
@Component
public class NotReusedPasswordValidator implements Validator {
private final PasswordEncoder passwordEncoder;
private final PasswordHistoryService passwordHistoryService;
private final PasswordValidator encodedPasswordHistoryValidator; //BeanConfigで設定したBean
public NotReusedPasswordValidator(PasswordEncoder passwordEncoder,
PasswordHistoryService passwordHistoryService,
PasswordValidator encodedPasswordHistoryValidator) {
this.passwordEncoder = passwordEncoder;
this.passwordHistoryService = passwordHistoryService;
this.encodedPasswordHistoryValidator = encodedPasswordHistoryValidator;
}
@Override
public boolean supports(Class<?> clazz) {
return PasswordChangeForm.class.isAssignableFrom(clazz);
}
@Override
public void validate(Object target, Errors errors) {
PasswordChangeForm form = (PasswordChangeForm)target;
String userName = form.getUserName();
String mail = form.getMail();
String newPassword = form.getPassword();
String currentPassword = passwordHistoryService.findPassword(userName, mail);
if(currentPassword != null) {
if(isCurrentPasswordCheck(newPassword,currentPassword)) {
if(isHistoricalPasswordCheck(userName,newPassword)) {
return;
} else {
errors.rejectValue("password",
"PasswordChangeForm.password",
"30日以内に使用したパスワードはご利用できません");
}
} else { //isCurrentPasswordCheckで新パスワードが現パスワードと一致した場合
errors.rejectValue("password",
"PasswordChangeForm.password",
"ログイン中のパスワードと一緒です");
}
} else { //passwordHistoryServiceでメールが取得できない場合
errors.rejectValue("mail",
"PasswordChangeForm.mail",
"メールアドレスに誤りがあります");
}
}
//入力された新パスワードが、現在使われているパスワードと一致した場合はfalseを返す
pprivate boolean isCurrentPasswordCheck(String newPassword,String currentPassword) {
if(passwordEncoder.matches(newPassword, currentPassword)) {
return false;
}
return true;
}
//入力された新パスワードが、過去30日以内に使われたパスワードと一致しないか確認する
private boolean isHistoricalPasswordCheck(String userName,String newPassword) {
LocalDateTime useFrom = LocalDateTime.now().minusDays(30)
.withHour(0).withMinute(0).withSecond(0).withNano(0);
List<PasswordHistory> histories = passwordHistoryService.findUseFrom(userName, useFrom);
//30日以内にパスワードを変えていないのなら、バリデーションは実行せずにtrueを返す
if(histories.isEmpty()) {
return true;
}
List<PasswordData.Reference> historyData = new ArrayList<>();
for (PasswordHistory h : histories) {
//HistoricalReference：履歴パスワードへの参照
historyData.add(new PasswordData.HistoricalReference(h.getPassword()));
}
//PasswordData：設定したルールでパスワード検証を実行するために使用するパスワード関連情報を扱うクラス
PasswordData passwordData = new PasswordData(userName,newPassword,historyData);
RuleResult result = encodedPasswordHistoryValidator.validate(passwordData);
if(result.isValid()) {
return true;
}
return false;
}
}
validateメソッド内でif文がネストされています。どこからどこまでの処理でErrorインスタンスに何を設定したいのか、パッと見で分かりづらく、読み手が理解するのにいたずらに時間を消費することになります。
if文のネスト
if文の複雑なネストは、早期returnで解消できます。if文で判定している条件を反転させ、処理ごとにreturn文を入れることで、その後の処理を行わないようにしています。
Validator.java
@Component
public class NotReusedPasswordValidator implements Validator {
//省略
@Override
public void validate(Object target, Errors errors) {
PasswordChangeForm form = (PasswordChangeForm)target;
String userName = form.getUserName();
String mail = form.getMail();
String newPassword = form.getPassword();
String currentPassword = passwordHistoryService.findPassword(userName, mail);
if(currentPassword == null) {
errors.rejectValue("mail",
"PasswordChangeForm.mail",
"メールアドレスに誤りがあります");
return;
}
if(!isCurrentPasswordCheck(newPassword,currentPassword)) {
errors.rejectValue("password",
"PasswordChangeForm.password",
"ログイン中のパスワードと一緒です");
return;
}
if(!isHistoricalPasswordCheck(userName,newPassword)) {
errors.rejectValue("password",
"PasswordChangeForm.password",
"30日以内に使用したパスワードはご利用できません");
return;
}
return;
}
}
その他
今回のリファクタリングで使った手法以外に、書籍を読んで有益だと感じたことをザックリ羅列します。
- 値をクラス(型)として表現する(値オブジェクト)
- 引数は入力値として受け渡す。引数を編集して戻り値にするのは避けるべき。
- 単一責任の原則(クラスが担う責任は、たったひとつに限定すべき)
- クラス名は、可能な限り具体的で、意味範囲が狭く、目的に特化した名前を選ぶ
終わりに
今回のリファクタリングでは大したことはやっていません。何を当たりまえのことを、と読んで思った方もいらっしゃるかと思いますが、その「当たり前」を理解していないがために「とりあえず動くが設計なイマイチなコード」を書いていました。プログラミングの文法以上に「読みやすく、バグを寄せ付けない書き方」が重要なんだと痛感いたしました。