この資料について
JJUG CCC Fall 2016 にて飛び込みLTで使用した資料に追記したものになります。
解説を追記してみました。
名乗り忘れましたがubansiといいます。
まえがき
ある時のお仕事。
例によって時間はない。
なので、昔の案件のリソースを再利用します。
ただ、そのリソースが酷かった。
言語はJavaだが...。
立場
- 新入社員2年目
- Git推進派として社内への導入をとりつけた
- 技術情報の社内発表を頻繁に行っていた
- プロジェクトには今回からコードレビューを導入
問題点
進行上の問題点
- 仕様書(完成版)がない
- コーディングを行った人物とは連絡が取れない
- 10年以上前のソース
- 時間がないのでリファクタリングはしない方針
- チームのリーダーが解説しながらテンションが下がる
- 例）「これもねぇ、どうかと思うんだけどねぇ…こうやってるんだよ↓」
コードの問題点
設計・コーディングを行った人物はC言語出身で、Java初挑戦でした。
言い伝えでは「あんなコードすぐに捨てろ」といっていたそうです。
コードの例
クラスのフィールドが一部public
object.field.field.method();
みたいな違和感のある呼び出しがあって、みてみるとpublicでした。
改善策
privateにしてgetter/setterを用意しましょう。（不要論もありますが…）
面倒ならアノテーションでgetter/setterを追加できるlombokがおすすめ。
リフレクションによる実装
Eclipseのリファクタリングの機能からカプセル化をすることで、
private化とgetter, setterを追加しました。
すると 動かない！
なぜならリフレクションでフィールド一覧に対応するファイルを読み込むという処理をやっていたのだ！！
（このメソッド何種類の例外が飛ぶんだ！？）
改善策
面倒でも舐めるように書きましょう。
それか何らかのコメントで警戒を促してください
そして、こんなソースに出会ったら、スコープ変えるまえに、
一度Field, Methodのインポートを検索することをおすすめします。
catch(Exception e){}があちこちにある
安心してください。
例外のスーパークラスですべての例外をキャッチしてるので何も飛びません！！
って やめなさい！
改善策
まず、例外は必要にわけて個別にキャッチして、
必要な処理を記述すべきです。
そして、キャッチして何も処理をしないのはダメゼッタイ…
最低でもログを残そう。
何があってもとりあえずnullを返す
なぜcatch(Exception e){}があちこちにあるのか。
それは、異常があったらnullを返すからだった。
おかげで、後半の処理はすべてnull checkが必須になってました。
改善策
Javaなんで例外つかってください！！
また、if文はネストするだけ読みづらくなります。
減らす努力をしましょう。
戻り値がListなどの場合はnullの代わりに空のリストを返すと
その後のfor文で0回のループになりますし、
処理を何もしないクラスを返すという手もあります。
ログは出さない宗派
邪教です。
おかしな値が来た場合に、全く関係無い後処理にてぬるぽが出てました（ガッ
デバッグで逆走しなければならなくすごく面倒です。
改善策
ログを使いましょう。
というか、後処理で使う値がおかしい場合は例外をなげてさっさと別の処理に行きましょう。
クラス名が動詞
run()という何をするのかよくわからないメソッドが大量にありました。
なぜrun()が実装されたのか。
それはクラス名が動詞だからです。
改善策
一般的にクラス名は名詞を使いましょう。
そもそもクラスって実行しないし…
名詞.動詞(目的の物)って書くとわかりやすいというのが高級言語の利点の1つだと思います。
謎の変数instance
メインタスクとなるクラスにinstanceという謎の変数がありました。
しかもスコープはやたら長い。
何者だよ！
※ちなみに、シングルトンってわけではないです。
改善策
役割を表しましょう。
どこかから駅までの距離を表すならtoStationDistみたいな。
短いスコープで適当な変数名が思いつかなければクラス名でもいいと思います。
どうしても、役割に悩む場合は変数自体の役割がおかしい可能性もありますね。
クラス名と役割が分かるような名前をつけると、キャスト例外にならない代入ミスも見つかりやすいです。
例）距離と時間など
int distance = 2000;
int minutes = 30;
...
distance = minutes; // 距離に時間いれるのっておかしくない？
こんな感じでぱっと見でバグが見つかりやすい。
そもそもこういう変数名ならこんなミス起こさずに済むと思います。
引数なしvoidのmakeメソッド
public void make(){
...
}
一体何をする気だ！？
何が起こるか全くわからない！
改善策
引数は動かすのに必要な材料。
戻り値は出来上がるもの。
変数名は何をするか。
それをしっかり意識して名前をつけましょう。
グローバル変数を利用して結果をグローバル変数へ…
さっきのmake()メソッドがある理由は外のスコープを操作してるから！
同じく、何が起こるか全くわからない！
改善策
素材はなるべく渡しましょう。
引数がおおい？
それはメソッドに役割をもたせすぎなのかもしれないです。
もしくはその引数はある単位で1つの概念を表してませんか？
その概念のクラスが作れるんじゃないでしょうか。
謎の継承
メソッドを使いたいがために関係ないクラスを継承してる！
改善策
そのメソッドがあちこちで必要ならUtilクラスを作ればいいと思います。
クラスには意味があるので、それを無視されると謎が生まれます。
テストはもちろん通らない
ま、まぁ、バージョン違うし！
改善策
バージョンも違うので仕方ないところもありましたが
通るようにというか、
リリース前にテストしてください。
しかもカバレッジは10％も無いと思われる
そりゃテスト作れないですよ。
引数も戻り値も無いメソッドが多々あるんですもの…
改善策
同じ引数では同じ戻り値となるようなメソッドであれば、
テストが楽になります。
テストを意識したコードを書くのも重要だとおもいます。
また、カバレッジを100％にするほど無駄なことはありませんが、
せめて、正常動作を保証するレベルではテストが欲しいです。
インデントすら整ってない場所がある
ネストが深い＋インデントがずれている＋スコープが長い
＝上級攻撃魔法
プログラマは混乱した。
改善策
Ctrl + Shift + F (Eclipse)
対策
とりあえず解析しながら次のように処理していきました。
リファクタリング問題
まず、チームのリーダーにリファクタリングを何度も提言。
当然はじめは受け入れられないが、
今回のように再利用するという前提で、
「今片付けなければ、次回もその次も襲ってきます」
と説得し、ある程度のリファクタリングはOKが出ました。
また、リーダーが分かる範囲で無駄な処理も削除していく方針になりました。
※社内発表などである程度説得力を増やしておいたのが吉だったと思います
フィールドのprivate化
Eclipseのリファクタリング機能でリフレクションに注意しながらprivate化しました。
と同時にlombokも導入してgetter/setterメソッドを廃止
命名の最適化
クラスは名詞、メソッドは動詞、変数名は役割
の最低限のルールすら守られていなかったので、解析しながら修正しました。
こちらもEclipseのリファクタリングから変更していきました。
ログの方針の決定
- 処理が停止するようなものはError
- 影響範囲が小さいものはWarning
- その他詳細はDebug
という感じに方針を決定し、catch句でログを出すように追加
ログの内容としては、「ブレークポイントかけなくても分かるように」を合言葉にしました。
結論
「良いコードを書きましょう」
このコードをきっかけに良いコードについて考えるきっかけになりました。
おすすめの本はリーダブルコードです。（ドヤ顔
※読んでいる途中
また、WEBなどに転がっているコーディング規約なども
理由が書いてあるものは知見になるので、読んでみるのはいいと思います。
あとがき
JJUGの懇親会にてLT飛び込み募集と言われて、
- スライド資料なし
- PCのバッテリーなし
- オフィスソフトなし
- 酔っぱらい
の状態から数分でQiitaの下書きからスライド形式に変更し
飛び込みLTを行い
5分ちょうどで終わるとは
我ながらなかなかロックなLTだったと思います。
LTではコードの問題点を箇条書きで挙げていきましたが、
挙げるたびにウケていたのでLTしてよかったなと思いました。
ちなみに下書きで保存していたのは書いた時にはリリースしていなかったからです。
少し前にFindBugsの警告0件の状態でリリースし、
仕様の勘違いに関する現地修正が３～４件ありましたが無事リリースできました。
最後に
現在転職活動中ですので、
良いお話があればぜひTwitterまで。
転職先決まりました。
ご連絡ありがとうございました。