こんにちは。システム部のマツムラです！
今年の夏頃から10年前の負債を返すプロジェクトが絶賛進行中で、バタバタな毎日を送っています。
さて、私たちのチームではコードレビューに関してチーム全員がレビュワーになり開発を進めているのですが、今回のプロジェクトでもほかの人のPR（Pull Request）を見たり逆に指摘してもらうことで、バタバタな中でも日々新しい気づきがあったりします。
この気づきをシステム部みんなに共有したい・・・・！
ということで今日はタイトル通り、GMO Researchシステム部でのPRの上げ方と、ソースを書くときに注意していることについてまとめてみます！
レビューする人に優しいPRの上げ方
まずはPRの上げ方についてです。
PRは「小さい粒」であげることを気をつけています。
どういうことかというと・・
（例）自分が担当してるのは、URLを組み立てる処理
① この処理を作るには、どんな機能が必要だろう？？
⇒ XXXtableを検索
⇒ ZZZZtableも検索
⇒検索結果がAなら１の処理 Bなら２の処理 などなど。
② 作る順番を考える
１．XXXtableを検索
２．ZZZtableを検索
３．１．２を呼び出して、何か処理する機能を作る
この場合、一番初めにXXXtableを作成するintegration層だけ作ります。
次にこのソースのTestクラスを作ると思うのですが、この時点でPRをあげてください。
すると、おおよそ2〜3個のファイルになると思います。
このようにして、小さい粒でPRを上げてあげると、レビュアーはレビューがとても楽で、
時間のかかる作業にもなりません。
ソースコードを書くときの注意点
次に、ソースコードを書くときの注意点です。
GMOリサーチ独自の変数名もバンバン使っちゃってるのですが、悪しからず・・。
function名やdataクラスの名前について
① ～data とか ～infoとかを使わない。
→dataもinfoも特に意味をなさないので、なくてよい。
② checkStatus とかupdateEnquete とかを使わない。
→何するかわからないメソッド名はつけない
⇒isClosed~~ ,isAllowed~~,closeEnqueteなどに変更する
＜参考＞
クラスの命名のアンチパターン
https://qiita.com/magicant/items/8134edf969f9629fa66e
プログラミングでよく使う英単語のまとめ【随時更新】
https://qiita.com/Ted-HM/items/7dde25dcffae4cdc7923
可読性について
①早期returnできないか確認する
後続ロジックが判定結果によって不要な場合は、早期returnする。
【書き換え前】
/* cookie管理フラグ */
var manageCookie = true
/* テスト割付の場合 */
if (TEST_FLG_ON == testFlg) {
manageCookie = false
} else {
/* 許可IPの場合 */
if (cookieIgnoreService.isAllowIp(remoteAddr)) {
manageCookie = false
}
}
/* 特殊モニターの場合 */
if (jnMonitorId in SPECIAL_MONITOR_FROM..SPECIAL_MONITOR_TO) {
manageCookie = false
}
return manageCookie
【早期リターンに書き換えた後】
/* テスト割付はCookieの管理対象外 */
if (TEST_FLG_ON == testFlg) {
return false
}
/* 許可IPはCookie管理対象外 */
if (cookieIgnoreService.isAllowIp(remoteAddr)) {
return false
}
/* 特殊モニターはCookie管理対象外 */
if (jnMonitorId in SPECIAL_MONITOR_FROM..SPECIAL_MONITOR_TO) {
return false
}
// 上記以外はCookie重複チェックの管理対象とする
return true
②戻り値のないメソッドについて
何かチェックして例外を投げるなど戻り値のないメソッドについては、中を読まないと何がどう処理されたのかわからないので trueやfalseを返却し、呼び元で例外処理をする。
【修正前】
このソースコードだと、呼び出し元を見た人は、service.check だけが呼ばれていて、この中でエラーが起きるのかや、これを呼ぶとどうなるのかがわかりません。
/**
* トークンチェックを行う
*
* @param token
* @param jnMonitorId モニターID
* @param enqId 割付ID
*/
fun check(token: String, jnMonitorId: String, enqId: Int) {
// token valueを取得
val tokenValue = cloudpanelRedisAccessor.getStringValue("$REDIS_KEY_ANSWER_TOKEN:$token”)
if (tokenValue.isBlank()) {
throw BusinessException("token is invalid. token:$token")
}
// redisからtoken削除cloudpanelRedisAccessor.deleteKeyValue("$REDIS_KEY_ANSWER_TOKEN:$token")
// token valueチェック
checkTokenValue(tokenValue, jnMonitorId, enqId)
}
private fun checkTokenValue(tokenValue: String, jnMonitorId: String, enqId: Int) {
val arrTokenValue = tokenValue.split(":")
if (arrTokenValue[0] != jnMonitorId) throw BusinessException( "token value is invalid. token_value:$tokenValue jn_monitor_id:$jnMonitorId")
if (arrTokenValue[1] != enqId.toString()) throw BusinessException( "token value is invalid. token_value:$tokenValue enq_id:$enqId")
}
【修正後】
このソースだと、呼び出し元は、isValidTokenがNGだったときに例外を投げているので、このサービスを見た人は何が行われているかわかります。
メソッド名もわかりやすくなってます。
fun isValidToken(token: String, jnMonitorId: String, enqId: String): Boolean {
// token valueを取得
val tokenValue = cloudPanelRedisAccessor.getStringValue("$REDIS_KEY_ANSWER_TOKEN:$token")
if (tokenValue.isBlank()) {
logger.warn("token is invalid. token:'$token'")
return false
}
// redisからtoken削除
cloudPanelRedisAccessor.deleteKeyValue("$REDIS_KEY_ANSWER_TOKEN:$token")
// token valueチェック
return isValidTokenValue(tokenValue, jnMonitorId, enqId)
}
private fun isValidTokenValue(tokenValue: String, jnMonitorId: String, enqId: String): Boolean {
val arrTokenValue = tokenValue.split(":")
if (arrTokenValue[0] != jnMonitorId) {
logger.warn("token value is invalid. token_value:'$tokenValue' jn_monitor_id:'$jnMonitorId'")
return false
}
if (arrTokenValue[1] != enqId) {
logger.warn("token value is invalid. token_value:'$tokenValue' enq_id:'$enqId'")
return false
}
return true
}
③ if の中にOR条件がある場合
if (pageContent.contains(“aaa1”) ||pageContent.contains(“aaa2”)){
～～なにか処理を色々する
}
上記のようなソースコードは、後で条件加えたりしてバグりやすいし、わかりにくいです。
（orとかandがあったら更に！）
こういう場合は、メソッドを切り出して、このメソッド内で判定しましょう。
isExistsAaa(){
}
④ メソッドのパラメータはなるべくDataクラスを使わない
【修正前】
fun getSurveyUrl(panel:Panel, research:ResearchAttributes){
return url=
uniqueUrlListMatter.getUrl(panel.panelType,research.researchId,research.testFlg)
}:String
【修正後】
fun getSurveyUrl(panelType:String, researchId:Int, isTest:Boolean){
return url=
uniqueUrlListMatter.getUrl(panelType, researchId, isTest)
}:String
⑤ 必要なデータは必要な分だけ必要な時に取得
SQLを書くときは、必要な項目だけを取得しましょう。
後で使うから、ここでも使うし、あっちでも使うから・・・と色んなデータを一度でとってそのデータを引き回スト、あとで見た時にかなり分かりにくいです。
もしかして未来で使うかもしれない。
こうしておいたら未来でも使えるかもしれない。
は、実際に必要になったときに修正したり追加したりしましょう！
テストコードについて
最後に、テストコードについてです。
①Mockしすぎない
サービス内で、色々メソッドが分かれている場合はそれぞれのメソッドを個別でテストするはのはOKです。
むしろ、そのメソッドだけのテスト自体はわかりやすいですが、個別でメソッドのUTがあるからといって、Mock不要なシンプルなメソッドをMockしまくらない。
普通に動かせるのはMockしないで動かしましょう。※データ作るのが大変等でMock使う場合もあります。
②１テストケースに1つの観点だけアサートする
一個のテストケースの中に、～～が１なら、××になる。～～が２なら、▲▲になる。
というように複数の評価をしないようにします。
テストケース名が仕様を説明できなくなってしまいます。
最後に
自分の作業に追われすぎていて、他の人のPRを見る時間がなかったりすると思いますが、是非、皆さんも一度レビューをしてみてください。
レビューは、だれがしないといけないなどの決まりはありません。
勉強になるし、仕様も理解できるし、メリットばかりなので、初めてやる言語でも積極的にレビューをしてみてください。
それでは、最後まで読んでくださってありがとうございました！
