##条件分岐後の処理が冗長な場合
if文で分岐した後の処理で同じコードが存在しているケースをよく見かける。特に、登録ボタンクリックに登録処理を書く場合、画面の情報をDTOに変換する処理で多い。
そこで、この例でサンプルコードを考えてみる。
##サンプルコード
/**
* 画面の情報をDtoに変換する。
* IdやnameはLabelやTextBoxのTextプロパティの値の想定。
* idは新規登録はnullになっている。
**/
public Dto CreateDto(string id, string name, string updateUserId)
{
if (id == null)
{
var dto = new Dto();
dto.Id = -1;
dto.Name = name;
dto.RegistUser = updateUserId;
dto.RegistDate = DateTime.Now;
dto.UpdateUserId = updateUserId;
dto.UpdateDate = DateTime.Now;
return dto;
}
else
{
var dto = GetDto(int.Parse(id));
dto.Name = name;
dto.UpdateUserId = updateUserId;
dto.UpdateDate = DateTime.Now;
return dto;
}
}
/** テーブルからDtoを取得する。コードは省略 **/
private Dto GetDto(int id)
{
return new Dto();
}
/** テーブルのDto。新規の場合、idは-1にする。 **/
public class Dto
{
public int Id { get; set; }
public string Name { get; set; }
public string RegistUser { get; set; }
public DateTime? RegistDate { get; set; }
public string UpdateUserId { get; set; }
public DateTime? UpdateDate { get; set; }
}
java
/**
* 画面の情報をDtoに変換する。
* IdやnameはLabelやTextBoxのTextプロパティの値の想定。
* idは新規登録はnullになっている。
**/
public Dto createDto(String id, String name, String updateUserId) {
Dto dto = null;
if (id == null) {
dto = new Dto();
dto.setId(-1);
dto.setName(name);
dto.setRegistUser(updateUserId);
dto.setRegistDate(new Date());
dto.setUpdateUserId(updateUserId);
dto.setUpdateDate(new Date());
} else {
dto = getDto(Integer.parseInt(id));
dto.setName(name);
dto.setUpdateUserId(updateUserId);
dto.setUpdateDate(new Date());
}
return dto;
}
/** テーブルからDtoを取得する。コードは省略。 **/
private Dto getDto(int id) {
// テーブルからDtoを取得する。コードは省略。
return new Dto();
}
/** テーブルのDto。新規の場合、idは-1にする。 **/
public class Dto {
private int id;
private String name;
private String registUser;
private Date registDate;
private String updateUserId;
private Date updateDate;
public int getId() {
return id;
}
public void setId(int id) {
this.id = id;
}
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public String getRegistUser() {
return registUser;
}
public void setRegistUser(String registUser) {
this.registUser = registUser;
}
public Date getRegistDate() {
return registDate;
}
public void setRegistDate(Date registDate) {
this.registDate = registDate;
}
public String getUpdateUserId() {
return updateUserId;
}
public void setUpdateUserId(String updateUserId) {
this.updateUserId = updateUserId;
}
public Date getUpdateDate() {
return updateDate;
}
public void setUpdateDate(Date updateDate) {
this.updateDate = updateDate;
}
}
##問題点
新規、更新の場合で処理を分けているのは分かるが、同じコードがいくつか存在している。例えば、電話番号が追加された場合、このコードだとDtoへ電話番号を代入するコードを２箇所追加しなければならない。if文で分岐するべき処理だけを分離すれば、この問題は解消される。
##リファクタリング 制限事項
リファクタリング方針について、制限事項を記載しておく。制限事項がなければ、修正方法がいくつでもあるため、本題と逸れてしまうことを避けたい。
1. コードの対応を分かりやすくするため、関数化はしない。
2. 画面の情報はViewModelにするべきなど、前提を変える修正はしない。
3. コードを大幅に変える修正はしない。
#リファクタリング後
/**
* 画面の情報をDtoに変換する。
* IdやnameはLabelやTextBoxのTextプロパティの値の想定。
* idは新規登録はnullになっている。
**/
public Dto CreateDto(string id, string name, string updateUserId)
{
Dto dto = null;
if (id == null)
{
dto = new Dto();
dto.Id = -1;
dto.RegistUser = updateUserId;
dto.RegistDate = DateTime.Now;
}
else
{
dto = GetDto(int.Parse(id));
}
dto.Name = name;
dto.UpdateUserId = updateUserId;
dto.UpdateDate = DateTime.Now;
return dto;
}
/** テーブルからDtoを取得する。コードは省略 **/
private Dto GetDto(int id)
{
return new Dto();
}
/** テーブルのDto。新規の場合、idは-1にする。 **/
public class Dto
{
public int Id { get; set; }
public string Name { get; set; }
public string RegistUser { get; set; }
public DateTime? RegistDate { get; set; }
public string UpdateUserId { get; set; }
public DateTime? UpdateDate { get; set; }
}
java
/**
* 画面の情報をDtoに変換する。
* IdやnameはLabelやTextBoxのTextプロパティの値の想定。
* idは新規登録はnullになっている。
**/
public Dto createDto(String id, String name, String updateUserId) {
Dto dto = null;
if (id == null) {
dto = new Dto();
dto.setId(-1);
dto.setRegistUser(updateUserId);
dto.setRegistDate(new Date());
} else {
dto = getDto(Integer.parseInt(id));
}
dto.setName(name);
dto.setUpdateUserId(updateUserId);
dto.setUpdateDate(new Date());
return dto;
}
/** テーブルからDtoを取得する。コードは省略。 **/
private Dto getDto(int id) {
return new Dto();
}
/** テーブルのDto。新規の場合、idは-1にする。 **/
public class Dto {
private int id;
private String name;
private String registUser;
private Date registDate;
private String updateUserId;
private Date updateDate;
public int getId() {
return id;
}
public void setId(int id) {
this.id = id;
}
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public String getRegistUser() {
return registUser;
}
public void setRegistUser(String registUser) {
this.registUser = registUser;
}
public Date getRegistDate() {
return registDate;
}
public void setRegistDate(Date registDate) {
this.registDate = registDate;
}
public String getUpdateUserId() {
return updateUserId;
}
public void setUpdateUserId(String updateUserId) {
this.updateUserId = updateUserId;
}
public Date getUpdateDate() {
return updateDate;
}
public void setUpdateDate(Date updateDate) {
this.updateDate = updateDate;
}
}
#まとめ
新規、更新に依存するコードとそれ以外を分離することで、保守性が高くなった。
多くの場合、サンプルコードのような状態か、新規用関数と更新用関数に分ける場合が多い。
関数化するまえに冗長なコードが無いか見直してほしい。
尚、このリファクタリング観点は、別の記事「関数呼び出しと条件分岐を分離できないか」と同じである。