質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

90.33%

  • VBA

    1906questions

    VBAはオブジェクト指向プログラミング言語のひとつで、マクロを作成によりExcelなどのOffice業務を自動化することができます。

見やすい モジュール,プロシージャ の書き方

解決済

回答 4

投稿 編集

  • 評価
  • クリップ 1
  • VIEW 450

kamikazelight

score 126

 見やすい モジュール,プロシージャ の書き方

今まで人に自分のコードを見てもらったことが無かったので
書き方がその時の気分によって変わっていました。

見やすくするために自分なりに条件を決めてみました。
悪いところ等の指摘をお願い致します。

 決めてみた条件

  • 変数の宣言を強制Option Explicitを入れ一行空ける
  • 先ず有れば条件付きコンパイルを書く
  • PrivateよりもPublicの方が上」の様にスコープが広いものほど上に書く
  • 定数より変数を上に書く
  • 宣言は同じ型が隣り合うように、またカウンタ変数など役割が同じものは隣り合うようにする。
  • 宣言の後は一行空ける
  • プロシージャの前後は3行空ける
  • プロシージャ内の最初の行に処理の簡易説明、その次に 返値と引数の補足説明
  • 宣言は インデントは1 
  • 処理は インデントは2
  • ループ等の始まりと終わりがセットになっているものは その中に書く処理はインデントを一つ増やす
  • 但し配列処理などの多重ループはあえてインデントをそろえる
  • 処理のグループはインデント数が同じ文章と隣り合う場合は一行空ける

条件に沿って過去に書いたコードを手直ししてみました。

Option Explicit

'配布する時には以下のフラグをFalseにする
#Const Coding = True
#If Coding Then
    Private FSO As FileSystemObject
    Private TextStream_ As TextStream
#Else
    Private FSO As Object
    Private TextStream_ As Object
#End If
#If VBA7 And Win64 Then '64ビット版
    Private Declare PtrSafe Function QueryPerformanceFrequency Lib "kernel32" (frequency As Double) As Long
    Private Declare PtrSafe Function QueryPerformanceCounter Lib "kernel32" (procTime As Double) As Long
#Else '32ビット版
    private Declare Function QueryPerformanceFrequency Lib "kernel32" (frequency As Double) As Long
    private Declare Function QueryPerformanceCounter Lib "kernel32" (procTime As Double) As Long
#End If

Private PNo As Long
Private Indent As Long
Private StartTime As Double
Private ErrorFlag As Boolean
Private FIlename As String
Private Const IndentWide As Long = 2



Function Redims(ByVal Ar As Variant, Optional ByVal Vertical As Long = -1, Optional ByVal Horizontal As Long = -1) As Variant
'二次元配列のリサイズを行う
'返値:二次元配列
'    Base は 0
'引数1: Ar = 二次元配列
'    Base は 0 or 1
'引数2: Vertical = 縦の要素数
'    Base 0基準
'引数3: Horizontal = 縦の要素数
'    Base 0基準

    Dim min As Long
    Dim VC As Long
    Dim HC As Long
    Dim VCF As Boolean
    Dim X As Long
    Dim Y As Long
    Dim Tray As Variant

On Error GoTo Err
        '元の配列サイズ
        min = LBound(Ar)
        VC = UBound(Ar)
        HC = UBound(Ar, 2)

        '変更後の配列サイズが指定されていなければ保持
        If Horizontal = -1 Then
            Horizontal = HC - min
        End If
        If Vertical = -1 Then
            Vertical = VC - min
        End If

        '配列の移し替え
        ReDim Tray(Vertical, Horizontal)
        Do While (Y <= Vertical And Y + min <= VC)
        Do While (X <= Horizontal And X + min <= HC)
            Tray(Y, X) = Ar(Y + min, X + min)
            X = X + 1
        Loop
            X = 0
            Y = Y + 1
        Loop

        '返値の指定
        Redims = Tray
Err:
End Function



