ひどいコードをメンテしてきたからこそ実感する、良いコードや良い設計の大切さ

はじめに

先日、社内で「良いコードの書き方やお作法、プログラミングの原則って、どうやったら身に付くんだろうねえ？」という話になりました。
もちろん、「本を読んで勉強する」っていのも勉強法のひとつなんですが、そもそも、もっと強烈なモチベーションがないと、必死になって良いコードの書き方やプログラミングの原則って勉強できないのでは？なんて思ったりします。

強烈なモチベーションというのは、たとえば、

いったい何なん！？このスパゲティコードは！！！
なんでこんなコードを俺がメンテしなきゃあかんの！！？？
あ～、もう最悪や！！俺はこんなコード、絶対に書かへんぞ！！！！

っていうぐらいのモチベーションです。
というか、これは単純に僕のケースですね、はい。

幸い、ソニックガーデンに入ってからは、周りのプログラマがみんなちゃんとしているので、そんな思いをすることはほぼなくなりましたが、前職、前々職ではそんなことがよくありました・・・というよりむしろ、ほとんどそんな経験ばかりだったかもしれません。
昔のブログにはそのへんの愚痴をつらつらと書いてあったりするものもあります（苦笑）。

というわけで、このエントリでは過去に遭遇した「アンチパターン」とその解決策をまとめてみようと思います。

主要な変数が全部連番、かつあらゆる変数がグローバルなシステム

かれこれ十数年前、僕がこの業界に入ってきて最初に担当したのはVB(Visual Basic)5やVB6の保守案件でした。
当時はすでに.NETフレームワークが登場し始めたぐらいの時期だったので、VB自体がやや時代遅れだったのですが、僕が担当していた案件はCOBOL時代のソースコードをほぼそのままVBに移植したアプリケーションでした。

画面に表示される項目はすべて INT001、STR002 のような「型名ハンガリアン＋連番」になっており、なおかつそれがすべてグローバル変数として宣言されていました。
実際はINT001に商品コード、STR002に商品名、みたいな割り振りになっているのですが、コード上の表記はあくまでINT001、STR002です。

' モジュール名も当然連番
Module T01S00031
  Dim INT001 As Integer
  Dim STR002 As String
  Dim STR003 As String
  ' 以下変数が何十個も並ぶ
End Module

さらに最悪なのは「i, j, k」のようなループ用の変数もグローバル変数で宣言されていたことです。
うっかり使い方を間違えると、ループ開始時の初期値がおかしくなったりします。

Module T01S00031
  Dim INT001 As Integer
  ' (以下省略)
  ' モジュールのトップレベルでループ用の変数を宣言
  Dim i, j, k As Integer

  Sub HOGE_RTN()
    For i = 0 To 5
      ' グローバル変数iでループ処理・・・
    Next i
  End Sub
End Module

テーブル名やカラム名も全部連番

コードの上の変数だけでなく、テーブル名やカラム名も全部連番です。
M01S0001は商品マスタ、C0100001は商品コード、みたいなそんな世界です。

ずっとその案件を保守している先輩プログラマは「M01S0001は商品マスタだから、ここは～」みたいに、脳内で変換テーブルを組めていましたが、新人だった僕はそんな芸当ができるはずもなく、いつもテーブル定義表を片手に「ええと、M01S0001は・・・商品マスタ、C0100001は・・・・商品コードか。で、C0100002は・・・？？」みたいに調べていました。

こんな状況なので、コードを書くのに時間がかかるのは言うまでもありません。

解決策

CODE COMPLETE、リーダブルコードを読みましょう！！
「人間にとって理解しやすく、保守しやすいコードとは何か」を教えてくれます。

CODE COMPLETE 第2版上完全なプログラミングを目指して

作者: スティーブマコネル,Steve McConnell,クイープ
出版社/メーカー: 日経BP社
発売日: 2005/03/26
メディア: 単行本
購入: 44人クリック: 1,166回
この商品を含むブログ (288件) を見る

リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック (Theory in practice)

作者: Dustin Boswell,Trevor Foucher,須藤功平,角征典
出版社/メーカー: オライリージャパン
発売日: 2012/06/23
メディア: 単行本（ソフトカバー）
購入: 68人クリック: 1,802回
この商品を含むブログ (140件) を見る

余談

その案件がCOBOLからの移植と知ったのは、ずいぶんあとになってからのことでした。
案件に入った頃は新人だったこともあり、「これがプログラムってやつなんだね～」と思いながら、何もわからずそのコードを（四苦八苦しつつ）保守していました。

