Q&A
すみません。泣いていいですか？
#稼働中システムのコードリーディングについて。
現行稼働中のシステムについて、引継ぎ作業を行っているのですが、
既存のソースが大変汚く読み解くのに苦労しています。
該当のコードは以下のような特徴を備えています。
- 仕様書も要件定義書も存在しない。
- 当然他のドキュメントも存在しない。
- どう動くべきなのか知っているのはお客様のみ(またはお客様も分からない)
- 変数宣言が強制されておらず、未宣言の変数だらけ。
- 宣言されているが、未使用の変数が沢山残っている。
- いつコメントアウトされたか分からないコード片が散乱している。
- 一つの関数内に様々な処理が詰め込まれている。
- コードのブロックが表現されておらず、コードとコードの間に1行ずつ空行がある。
- 至る所でexitされている。
- コピペしたであろうコードが大量にある。
- IFやSELECTのネストが深い。(最低3～4ぐらい？)
- インデントがずれている。
- エラーは全て同じメッセージを表示して握りつぶす。
- 至る所でDBへのコネクションを生成している。
- 同じ変数をやたらと使い回す。
- 変数名は2～3文字の省略形
- DBのカラム名はローマ字省略形
- etc...
稼働中のシステムなので具体的なコードは書けませんが、
例えば以下のようなコードです。
VBA
1 2private sub cmd1_Click() 3On Error Goto Err 4 5変数名1 = Function1 6 7IF 変数名1 = -1 then 8 9 変数名2 = コントロール1.Value 10 11 SELECT CASE 変数名2 12 13 CASE -1 14 Goto Err 15 16 CASE 1 17 18 IF 関数名2 = 1 then 19 20 ---何かの処理が数十行書かれている--- 21 22 IF コントロール2.value = 1 23 24 exit sub 25 26 Else 27 28 ---何かの処理--- 29 30 End IF 31 32 IF コントロール3.value = 1 33 変数名4 = コントロール.value 34 SELECT CASE 変数名4 35 36 CASE 1 37 Sub1 38 exit function 39 CASE 2 40 SQL = "DELETE ～～～～～～" 41 42 Docmd.RunSQL SQL 43 44 SQL = "INSERT INTO ～～～～～～" 45 46 Docmd.RunSQL 47 48 Else 49 50 ---何かの処理--- 51 52 End IF 53 54 Else 55 56 ---上の処理のコピペ(一部だけ違う)--- 57 58 Sub1 59 60 exit sub 61 62 End IF 63 64 CASE 2 65 ---何かの処理--- 66 ---以下続く--- 67 68 End Select 69Exit Sub 70Err: 71 MsgBox "エラーが発生しました。" 72 Exit Sub
正直どこから手をつければ良いのかさっぱり分からない状態です。
皆さんはこういったコードを読み解くとき、どのような方法を採用されていますか？
前任者よりも前の人が作ったシステムってことでしょうか？ 前任者はどう管理していたんでしょうか？
多数の開発者の手を経て複雑怪奇なものになっていったのかなぁ？と思いますが...。
まさに秘伝のソース状態になっています。
VBAなのでバージョン管理というかはファイルやフォルダに日付を付けて管理という感じになっております...。
> VBAなのでバージョン管理というかはファイルやフォルダに日付を付けて管理という感じになっております...。
バージョン管理ではなく、運用管理という意味で質問しました。ドキュメントもない中で今まで使用者からの問い合わせや、不具合発生時の対応は一体どうされていたのか？？と。正直、問い合わせや不具合発生もないのであれば様子見するのもありなのではと。。質問者さんが引き継いだ経緯が不明なので無責任な独り言ですが。
なるほど。
運用管理のほうでしたか。
システムの稼働期間自体は長いので、致命的なバグなどはないんじゃないかと思われるのですが、
定期的に数字がおかしいだのなんだのとちょこちょこ問い合わせはある状態です。
その際はお客様に不具合の発生状況などをヒアリングし、該当しそうな箇所のコードを読み解いて回答するような形を取っています。
その中でほぼ全てのコードがこんな感じだったので、皆様どんな風にコードを読んでおられるのかなぁと思いまして、質問させて頂いた状態です。
回答6件
0
ベストアンサー
VBAは殆ど触ったことがないので、言語を問わずどうしても引き継がなきゃいけない場合の話としての回答ですが、自分の場合は以下の様に進めることが多いです。
ポイントとしては、内部仕様の把握は必要になる時(その部分の改修等)までは保留しておいて、各単位の外部仕様の把握に専念する事です。
- 壊しても良い検証環境の構築
とりあえず可能な限り同じ環境を構築する。
データも本番から取得したものを使う。
環境のバックアップを取っていつでも戻せる様にする。(ソースコードレベルでは無く、OSのイメージ化や仮想化で確実に元に戻せる状態を確保する)
好き放題やっていい環境があるのと無いのとではその後の作業負荷に雲泥の差が出るので超重要です。
ソースコードを眺めてなんとかしろと言われちゃった場合は、効率が数十倍変わりますが良いですか?とアラートを出します。
- インデントや体裁が汚いものはIDEを駆使して奇麗にする
機械的に直せるところは直してしまいましょう。
- 内部仕様を読み解くのでは無く、確認出来る単位(ユーザー定義関数や、一連の処理)で外部仕様を確認していく
例えばユーザー定義関数なら、
仕様の仮説を立てる
↓
仮説に沿ったテストケースを作って実行(その関数を使っている部分に固定値でテストデータ無理やり入れるとか)
↓
結果の検証
と言うのを繰り返して、どういった役割と外部仕様を持った部品なのかを把握していきます。
- 結果が仮説と違った場合は、
仮説と結果の差異を確かめる事に目的を絞ってブレークポイントを仕掛ける等して内部仕様から外部仕様を導き出す - 大きな単位（一連の処理）の正常形を最初に確認して（業務要件と直結しているはずなので、実際に使っている人に教えてもらうのが早いことが多いです）、一段ずつ単位を細かくしていく
- 一つ下の単位の部品の外部仕様が把握できて来たら、一段上の単位に戻って異常系についても確認していく（行ったり来たりして挙動を把握する）
- 最初のうちは異常系まで一気に全部把握するのは無理なので、正常系に絞ってそのシステムの全体の流れを把握することに注力し、全体像や作りが把握できて来たら異常系や境界値について仕様を仮定してテストケースを作る
投稿2020/10/01 14:37
編集2020/10/01 15:02
総合スコア18727
ありがとうございます。
一応壊れても良い環境は準備されています。
VBEにはインデントを自動整形する機能がなく、アドインを追加すればなんとかなりそうかと思ったのですが、未定義の変数などを乱発していたり、OnErrorGotoでエラーをねじ伏せていたりするので、そもそもコンパイルすら通らない箇所が多く、ツールには頼れそうにありません...。
ユーザー定義関数については、細切れに分解したいところなのですが、一関数が数百行あったりすることと、フォーム上の入力値検査からSQLの発行・実行、分岐のネストが深い、未定義変数の連発などで、中々進んでいない状態です。
ただとりあえず手が付けられそうな箇所はコード整形と関数の切り出しかな？と感じましたので、少しずつ進めてみたいと思います。
> VBEにはインデントを自動整形する機能がなく、
VBAにも色々ありますが、例えば`VBA 開発 VSCode`で調べられるような形でVSCodeが使えないか検討されるのが良いのかなと思います。
> ユーザー定義関数については、細切れに分解したいところなのですが、一関数が数百行あったりすることと、フォーム上の入力値検査からSQLの発行・実行、分岐のネストが深い、未定義変数の連発などで、中々進んでいない状態です。
これは頭が痛いところだと思いますが、その関数を細切れにするより前に、どうにかして（その部分に関連するところを知っている人にヒアリングするとか、最悪勘でも良い）仮説を立てて、その関数の外部仕様（の一部）を把握することを最優先にするのがお勧めという主旨の回答です。
数万行とかあったら泣くしか無いですが、数百行なら何とか外部仕様の予測（外れたら検証すればいい）は立つんじゃないでしょうか。
> 数万行とかあったら泣くしか無いですが、数百行なら何とか外部仕様の予測（外れたら検証すればいい）は立つんじゃないでしょうか。
と言うのは、例えば、
「このifに入る条件とその後の処理の外部仕様だけは予測する」
という形での切り分けですね。
ありがとうございます。
VSCodeの機能ですね。
探してみます！
なるほど。
分岐のネストが深く処理を抜けるまでに経由するパスが多いので検証が中々難しくはありますが、仮説立てながら進めてみます。
様々な方法、体験談をお聞かせくださいまして、
皆様本当にありがとうございました！
大変勉強になりました。
どの方も親身になってくださり、皆様全員にBAをつけさせて頂きたいぐらいなのですが、
今回はtanat様とさせて頂きました。
また何かありましたら宜しくお願い致します。
0
皆さんはこういったコードを読み解くとき、どのような方法を採用されていますか？
まずは現状の動作を確認します。
但しそれはソースではなく、運用者達へのヒアリングです。
ヒアリングするにしても教えてくださいでは聞く側も教える側も心情的にハードル高いと思うので、ザックリと現状の動作を確認し、さらにザックリとソースを見てQA形式で質問しつつ、不足してたり認識漏れてるところあったら教えて欲しいと頼みます。
情報が集まったら後は汚いコードを整形して動作の仕様書に書き下ろしていき、ヒアリングした内容と差異があれば改めて本来の仕様（要件）を洗い出します。
上記までで出来上がったドキュメントを元に、新規に要件を満たす（可能なら改善）システム開発します。
但しこれはその作業に伴う工数に周囲が納得してくれて開発環境が整えられることが条件です。
投稿2020/10/01 14:42
総合スコア6426
ありがとうございます。
どう動くのが正であるのかをヒアリング等で把握すべきということであっておりますでしょうか。
ただ追記でも書いております通り、多数の開発者の手を経ているため、社内に要件全体を把握している人間が居ない可能性が高いです。
正直リファクタリングなどを施すほどの工数、予算は頂戴できないのではないかと思われます...。
> どう動くのが正であるのかをヒアリング等で把握すべきということであっておりますでしょうか。
そうですね、より正確に言うなら正しい動作というよりかは運用者が理解している動作仕様をを文書化する為のヒアリングです。
> 多数の開発者の手を経ているため
多数の開発者は無視で良いです。
> 社内に要件全体を把握している人間が居ない可能性が高いです。
複数の人が要所要所で機能を使っているシステムなんでしょうか？複数人に聞く事で仕様書（というより要件定義書）が出来上がるなら機能ごとに把握している人にヒアリングするしかありません
それどころか機能を分割出来るなら分割して開発する事で段階を踏んでシステム移行出来るんじゃないでしょうか？
> 正直リファクタリングなどを施すほどの工数、予算は頂戴できないのではないかと思われます...。
そのまま運用する場合の事故発生のリスク・機能改修不可のリスクと、再開発の工数を負うリスクを天秤にかけて検討するしかないですね
ありがとうございます。
まず、言葉足らずで申し訳ありません。
運用者が理解している動作仕様の把握が一番難しいところでありまして。
言語がVBAであるため、一人の担当者が一つの案件を管理している状態になっており、担当者が退職してしまうと、なぜそうなっているのかを誰も知らないという状況になっているようで...。
その状態で引継ぎが発生するため、多数の開発者(退職済)の手を経ているという意味でした。
機能毎に分割して仕様把握を進めてみたいと思います。
> 運用者が理解している動作仕様の把握が一番難しいところでありまして。
誤解があるかもしれないので念のため言うと、ヒアリングでは内部の動作仕様はいらないんです。
運用者はインプットに対する正解のアウトプットの形を知っていると思うのでそれを洗い出すんです。
詳しい内部仕様は、その後のソース調査でやるところです。
> なぜそうなっているのかを誰も知らないという状況になっているようで...。
この意味が、運用者がそれすら知らずに、「ただ出てきたデータを使っている」状態を指すならヒアリングも無理なので回答した手順では進めませんね・・・
お返事ありがとうございます。
処理の出入り口を把握するということですね。
>この意味が、運用者がそれすら知らずに、「ただ出てきたデータを使っている」状態を指すならヒアリングも無理なので回答した手順では進めませんね・・・
ご推察の通り、そんな感じの状態だったりします...。
業務ロジックの殆どがVBAで言うところのSub内にベタ書きされており、Functionは四捨五入などの単純な処理に使われている程度です。
とりあえず現状どう動いているのかを把握する向きで進めてみたいと思います。
0
こんにちは。
典型的なスパゲッティコードですね、心中お察しします。
他の回答者の方々も仰ってますが、自分なら上長の許可を得て作り直ししますね。
もっとも作り直しするか否かについては、
- 稼働システムのライフサイクル
- 改修頻度
- 現在の品質
などを考慮して費用対効果が見込めるかどうかも評価する必要があります。
仕様書・設計書がないということですので、自分ならこんな順序で進めます。
- テスト用環境を構築する。
先ずはこれですね。
外部DB（ORACLE?)等のテスト用環境を用意します。
→既にあると思いますが、、、
0. 入出力仕様を作成する。
画面・ファイル・DBの入出力仕様書を作成します。
→何はともあれ入出力を固める。
0. ビジネスロジックを整理する。
機能分け・通常処理・例外処理・変換規則などを整理します。
これらをまとめ、概略化したものを要件として運用者の方に確認して頂く。
0. 改修（作り直し）する。
詳細設計書はMUSTではないと思います。
構造化なりオブジェクト指向なりのアプローチでコードが書ける方なら、むしろ
VBAコード内のコメントを充実させた方が時間短縮できます。
0. その他
修正履歴は（自分は）残しません。
変更前や削除前のコードを残すとコードの可読性が落ち、見通しが悪くなります。
せいぜい改修等で追加変更した場合、その旨を説明文でコメントを残す程度です。
自分の場合、REDMINEで案件管理しているので、日付とREDMINEのチケット番号程度で
済ませることが多いです。
リリース世代をSubversion等で管理していれば切り戻しも楽です。
以下余談ですが、、、
C言語で１関数が12000ステップ(!!)のプログラムデバッグを頼まれ、その日のうちに
作り直しを提案し、書き直した経験があります。
入出力仕様だけはしっかりしておりなんとか１か月で完成しました。
自分のキャリアでは断トツのワースト１でしたWw
投稿2020/10/02 02:37
総合スコア1095
ありがとうございます。
>入出力仕様を作成する。
やはり入出力の仕様把握を行うべきなのですね。
hentaiman様への返事でも書かせて頂いたのですが、中々入出力の把握が難しそうではあるのですが、
まずはここから着手したいと思います。
>ビジネスロジックを整理する。
これについても、時間はかかりそうですが何とかまとめてみたいと思います。
>改修（作り直し）する。
詳細設計書を作っても多分更新されないので、コード内にコメント埋め込んでいきます。
(それですらメンテされなさそうな予感がしますが...。)
>その他
RedMineについては運用を提案してみたのですが、他の方が乗り気ではなく稼働できませんでした。
とりあえず自分の案件だけでもチケット管理していきたいと思います。
VBAのコードをプロジェクトから吐き出すツールを見つけたので、バージョン管理も徐々に行っていこうと思います。
0
質問者様本当にご苦労様です。
ここで私の想像の話をします
・システムはメインは「Access」DBは「Oracle」
・2000年前より稼働
・接続方式はDAO
・データベースへの接続はリンクテーブル主体だが一部
DAOでOracleへ直接SQLを流し込んでいる。
といった想像を立てました。
この場合一番早いのは書き直す。
まず上司に申し出て、許可をもらいます。
理由は「現在のシステムは限界を迎えこれ以上のシステム変更はできません。
今後変更するなら新しいシステムとするしかなく今のうちから調査する必要があります」とでもして下さい。
(私はこれで許可もらいました)
許可がおりたら、もう一つ環境を整えコードをコピーして書き直します。
その時変数名やインテンド、コメント等を整理します。
あとは根気の問題で私ならすべて書き直します。
でも直したコードは使いません。自分用にとっておきます。
なぜなら元のバグをうっかり直してしまう事があるからです。
この状態のシステムだとバグがお客様かすれば、すでに「仕様」になっていることが多く
直すと「動きが違う！」と逆に怒られます。
きれいに書き直されたコードはそれ自体仕様書と変わりません。
そうすれば現システムの問題点が指摘できるレベルでまとまるでしょう。
そうしたら今度はその事を「新システムづくりの提案」として報告しましょう。
これで質問者様は「新システムづくりのリーダ」になります。
めでたしめでたし。
投稿2020/10/01 13:58
総合スコア2506
ありがとうございます。
ご推察の通りAccessをPGとDBに分けて開発している状態です。
リプレースのためにリファクタリングをするのではなく、自分用に行うことでプログラムの仕様把握を行うということですね。
確かにバグなのか仕様なのか判別できない箇所もそこそこ多く、直して良いものやらと悩んでいたところでした。
コードレビューなどの文化がありませんでして...。
動いてるんだからいいじゃないか！誰がその費用を払うんだ！という雰囲気でして。
残念ながら許可はおりなさそうです...。
これ「動いてるんだからいいじゃないか！」の反論に使えるんですよ。
話的には「質問者様が全部コードを直す」としていますが
直せなくてもいいんです。その証拠が残れば。
(下手に直したりするとトラブルの元ですから注意してください。)
このままですと次の改修で質問者様が引き継げなかった悪者になります。
ただ引き継ごうとした結果（ソース）が残っていれば
「ここまでは行いました、でも全体は無理でした」とできます。
(何もないと言い訳に聞こえる)
その状態なら「一度ソースを外注へ出して内容確認してもらってください」と言えるはずです。
今の状態からすれば新システムはAccessから離れるでしょうからコードも整理されます。
システムに理解のない上司を説得するにはとにかく材料集めが大切です。
とにかく質問者様が担当でなくなるか、新システムだ！と他人任せでコードの修正を変更してもらうよう働きかける方向のほうが得策ですよ
(私はそれで地獄をみています。)
あと書きわすれていましたが「難読ソースはなれる」という事
コード写ししていくとなれますよほんとに。
質問者様が提示れていたレベルですと私の場合「難読ソース」のカテゴリに入らなくなっています。
(大体なにしたいか理解できましたから)
「変数名やインテンド、コメント等」動き自体に影響ないところから整理するのがポイントです。
お返事ありがとうございます。
なるほど...。
引継ぎというか整理というかを行おうとした証拠は残すということですね。
上司も現役で開発をしているのですが、本件のソースの作成者が当該上司のような気配がありまして、
中々大きな声で指摘できないという...。
コードの書き方の特徴？が把握できるようになるんでしょうかね？
動きに関係無いところを整形しながら、徐々に慣れていきたいと思います。
0
現状のものは捨てて、一から作り直した方がよさそう。
稼動中のシステムということなので、
DBが正規化されているなら、DBは現状のものを共有で使って、
少しずつ処理を新規のシステムに移行させていくようにすればどうでしょう。
他の回答者さんから有益なアドバイスが出ていますので、
自動インデントのアドインについてのみ追記しておきます。
下記から Smart Indenter というアドインがダウンロードできます。
Office Automation Ltd. - Smart Indenter
windows10で使用するには下記の対応が必要です。
Smart Indenterをwindows10で利用する(VBA コード整形) : 日経225 トレードメモ出張所
これがあると、
Ctrl+Shift+P でプロシージャ単位の整形
Ctrl+Shift+M でモジュール単位の整形
が簡単にできます。
投稿2020/10/01 13:45
編集2020/10/02 03:01
総合スコア34075
ありがとうございます。
そうできれば万々歳なのですが。
一つの処理であっちこっちのテーブルをDELETEしてみたり、INSERTしてみたりしているので
最悪、正規化とは一体...!?の可能性もあります。
予備カラムも沢山用意されておりますので、そもそもカラムに入ってる値が何なのかも分からない箇所があったりします。
どうあるのが正なのかが分からないため、まず仕様把握頑張ってみます。
気付くのが遅くなってしまい、お礼を申し上げるのが遅くなってしまいました。
申し訳ありません。
インデントツールを教えて頂きありがとうございます。
VBE単体だとインデントを揃えるだけでも中々の作業だなぁと思案しておりましたので、
非常に助かります。
0
時間もお金もあるなら、設計からやり直し。
VBAからも離れていいんじゃないでしょうか。
どっちもないなら、がんばってください。
投稿2020/10/01 13:36
総合スコア80875
ありがとうございます。
時間もお金もなく設計からやり直すためリソースが無い状態です。
VBAにこだわりがあるのか他言語は使用できなさそうです。
頑張ります！
あまり積極的な応援ではないですよ。「好きにしてください」くらいの突き放しです。
結局業務関連するなら他者にはどうにでもできません。
自身の責任でやりきるしか無いのですが、
正直どこから手をつけたものやらという状態でしたので、
経験の豊かな皆様方のお知恵を拝借できればと考え質問した次第です。
お返事ありがとうございました。
あなたの回答
tips
プレビュー
