コーディングのアンチパターンを自分なりにまとめてみた

コードレビューをしてると「なんじゃこりゃぁ！？」というコードにまれに出くわします。
既存のコードとの兼ね合いでなってる場合は、致し方無くても、新規コードまで真似するのは良くないですよね。 

そろそろ新人エンジニアの中には「はじめてのこーでぃんぐ」をする人も現れるのではないでしょうか。

そんな時、参考にするのは当然、既存のコードでしょう。でも、果たして既存のコードは真似するべき綺麗なコードでしょうか？

というわけで、私がレビュー時に良く注意する点をアンチパターンとしてまとめてみました。

ちなみに私はWeb屋さんなので、業界違うと微妙に違うところもありそうですけど、本質的なところは変わらないと思ってます。

パターンの名前は一般的なのをWikipediaから引っ張ってきたり、自分で思いついたのを適当に書いたりしています。

 

太っちょメソッド

名前のとおり大きすぎるメソッドを作るアンチパターンです。

プログラムを長く書いている人なら1000行を超えるメソッドや関数を見たことが何度かあるでしょう。

どんなにグローバル変数を止めてローカル変数にすることで影響を小さくしようとしていても、これでは意味がありません。

コーディング規約以内にするのはもちろんですが、だいたい5から20行くらい。

長くても1画面に収まるサイズにしましょう。それが人間の理解できる限界です。 また、非常にテスタビリティが悪いので、 その点でも改善するべきです。

 

神のメソッド

あらゆる処理が可能な神の如きメソッドを作るアンチパターンです。

また、多くの場合、神は太っています。

execとかrunとかmainという名前には要注意です。

フラグで制御し始めたら危険信号。システムの構成上どうしても引数の内容で処理内容を選択したいときは、ハンドリングだけを対象メソッドにさせて具体的な処理は別なメソッドにしましょう。

StrategyパターンとかCommandパターンとかが解決の糸口になることもあります。 

 

神のオブジェクト

あらゆる処理が可能な神の如きオブジェクトを作るアンチパターンです。

また、神は巨大なるモノであり人による理解は出来ません。

メソッドもそうですが、一つのクラスは一つの仕事だけをやるのが良い設計です（SRP - 単一責務の原則） 。小さいのはステータスですよ？ 

WebアプリではActionが神のオブジェクトになりやすいです。

Actionはセッションやリクエストの操作、Viewの制御に徹して具体的なロジックは別クラスに切り出して扱うことで保守性とテスタビリティが良くなります。

 

例外隠し

例外を途中でcatchして無かった事にするアンチパターンです。

バグが出たときに原因特定が困難を極めるので運用してる人が死にます。

例外処理は、その処理の中でcatchして特別な事をしたい(リトライとか)を除く場合は、最上位まですべてスローして、そこで適切にエラーにするなりのハンドリングを行うのが良いかと思います。

いちいち例外をキャッチしてログに出すのもやめしょう！

 

閉じられないリソース

ファイルのリソースとかを閉じていないアンチパターンです。

finallyに入れ忘れるという単純なケースでも最悪リソースを開放せずに残念なことになります。

やや、複雑なのが、初期化と利用箇所が分かれている場合。不用意にcloseするとそれ以降で利用しているため、エラーになるとかあります。

リソースは原則フィールドには持たずに、ローカル変数として利用し、初期化とfinally-closeをセットで行うことで、問題を解決することができます。

Rubyや関数型系言語で多いようにファイルを操作する処理はLambdaでコールバック関数を渡し、closeを不要にするのも手でしょう。

もちろん、処理が長い場合はメソッドに分割しましょう。 

 

なんでも初期化

 Javaはオブジェクト指向だからとりあえず初期化しちゃうぜ！　ってアンチパターンです。
一般的には問題無いのですがstaticメソッドを集めたUtil系を初期化しちゃうやんちゃな人を見かけたことがあります。
この手のものは利用意図を明確にするために、privateなコンストラクタを作り、外から初期化できないようにするのが良いです。

 

フィールド大好き

不要なほどにフィールドを多様するアンチパターンです。

オブジェクト指向言語の最大の特徴はフィールドによる状態管理です。

非常に便利ですが、一般的にステートフルはステートレスよりもシステムとして複雑なので、持たなくても良い状態を持つと複雑さが増します。

良くあるのがprivateメソッドに引数を渡すのを嫌ってフィールドを利用する場合です。神のオブジェクトと組み合わさると状態の管理が複雑になり可読性・保守性が落ちます。
特にWebApplicationのようなマルチスレッドな環境では、スレッドセーフでも無くなり、テストもしづらくなります。