まあ、COBOL時代なら「コンピュータの都合に人間が合わせる」というのも理解できますが、そういった制限がなくなったのであれば、人間が読み書きしやすいコードを書きたいものです。

カンマ区切りで複数のコード値を格納しているテーブル

売上げテーブルの商品コードカラムに、

ABC001, LMN123, XYZ111

のように、カンマ区切りの文字列が格納されているプログラムを見たことがあります。
画面上にはこれが

肉, にんじん, たまねぎ

のようになって、商品名が出てくるのですが、これを実現するためにストアドファンクションの中でいちいち文字列をカンマでぶったぎって、ループを回して商品マスタから1件ずつ商品名を取得していました。

適当な擬似コードで表現するとこんな感じです。

FUNCTION F_ITEM_NAME(CODE_TEXT)
  CODES = SPLIT(CODE_TEXT, ',');
  RET = '';
  NAME = '';
  FOR CODE IN CODES
    SELECT NAME INTO NAME FROM SHOHIN WHERE CODE = CODE;
    RET += NAME + ',';
  END
  RETURN RET;
END

売上げ一覧を表示する場合はこのストアドファンクションを何十回も実行することになるので、それはそれは遅～～～～いプログラムになっていました。

SELECT
  CODE,
  NAME,
  -- 100件データがあれば、100回ファンクションが呼ばれる
  F_ITEM_NAME(ITEM_CODE) AS ITEM_NAME,
  -- しかも他にも同じようなファンクションが呼ばれてたりする・・・
  F_ITEM_KUBUN_NAME(KUBUN) AS ITEM_KUBUN,
  F_ITEM_SIZE(SIZE_CODE) AS ITEM_SIZE
FROM
  ORDERS
WHERE
  ORDER_DT > '2016-07-01'

解決策

SQLアンチパターンを読みましょう！（ジェイウォークというアンチパターンで載っています）
そしてテーブルの正規化についてしっかり学びましょう！！

SQLアンチパターン

作者: Bill Karwin,和田卓人,和田省二,児島修
出版社/メーカー: オライリージャパン
発売日: 2013/01/26
メディア: 大型本
購入: 9人クリック: 698回
この商品を含むブログ (46件) を見る

あわせて読みたい

なお、この件に関する詳しい話は以下のエントリに載っています。

Ruby on Railsでもserializeを乱用すると同じような状況が発生するので注意してください。

エラーを握りつぶして無視するシステム

エラーが発生したら「うるさい、黙れ！」と握りつぶして、何事もなかったように処理を再開するシステムも見たことがあります。
たとえばこんな感じです。

public int getDataTable(string id) 
{
  try {
    DataTable dataTable = new DataTable(); 
    // (データベースにアクセスするコードが入る)
    //処理結果は戻り値じゃなくてインスタンス変数に格納!?
    this.table = dataTable;
    return 1; 
  }
  catch (Exception e) {
    //エラーが起きたら全部握りつぶすよ！
    //でも一応メッセージだけインスタンス変数に保存しておくね♪
    this.errMsg = e.Message; 
    return 0; 
  }
}

//戻り値が1であろうと0であろうと100であろうと、確認されることなく処理が進む（涙）
int val = someObj.getDataTable(id);

//エラーが起きたので、tableはnullになる・・・
DataTable table = someObj.table; 

//tableがnullなので当然例外発生！
//しかし、元の例外の情報はどこにも保存されておらず、真相は闇の中・・・
string name = table.Rows[0][0].ToString();

「なんか動きがおかしい」
「なんかデータの整合性がとれてない」
「ユーザーから問い合わせが来てるけど、ログを見ても特にエラーが出てない」

・・・みたいな現象が起きるのでコードをじっくり追っかけていくと、エラーが握りつぶされてた！！ということが何度かありました。

解決策

エラー処理のベストプラクティスを学習しましょう！
僕は.NET時代に以下のWeb記事を読んで勉強しました。

https://blogs.msdn.microsoft.com/nakama/2008/12/29/net-part-1/

この考え方はRailsでもほぼ応用が利くので、Railsでの実践例を以下の記事にまとめています。

あわせて読みたい

ちなみに先ほどのコードは例外を握りつぶしている点のみならず、「戻り値で処理の成功や失敗を表そうとしている点」や、「処理の途中で必要になるデータを、グローバル変数であるかのごとくインスタンス変数に格納している点」もよろしくないです。
「処理の途中で必要になるデータを、グローバル変数であるかのごとくインスタンス変数に格納するな！！」という話は以下のQiita記事にまとめています。