Function Extract(ByVal Ar As Variant, ByVal Index As Long, Optional ByVal Vertical As Boolean) As Variant
'二次元配列から任意の一行(列)を一次元配列で抜き出す
'返値:一次元配列
'    Base は 0
'引数1: Ar = 二次元配列
'    Base は 0 or 1
'引数2: Index = 抜き出す行又は列
'    Ar の Base 基準
'引数3: Vertical = 抜き出す方向
'    True:縦方向
'    False:横方向

    Dim Ans As Variant
    Dim Str As String

        With WorksheetFunction
            '列を書き出す場合は配列の縦横を反転
            If Vertical Then
                Ar = .Transpose(Ar)
            End If

            '指定された行をタブ区切りの文字列として格納
            Str = Join(.Index(Ar, Index, 0), vbTab)
        End With

        'タブ区切りの文字列を一次配列に変換
        Ans = Split(Str, vbTab)

        '返値の指定
        Extract = Ans
End Function

具体的な処理の内容は置いておいて
見やすさ はどうでしょうか?
ご教授をお願い致します。

 蛇足 ~見やすいコードが書きたい~

最近 作る内容が複雑になってきて過去に作ったコードを再利用出来る様に
したいと思い始めました。
現状 コードがぐちゃぐちゃで探すよりも書き直した方が早いという感じで
再利用が出来ません。
書籍を買って読んだりしていますが
知識が増える度に選択肢の多さに迷い
手が止まります。
最終目標は判断基準の確立と
コードの使いまわしが出来るようにすることです。
宜しくお願い致します。

  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

質問への追記・修正、ベストアンサー選択の依頼

  • mts10806

    2018/07/19 14:47

    (後で見やすいコードの書き方が分からない_2) は不要かと思います。タイトルは要件のみを記載してください。

    キャンセル

  • kamikazelight

    2018/07/19 15:26

    失礼致しました。

    キャンセル

  • mts10806

    2018/07/19 15:28

    質問は編集できるので指摘があった場合は適宜対応してください。

    キャンセル

  • kamikazelight

    2018/07/19 15:49

    すみませんでした。タイトルも修正できることを初めて知りました。気を付けます。

    キャンセル

回答 4

checkベストアンサー

+3

いい質問だね。

折角だから、自分のマイルールを少しばかり。

・Option Private Module を Option Explicit の下に追記する
※ユーザ側から関数を隠す

・関数の説明
中でも外でもどちらでもメリデメがあると思います。
 
中に書く場合は、VBAでコードの説明を抜き出す時に便利
(モジュール内の最初の関数の説明を抜き出すのが面倒)
また、1関数だけ表示する「プロシージャの表示」にしても説明が見れる
(外に書いた場合は説明が見えない)

ただ、往々にして、説明が更新されずに処理が変わる、という事が起こりやすいので、
説明を書くよりも、コードを分かりやすく書く方が重要だと思います。
分かりやすく書くという事は、関数名、変数名、処理の流れ等を意識するという事ですね。

・変数の宣言位置
最初は関数の頭に書くように教えられたけど、
正直、コーディングしにくい。。。
しかも、他の言語では直前に宣言する事が主流だと聞く。
なので、自分も必要になった時に、近場の分かりやすい位置に書くようにしてます。

・繰り返し
どうしても Do-Loop じゃなきゃって時意外は、For-Next がメインかな。
ていうか、インクリメントを自分で書くという事は、バグの可能性が増えるということで。。。
そして、Do-Loop は無限ループの可能性があるから、敬遠する気持ちが必要かと。。。
 
あと、ループx2の時にインデント揃えるのは微妙な気がする。。。

・ネーミングセンス
これからビシバシ鍛えた方がいいと思うよ。
Ar なんていう変数名、何だと思えばいいのか。。。
共通関数ならなおさら。
 
他にも2文字を多用してるけど、なぞなぞになっちゃうのでは? 
 
Vertical って変数名で Boolean型 にしてるところは、
自分は Direction って変数名で xlRowCol列挙体 を使うかな~
というのも、エクセルではシートの行・列操作が多いから、
必然的に配列も1,2次元配列で十分なんだよね。
だから、配列の次元数は、基本的に2次元以内という前提で共通関数を作ってるよ。

・関数名
色々なんだよね。まぁ現場に合わせるのが原則だけど。
ただ、自分ルールでは、共通関数は fArray_Trans とか、
頭にf、その後にArray とか String とか Range とかメインとなる名称、
で、その後ろに機能を付けていくようにしてます。
 
