読者です 読者をやめる 読者になる 読者になる

ぺぷしのーげん

大企業からスタートアップに転職したアプリケーションエンジニアのブログ

こんなソースコードばかりで疲れ果ててしまったんですけど

f:id:hazakurakeita:20150726114647j:plain

ぱくたそ - フリー写真素材・無料ダウンロード

たまにはエンジニアっぽい記事を。最近色々なアプリケーションのメンテや不具合修正しているんですけど、初めてソースコードに対面するときに使うエネルギーは計り知れません。クラス図とかシーケンス図とかあればいいんですが、弊社そういう高度な開発管理できてないので、ソースコードが設計図みたいなもんです。それでまたそのソースコードがすごいことになってるので最近疲れ果ててしまっております。

 

クラス図を出力してみたら死んだ

とりあえず全体像を掴もうじゃありませんか。会社で使ってるソフトウェアを使ってクラス図を自動生成してみます。

f:id:hazakurakeita:20150820231426p:plain

な、なんかお化けでたー!!!

超巨大クラスと米粒みたいなクラスが複数でてきました。しかもなぜか関連線が一切ない。どうなってるんだこれは。最初意味が分かりませんでしたが、超巨大クラスがでかすぎて表示がズームアウトされており、クラス図中の文字は全て表示されないという状態になってました。で、その超巨大クラスには天文学的な数の変数が定義されており、UIもロジックも全て含んだモンスターと化していました。開始数秒でヤル気は地に落ちます。

 

オブジェクト指向に全然なっていない

いやもうとにかく関数が長い。どんだけロジック書いてるんですか。if文はロジックを分岐するためにあるんじゃないんですよ。なんでもかんでもif文で処理を分けないでください。

  private void Method(Instance instance){
    if(instance is ObjectA)
    {
      // Logic A
    }
    else if(instance is ObjectB)
    {
      // Logic B
    }
    else if(instance is ObjectC)
    {
      // Logic C
    }
    ...

 長い。長すぎるよ。しかもよく見たら途中がifになってて、いつの間にか新たな分岐になってるうううううってこともしばしば。こういうロジックはインスタンスのオブジェクトに記述するべきでしょう。

    private void Method(Instance instance){
      instance.Logic();
      ...

 「いやいや、これどうやって処理分けるんだ」とか言われるともう死にたくなりますね。InstanceはObjectA, B, C, etcのスーパークラスかインターフェースにしておけばこれでOKです。可読性が上がるのはもちろんのこと、メンテナンス性も上がります。影響範囲も狭くなるので、テスト工数を削減することもできます。

 

マジックナンバーで条件分岐されていて、分岐後にマジックナンバーを処理する

    if(magicNumberA == 1)
    {
      magicNumberB = 5;
    }
    else if(magicNumberA == 3)
    {
      magicNumberB = 6;
    }
    else if(magicNumberA == 4)
    {
      magicNumberB = 7;
    }
    else if(magicNumberA > 0 && magicNumberC < 0)
    {
      magicNumberB = 8;
    }
    else
    {
      magicNumberB = 0;
    }

 うわあああああああ(以下略

unkode-mania.net

同志は世界中にいる模様。