（悪い話ではなく、良い話）最新のフレームワークやオブジェクト指向プログラミングを活用したシステム

ここまではひどいコードの話ばかりしてきましたが、ひどいコードをたくさん見てきてるからこそ、良いコードや良いフレームワークの素晴らしさを知ったときの感動がいっそう際立ちます。

僕が最初に担当したのはCOBOLチックなVBプログラムだと書きましたが、入社して1年ぐらいした後に参加したJava案件が僕にとって素晴らしいものでした。
この案件は当時最新だったStruts(Webフレームワーク)やHibernate(O/Rマッピングツール)といったフレームワークを使った案件でした。

STRUTS・イン・アクション

作者: テッド・ハスティッド
出版社/メーカー: ソフトバンククリエイティブ
発売日: 2003/12/10
メディア: 単行本
購入: 1人クリック: 24回
この商品を含むブログ (28件) を見る

HIBERNATE インアクション

作者: Christain Bauer,Gavin Ki,倉橋央,勝嶌和彦
出版社/メーカー: ソフトバンククリエイティブ
発売日: 2005/12/28
メディア: 大型本
購入: 3人クリック: 109回
この商品を含むブログ (35件) を見る

「1箇所変更すれば、全画面が変わる」の衝撃

この案件はそれまでのVBプログラムに比べると、作りやすさに天と地ほどの差がありました。
それまでは「プログラムはコピペして作れ」と言われていて（ひどい）、ちょっとした仕様変更する場合でもコピペした部分を全部探して、一個ずつ変更、変更、変更・・・を繰り返していました。

しかし、このJava案件ではオブジェクト指向を活用したプログラムの再利用性や、重複を許さないDRYの考え方が導入されていて、コードを1箇所変えるだけですべての画面にその変更点が適用されました。
これは当時の僕にとって本当に衝撃的でした。

また、O/Rマッピングツールを使うと簡単にデータベースとデータの出し入れができるのも非常に便利でした。

「今ドキのアプリケーション開発」を知らなかった僕は、旧石器時代から急に現代にタイムスリップした原始人のような状態でした。

オブジェクト指向プログラミングが便利な例（テンプレートメソッドパターン）

当時のJava案件で「オブジェクト指向ってすごく便利！！」と思った例のひとつが、デザインパターンの「テンプレートメソッドパターン」を活用した仕組みです。
テンプレートメソッドパターンを使うと、「SQLを実行して、その結果をCSVに出力する」という基本ロジックは再利用しつつ、「実行するSQL」や「出力データの加工」など、出力対象によって異なる処理をサブクラスで定義することができます。

残念ながらJavaは何年も書いていないので当時のコードをそのまま再現することはできません。
ここでは僕が今一番得意なRubyのコードで同じようなコードを書いてみます。

# CSV出力の共通処理を定義した親クラス（抽象クラス）
class CsvGenerator
  # CSV出力の共通処理
  def generate_csv(file_name)
    output_path = Rails.root.join('tmp', file_name)
    File.open(output_path, "w") do |file|
      # sqlはサブクラスのsqlメソッドから取得する
      records = ActiveRecord::Base.connection.execute(sql)
      records.each do |row|
        # 必要に応じて出力データを加工する
        cols = process_row(row)
        file.puts(cols.join(','))
      end
    end
  end

  # デフォルトは単純にSQLの実行結果をそのまま出力する
  def process_row(row)
    row.values
  end
end

# ユーザーCSVを出力するサブクラス
class UserCsvGenerator < CsvGenerator
  # ユーザーCSV用のSQL
  def sql
    'SELECT id, name, email FROM users ORDER BY id'
  end

  # 出力データを加工する
  def process_row(row)
    cols = []
    cols << row['id']
    # 名前は"Junichi Ito (foo@example.com)"のように出力する
    cols << "#{row['name']}(#{row['email']})"
    cols
  end
end

# ブログポストCSVを出力するサブクラス
class PostCsvGenerator < CsvGenerator
  # ブログポストCSV用のSQL
  def sql
    'SELECT id, title, category FROM posts ORDER BY created_at'
  end

  # 出力データは加工しない（デフォルトの実装を使う）
end

# ユーザーCSVを出力する
generator = UserCsvGenerator.new
generator.generate_csv('users.csv')

# ブログポストCSVを出力する
generator = PostCsvGenerator.new
generator.generate_csv('posts.csv')