非常に多くの共通関数を作っていくうちに、こうなったんだけど、
理由としては、自作関数だと見て分かりやすい事と、
f〇〇でインテリセンスを出せば、名前を憶えてなくてもリストから見て探せる事。
 
※Array用の共通関数は、それ用の1モジュールにまとめてたりします(M_Array モジュール等)
 
※本当はクラス化とかした方がいいのかもだけど、VBAの世界でクラス化を進め過ぎると、
ついてこれない人がいるかもだし、移植しやすいので、今はこれで良しとしてます。
 
※要望としては既存のオブジェクトの機能を上書きしたいんだけど。。。VBAではできないから残念。。。

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2018/07/19 18:25 編集

    Option Private Module 初めて知りました。
    クラスにするかプライベートにしないと
    隠せないと思っていました。
    必ず入れるようにします。

    xlRowColは調べて始めて知りましたが
    自動補完が利かないので
    忘れた時にどんな値を指定していいのかわからなくなりそうですね...

    また 宜しくお願いします

    キャンセル

  • 2018/07/20 09:25

    自動補完って、インテリセンスのことかな?
    Excelなら、xlRows か xlColumns を選べるはずなんだけど。。。

    キャンセル

  • 2018/07/20 13:24

    出てこないですね
    Sub test()
    Dim RowCol As XlRowCol
    RowCol.
    End Sub

    VBEに入力してピリオドまで打った状態でCtrl+Space 押しても
    候補が出てきません。

    ないとは思いますが環境の違いでしょうか?
    こちらの環境
    Excel 2016 64bit

    キャンセル

  • 2018/07/20 13:59

    えと、、、列挙体を使ったことがなかったのかな?
    自分で Enum を作っても同じ事になるんだけど、
    この場合だと、「RowCol = 」の状態で候補が出てくるはず。

    どう伝えるのが正しいか分からないけど、
    Boolean型 は True/False が候補に出てくるように、
    XlRowCol型 は xlRows/xlColumns が候補になる、という感じ。

    キャンセル

  • 2018/07/20 16:07

    ありがとうございます...
    列挙体は一年以上前に「へーこんな機能があるんだー」っと使ったきりで
    完全に忘れてました。
    思い出しました...

    キャンセル

  • 2018/07/20 17:12

    僕はよく使ってるよ~
    特に、シートの行や列を定義するのに超便利!
    ↓の fRange_Value と Enum の組合せは最強だと思ってるので、お試しあれ~
    http://www.excelsystem.jp/Library/archives/3

    キャンセル

+3

前回も書きましたが、書いてる人が見やすければそれでよしです。
但し、条件のうち以下の4つはあまり一般的ではないと思われます。

プロシージャの前後は3行空ける
プロシージャ内の最初の行に処理の簡易説明、その次に 返値と引数の補足説明

これは2つセットで考えますが、関数の説明は一般的に外に置きます。
そうすることで関数ごとの区切りがわかりやすくなります。
またそうすることで「3行空ける」などの縛りも必要なくなります。
関数の説明文もなるべく簡潔に書いたほうが良いでしょう。

処理は インデントは2

無用なインデントは避けるべきです。
基本的に関数内は変数宣言以降処理しかないはずですから(途中で変数宣言をする場合を除く)そうすると無駄にインデントが空いた状態になります。
プログラムは右へ行くほど見づらくなりますので、必要のないインデントは行わないべきです。

但し配列処理などの多重ループはあえてインデントをそろえる

ループの開始と終わりが簡単に認識できるようにするためにもこれは行わないべきです。

VBAの場合、自動的にコード整形が行われるので、それにそぐわない書き方をすると生産が落ちる可能性があります。
自動でここから書きなさいとインデントしてくれているのに、更にインデントを加えたり、また逆に減らしたりといったことです。

というのを踏まえて前回同様私なりに修正したコードを貼っておきます。
(関数の説明は実際にそろっているのですが、この解答欄ではずれてしまいます)

Option Explicit

'配布する時には以下のフラグをFalseにする
#Const Coding = True

#If Coding Then
    Private FSO As FileSystemObject
    Private TextStream_ As TextStream