なので安易なフィールドの導入は控え、なるべく引数で値を渡すようにした方が綺麗なコードとなります。フィールドが多過ぎる場合はInmutableなBeanクラスを作るなど方法で対応しましょう。

 

早熟な最適化

曰く「StreamAPIは遅いから使用するべきではない」「Stringの結合は重いから常にStringBuilderを使うべし」「インスタンスの初期化はコストだから全部シングルトンかstaticで作ろう！」「（ちょっと複雑だけど）この書き方が一番リソース効率が良いです」

分かります。少しでもリソースを最適化したいですよね？ エンジニアの腕の見せ所ですし、速かったり省メモリは単純に楽しいです。

でも、どのくらいの効果があるかはちゃんと考えていますか？ それは貴方のシステムに必要なチューニングですか？

msオーダーのパフォーマンスや積もり重なったメモリが問題になることは実際にあります。何度も経験しました。

それでもほとんどのケースでは全くそれは関係なく、可読性の方が圧倒的に優先されます。遅くなった時、遅くなる所だけ最適化すれば十分なので通常は可読性をあげるテクニックは気にせず使いましょう。

 

コメントが多すぎる!

コメントが多すぎるアンチパターンです。

一般的にコメントは書くべきと言われますが、多様している場合は危険信号です。

何故なら太っちょメソッドや神のメソッドになっている可能性が非常に高いからです。

コメントを書くということは意味のまとまりであるので、そのタイミングでメソッドに分割を考えると良いでしょう 。

コメントが無いと分からないようなコードを極力減らすことが大事なのです。 

 

また、改修者や改修時期なども不要です。これらはバージョン管理システムで管理されるべきものだからです。

基本的にはコメントにはコードを読むだけでは分かり辛い意図などを記載するのが良いコメントとされています。 

 

嘘つきメソッド

メソッド名が誤っている、もしくは著しく不適切なアンチパターンです。

コードの改修漏れで実際は名称と全然関係の無い処理をしている場合があります。

また、 getという名前で値を変更していたり、countと言う名前でvoidを返して、フィールドを変更していたりと個性的な挙動をするメソッドも困り者です。典型的なのはgetXXXでRDBの値を取得してフィールドにセットとかでしょうか。

一般的なコーディングルールにしたがうことで回避できます。特にフィールドの変更など副作用があるものは明確にそれと分かる名前を付けましょう。上の例なんかはloadXXXとするだけでだいぶ変わります。
 

カメレオン変数

常に同じ変数が参照している値が変わるアンチパターンです。

同じ変数ですが、何度も何度も代入していて、全部のコードを読まないと今何が入ってるかを特定できない使い方をしているケースです。

太っちょメソッドと併発することで、非常に可読性の悪いコードを生みます。
関数に適用した結果を再代入する場合や、別の似たような処理の戻り値を格納したい時は、基本的に意味が別になるはずなので別名を付けるのが良いでしょう。

名前を節約したい時は関数への切り出しが有効なこともあります。
同様の理由で、仮引数とかを変更するのも非常に可読性が悪くなるので止めましょう。

Javaの場合はfinalを付けることで、関数型言語のように代入を不可にすることができますが、コードが冗長になるのが玉に瑕です。 

 

移り気なシングルトン

シングルトンオブジェクトの値が頻繁に変わるアンチパターンです。

シングルトンオブジェクトはシステム全体で不変となります。そのためあるところでの変更が別のところに影響するのは良くあることです。

以前見たヒドいケースでは、メッセージファイルの再読み込みをしており、特定のメソッドを経由すると出力結果が大きく変わるというヒドい状態でした。

シングルトン自体はimmutableな不変オブジェクトとして作ってグローバル変数であるリスクを最小にした使い方が良いでしょう。

例に出したようなケースの場合は読み込むメッセージオブジェクトごとにインスタンスを別に作ることで対処できます。 

 

バベルの崩壊

システム内で変数名等が一致しないアンチパターンです。

一つの単語をあるクラスではローマ字で表現し、あるクラスでは英単語で表現し、あるクラスでは謎の略称で表現してあります。

最悪、よそからコピペした変数名を使いまわしていて、意味がおかしいものすらあります。
当然非常に読みづらいです。用語表などを作るのが正しいですが、そんなドキュメント無い場合も多いので、既存のコードを読んだり古株そうな人に尋ねたりしてください。 

また、CamelCaseと スネークを混ぜるなども言語レベルで標準が決まってないケースだと起こりがちなので要注意です。

 

まとめ

さて、ざくっとですが、こんな感じです。多少は何かの参考になればと思います。
では、Happy Hacking!