私は無能なITエンジニアをしているダメ系と申します。
この記事では、私が過去に駆け出しエンジニアだったときに参加したJavaのSpringMVC勉強会でログイン画面およびユーザ登録編集画面を作成したときに、先輩エンジニアから指摘を受けたコードレビュー結果を晒します。
今思えばしょうもないことで指摘を受けていますが、恥を忍んで晒してみることにしました。駆け出しエンジニアの人の参考にもならないような低レベルの指摘になりますが、私と同じ失敗をしないようにして頂けましたら幸いです。
コードレビューでコメントが付いた12の指摘
1. クラスの名前が分かりにくい
@Repository
public class RegistService {
…
@Controller
@RequestMapping(value = "/update")
public class UpdateControler {
…
@Controller
@RequestMapping(value = "/view")
public class ViewControler {
…
何に対しての登録処理、更新処理クラスなのかが分からない。
ユーザの登録・更新画面だったので、Userを名称として付ければ良かったですね。これだと他に機能が追加されたときの名称にも困るし、この機能自体、何の機能なのか分からなくなってしまいますし。
2. メソッドの名前が統一されていない
@RequestMapping(value = "/regist")
public class RegistControler {
@RequestMapping(method = RequestMethod.GET)
public String registGet(HttpServletRequest request) {メソッド名に意味がないのであれば、初期表示の処理は全て「init」にした方がよい。
初期処理にいちいち名称を付けていたんですが、確かにおっしゃる通りだなと。
3. メソッドの名前がおかしい
private SearchService searchService
@RequestMapping(method = RequestMethod.GET)
public String registGet(HttpServletRequest request, UserModel userModel, Model model) {
return "search";
}「registGet」ってメソッド名おかしくね？ ここは検索機能でしょ？
検索機能なのに、登録を示す機能名を付けてしまっていました。これは完全にコピペミスですね。検索機能のため、「searchUser」に修正しました。
4. メソッドの命名規則が違う
@RequestMapping(method = RequestMethod.POST)
public String Confirm(@Valid @ModelAttribute("userModel") UserModel userModel, BindingResult bindingResult,
Model model, HttpServletRequest request) {
registService.insertUser(userModel);
return "complate";メソッド名の頭は小文字にしてよ。
Javaはクラス名の頭は大文字。メソッド名は小文字が命名規則ですね。これを作った時はこんなことも分かっていなかったのか？
5. 文字列の比較方法が危険
if (userModel.getFirstName() != "") {Stringの比較はequalsを使用してよ。
これも常識的なことですね…。また、 if (userModel.getFirstName().eqauls(“”)) だと userModel.getFirstName()がNullの時にエラーがでるので if (“”.equals(userModel.getFirstName()) とするほうが安全なので、そのように修正しました。
6. 意味のない定義がある
public UserModel userInfo(JdbcTemplate jdbcTemplate, String mailAddress, String pass) {
StringBuffer sql = new StringBuffer();
sql.append("SELECT * FROM m_user WHERE mail_address = ? AND pass = ?");
List<Map<String, Object>> list = jdbcTemplate.queryForList(sql.toString(), mailAddress, pass);
Object object = null;
object = jdbcTemplate.queryForObject(sql.toString(), new Object[] { mailAddress, pass },
new BeanPropertyRowMapper(UserModel.class));
return (UserModel) object;
}Object object = null;の定義が意味ないよ。っていうか、この処理NULLになることあるんじゃないの？
確かにこの定義は意味がない。次の行で即代入しているし。そしてNULLになることを考慮できていないので「ぬるぽ」エラーになるので前処理でNULLチェックとかしないとだめですね。
7. 戻り値が不適切
/**
* 検索条件を指定してユーザーを検索する
* @param jdbcTemplate
* @param userModel
* @return
*/
public List<UserModel> searchOneUser(JdbcTemplate jdbcTemplate, Integer id) {searchOneUserなのに、何でリストを返してるの？
この処理は複数のユーザではなく、1人のみのユーザを返却する処理なので、リストで返す必要がなかったです。メモリも余計に食うし。なんでこんなことをしたのか？まあ、分かっていなかったのか、私のことだからユーザ一覧を検索する処理からコピペして持ってきたんでしょう？
8. NULLになることを考慮できていない
/**
* ログインを判定する
* @param mailAddress
* @param pass
* @return
*/
public Boolean login(String mailAddress, String pass) {
UserModel user = getUserInfo(mailAddress, pass);
if (user.getMailAddress() == null) {
return false;
}
return true;
}userがNullになることがあるんじゃないの？取得できない場合はNullであって、判断したいのは「userがNull」でしょ？
おっしゃる通りですね。また、ID・パスワードで一致するデータがないかどうかをチェックするべきですね。
9. 引数の渡し方が不適切
@Repository
public class MUserDao
/**
* ログイン情報を検索する
* @param jdbcTemplate
* @param mailAddress
* @param pass
* @return
*/
public UserModel userInfo(JdbcTemplate jdbcTemplate, String mailAddress, String pass) {
StringBuffer sql = new StringBuffer();
sql.append("SELECT * FROM m_user WHERE mail_address = ? AND pass = ?");
List<Map<String, Object>> list = jdbcTemplate.queryForList(sql.toString(), mailAddress, pass);各メソッドの「JdbcTemplate jdbcTemplate」の引数は、コンストラクタで渡して保持しておけばいいのでは？
おっしゃる通り、データベースの接続情報をわざわざ無駄な引数として検索処理のメソッドの引数に渡すという暴挙をしておりました。
10. 削除のメソッド名が不適切
public void deleteUser(JdbcTemplate jdbcTemplate, Integer id) {物理削除なのか論理削除なのかメソッド名で分からない。
Daoのメソッドは機能を意識した名称ではなく、処理を意識した名称にするべきでしたね。 また、論理削除で削除フラグを更新するだけなので、updateDeleteFlagとかで良かったですね。
11. 同じ分岐処理がある
if (userModel.getMemo() != "") {
paramList.add("%" + userModel.getMemo() + "%");
}
if (userModel.getMemo() != "") {
sql.append(" AND memo like ? ");
}同じ分岐処理を2回しているのは無駄では？
全く同じif文を2回書いているという…。不具合の原因にもなるし、無駄なコードだし…ね。
12. 文字コードがおかしい
<title>���O�C�����</title>文字化けしてるけど、ファイルのコードがおかしくね？
UTF-8ではなく、Shift-JISになってのかもしれません。当時は文字コードのことも良く分かっていなかったんでしょう。
まとめ
この記事では、駆け出しエンジニアのときにフルボッコにされた12の指摘内容をまとめたコードレビュー結果を晒してみました。
名称が分かりにくいとか命名規則とか基本的なことは指摘を受けないようにしましょう。基本中の基本ですし、名称関連で指摘を受けてしまうと、不具合の原因となるロジックの部分や設計書と異なる部分など、大事なところを確認してもらうところまでも到達できていないレビューになってしまうので、あまり意味のないレビューになってしまいます。
また、レビューする人もいちいち同じような指摘を複数個所に付けなければならないので骨が折れてしまいます。
基本的なことはリーダブルコードなどの書籍で学ぶことができますので、駆け出しエンジニアの人は絶対に必読です。
また、良いコード、悪いコードという書籍でも私が指摘を受けた内容のことは載っています。
コードを見る人の気持ちを考えて、基本的なことで指摘を受けないように本を読んだり、他の人のコードを見るなどするようにしましょう。