#Else
    Private FSO As Object
    Private TextStream_ As Object
#End If
#If VBA7 And Win64 Then '64ビット版
    Private Declare PtrSafe Function QueryPerformanceFrequency Lib "kernel32" (frequency As Double) As Long
    Private Declare PtrSafe Function QueryPerformanceCounter Lib "kernel32" (procTime As Double) As Long
#Else '32ビット版
    private Declare Function QueryPerformanceFrequency Lib "kernel32" (frequency As Double) As Long
    private Declare Function QueryPerformanceCounter Lib "kernel32" (procTime As Double) As Long
#End If

Private PNo As Long
Private Indent As Long
Private StartTime As Double
Private ErrorFlag As Boolean
Private FIlename As String
Private Const IndentWide As Long = 2

'二次元配列のリサイズを行う
'    返値        : 二次元配列 Base は 0
'    Ar        : 二次元配列 Base は 0 or 1
'    Vertical    : 縦の要素数 Base 0基準
'    Horizontal    : 縦の要素数 Base 0基準
Function Redims(ByVal Ar As Variant, Optional ByVal Vertical As Long = -1, Optional ByVal Horizontal As Long = -1) As Variant

    Dim min As Long
    Dim VC As Long
    Dim HC As Long
    Dim VCF As Boolean
    Dim X As Long
    Dim Y As Long
    Dim Tray As Variant

    On Error GoTo Err

    '元の配列サイズ
    min = LBound(Ar)
    VC = UBound(Ar)
    HC = UBound(Ar, 2)

    '変更後の配列サイズが指定されていなければ保持
    If Horizontal = -1 Then
        Horizontal = HC - min
    End If
    If Vertical = -1 Then
        Vertical = VC - min
    End If

    '配列の移し替え
    ReDim Tray(Vertical, Horizontal)
    Do While (Y <= Vertical And Y + min <= VC)
        Do While (X <= Horizontal And X + min <= HC)
            Tray(Y, X) = Ar(Y + min, X + min)
            X = X + 1
        Loop
        X = 0
        Y = Y + 1
    Loop

    '返値の指定
    Redims = Tray
Err:
End Function

'二次元配列から任意の一行(列)を一次元配列で抜き出す
'    返値    : 一次元配列 Base は 0
'    Ar    : 二次元配列 Base は 0 or 1
'    Index    : 抜き出す行又は列 Ar の Base 基準
'    Vertical: 抜き出す方向 True=縦方向 False=横方向
Function Extract(ByVal Ar As Variant, ByVal Index As Long, Optional ByVal Vertical As Boolean) As Variant

    Dim Ans As Variant
    Dim Str As String

    With WorksheetFunction
        '列を書き出す場合は配列の縦横を反転
        If Vertical Then
            Ar = .Transpose(Ar)
        End If

        '指定された行をタブ区切りの文字列として格納
        Str = Join(.Index(Ar, Index, 0), vbTab)
    End With

    'タブ区切りの文字列を一次配列に変換
    Ans = Split(Str, vbTab)

    '返値の指定
    Extract = Ans
End Function

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2018/07/19 16:01

    On Errorのインデントですが
    もしも 後ろで On Error Goto 0 等を
    入れる場合はエラー処理対象の始まりと
    終わりの境目が見にくくなる気がしますが、
    気にしない感じでしょうか
    それともOn Errorインデントを減らしたり、後の処理のインデントを増やしたりするのでしょうか

    Do Whileですが
    もし、三次配列ループ内でIfを使うと
    かなり階層が深くなりますが
    そういう場合は気にしない感じでしょうか?
    それとも、別関数として書き出したりするのでしょうか。
    教えて下さい。

    キャンセル

  • 2018/07/19 16:15

    On Errorのインデントについては私もどちらがよいと言えないですね。
    私も昔は1文字目から書いてましたけど、最近は特に気にしていないです(というかまじめにVBAのコードは書いていない)。

    一般的に(明確な理由もなく)ネストが3重にもなるのは何か設計上の問題があると思います。
    三次元配列のような明確な理由がある場合は仕方ないでしょう。
    但し最深の処理でIfしてSelectしてみたいに更なるネストを生むようであれば、関数化するなりしたほうがよいかもしれません。
    但し安易に関数化すると可読性が落ちる可能性もあるので、実際のコードを見て判断するしかないです。

    キャンセル

  • 2018/07/19 16:21

    さじ加減が難しいですね...
    頑張ります。

    キャンセル

+1

コードを見て気になる点

・Functionの説明がFunction内に記述されている。Function定義の前に移動した方がいいと思います。

・2つのDo Whileのインデントが同じ位置なのが気になります。2つ目のDo Whileはインデントするべきでは?

・Function ExtractのDimのインデントとその後のWith以降のインデントが違和感あります。DimとWithは同じ位置がいいと思います。

・On Error GoTo Errの位置も上のDimと合わせるかな。

・'変更後の配列サイズが指定されていなければ保持の下のIF~End Ifと次のIf~End Ifの間は1行空けますね私なら

上記はあくまで私の私見なので自分が見やすいのが一番だと思います。

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2018/07/19 15:47

    試しに直してみたら見やすくなりました。

    ただ、
    あんまりないですが Withのネストをするときは
    内側のWithもDimのラインでいいでしょうか

    Do Whileですが
    三次配列以上(三次配列はちょくちょくありますが、四次配列以降は使ったことないです。)の場合
    インデント数が多くなってしまうので
    ・ループ等の始まりと終わりがセットになっているものは その中に書く処理はインデントを一つ増やす
    ・但し配列処理などの多重ループはあえてインデントをそろえる
    としていました。
    もし、三次配列ループ内でIfを使うと
    かなり階層が深くなりますが
    そういう場合は気にしない感じでしょうか?
    それとも、別関数として書き出したりするのでしょうか。
    教えて下さい。

    キャンセル

  • 2018/07/19 16:01

    ネストする場合はインデントしますね。
    階層が深くなる場合は切り出したりします。ただ、そこでしか使わない場合はそのまま記述します。
    見やすくなるからといって切り出すと逆に追いかけにくくなるので。
    インデントもTABの場合、エディタ上で変更できれば1TAB = 2SPACEとかで見やすくしたりします。

    キャンセル

  • 2018/07/19 16:05

    なるほど...
    階層が深くなった場合は他でも使える処理なら書き出し、そうでないなら
    インデントの幅を減らして見通しをよくするのですね
    参考になります!!

    キャンセル

0

コードを読んで気になった点。
a. 以下の変数の用途が書いていないのが気になりました。
何のための変数なのでしょうか。

Private PNo As Long
Private Indent As Long
Private StartTime As Double
Private ErrorFlag As Boolean
Private FIlename As String
Private Const IndentWide As Long = 2


b. 以下はWinAPIの宣言ですよね。宣言をPrivateからPublicにして別モジュールにすることをお勧めします。

#If VBA7 And Win64 Then '64ビット版
    Private Declare PtrSafe Function QueryPerformanceFrequency Lib "kernel32" (frequency As Double) As Long
    Private Declare PtrSafe Function QueryPerformanceCounter Lib "kernel32" (procTime As Double) As Long
#Else '32ビット版
    private Declare Function QueryPerformanceFrequency Lib "kernel32" (frequency As Double) As Long
    private Declare Function QueryPerformanceCounter Lib "kernel32" (procTime As Double) As Long
#End If

インデントや見やすさは勿論大事なのですが、
調査のために3ヶ月後に処理を読んだ時に思考が遮らないような
モジュール分けやコメントも意識してみてはどうでしょうか。

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2018/07/19 18:30

    調査のために3ヶ月後に処理を読んだ時に思考が遮らないような を
    実現するために見やすくしたいと思っております
    WinAPIをprivateにしているのは移植の時にAPI定義のしわすれやAPI定義かぶるのを
    嫌った為ですが確かに分けたほうが良さそうな気がしますね。
    考え直してみます

    モジュール分けなども今困ってます...
    別質問で出す予定でいますので
    またよろしくお願い致します

    キャンセル

同じタグがついた質問を見る

  • VBA

    1906questions

    VBAはオブジェクト指向プログラミング言語のひとつで、マクロを作成によりExcelなどのOffice業務を自動化することができます。