過去のコードを見て進歩を実感する

天気がいい土曜の午前です。 我が家のワンコも気持ちよさげに寝ています。

つい添い寝したくなりますが、実行したら露骨に迷惑そうなカオして去っていくので、我慢なあてくしです。

こんちくわ｜ω・)ﾉ

さて本日の記事ですが、昔書いたコードを見たらあまりのできていなさっぷりにビックリした！　なお話です。

でわいきます￣▽￣)ノ

目次

1. 事の経緯と話題のコード
2. 過去のコードを見て思ったこと
3. このコードを書いた過去の自分を振り返る
4. まとめ
5. 因みに・・

事の経緯と話題のコード

管理人はExcelを使った実験データの解析にVBAを利用しています。

それで、2年前に作って使っていたデータ集計用ツールを同僚に提供することになったため、いま一つ使いにくかった操作を修正しようとコードを見たのが事の始まりです。

まずは惜しげもなくその全貌を全宇宙に、しかもノーカットで晒します。

Private Sub Worksheet_BeforeRightClick(ByVal Target As Range, Cancel As Boolean)
  P = Selection.Column
  r = Selection.Row
  PAdd = Selection.Address
  c = 3
  ws = ActiveSheet.Name
  Application.CommandBars("Cell").Enabled = False
  
  With Sheets(ws)
    Select Case r
      Case Is = 9
        N = Application.WorksheetFunction.CountA(.Rows("9:11"))
        CN = N - 4
        .Range(PAdd) = "O"
        If .Range(PAdd).Offset(0, -2) <> "O" Then
          If CN > 1 Then
            .Cells(12, .Range(PAdd).Column) = "O"
          End If
        End If
      
      Case Is = 10
        N = Application.WorksheetFunction.CountA(.Rows("9:11"))
        CN = N - 4
        .Range(PAdd) = "O"
        If .Range(PAdd).Offset(0, -2) <> "O" Then
          If CN > 1 Then
            .Cells(12, .Range(PAdd).Column) = "O"
          End If
        End If
      
      Case Is = 11
        N = Application.WorksheetFunction.CountA(.Rows("9:11"))
        CN = N - 4
        .Range(PAdd) = "O"
        If .Range(PAdd).Offset(0, -2) <> "O" Then
          If CN > 1 Then
            .Cells(12, .Range(PAdd).Column) = "O"
          End If
        End If
      
      Case Is = 22
        N = Application.WorksheetFunction.CountA(.Rows("22:24"))
        CN = N - 4
        .Range(PAdd) = "O"
        If .Range(PAdd).Offset(0, -2) <> "O" Then
          If CN > 1 Then
            .Cells(25, .Range(PAdd).Column) = "O"
          End If
        End If
                
      Case Is = 23
        N = Application.WorksheetFunction.CountA(.Rows("22:24"))
        CN = N - 4
        .Range(PAdd) = "O"
        If .Range(PAdd).Offset(0, -2) <> "O" Then
          If CN > 1 Then
            .Cells(25, .Range(PAdd).Column) = "O"
          End If
        End If
                
      Case Is = 24
        N = Application.WorksheetFunction.CountA(.Rows("22:24"))
        CN = N - 4
        .Range(PAdd) = "O"
        If .Range(PAdd).Offset(0, -2) <> "O" Then
          If CN > 1 Then
            .Cells(25, .Range(PAdd).Column) = "O"
          End If
        End If
            
      Case Else
      Application.CommandBars("Cell").Enabled = True
    
    End Select
    
    EN = Application.WorksheetFunction.CountA(.Rows("9:11")) - 4
    AN = Application.WorksheetFunction.CountA(.Rows("12:12")) - 1
    .Range("C15") = EN
    .Range("C16") = AN
    .Range("C17") = "=(C16/(C15-2))*100"
    
    EN = Application.WorksheetFunction.CountA(.Rows("22:24")) - 4
    AN = Application.WorksheetFunction.CountA(.Rows("25:25")) - 1
    .Range("C28") = EN
    .Range("C29") = AN
    .Range("C30") = "=(C29/(C28-2))*100"
  
  End With
End Sub

右クリックイベントを利用してワークシートにチェックを入れるコードなのですが、コレをみた管理人は思わず目をパチパチしてしまいましたw

過去のコードを見て思ったこと

で、これを見た瞬間の感想は、
同じコードの塊が何個も見える・・・

気が付いた方もおられるかと思いますが、

N = Application.WorksheetFunction.CountA(.Rows("9:11"))
CN = N - 4
.Range(PAdd) = "O"
If .Range(PAdd).Offset(0, -2) <> "O" Then
    If CN > 1 Then
        .Cells(12, .Range(PAdd).Column) = "O"
    End If
End If

の塊が山盛りありますね。

また、

EN = Application.WorksheetFunction.CountA(.Rows("9:11")) - 4
AN = Application.WorksheetFunction.CountA(.Rows("12:12")) - 1
.Range("C15") = EN
.Range("C16") = AN
.Range("C17") = "=(C16/(C15-2))*100"

も、なんで2回も書いているのか。。

次に思ったことは、
変数名がテキトーで何を示しているのかわからない。

ロジックなどはすでに忘却の彼方にある2年前に書いたコード。。
変数が何を意味してどう使われているのかを理解するのに時間がかかってしまいました。

さらに、びっくりしたのは
変数宣言がない

これはちょっと修正してテストランしたときにVBEが、

と叫んできて初めて気が付いたのですが、

変数はなーんにもいぢってなかったので、想定外のエラー過ぎて、数秒間固まってしまいましたｗ

その他にも、

• 無駄に変数が多い
• シートモジュールにコードを書いているのにワークシート名を指定している
• イベントプロシージャ内でデフォルトでTargetとして宣言されている変数を使っていない
• 右クリックイベントメニュー発生防止の用のコマンドを使わず、わざわざApplication.CommandBars("Cell").Enabled = False なコマンドを書いている

など、コードの書き方の下手っぷりが目に余りましたw

このコードを書いた過去の自分を振り返る

VBAのプログラミング自体はかなり長いことやっていますが、きちんと体系的に勉強し始めたのは去年の夏ごろでした。

それまではとりあえず動けばいい 俺様コード を量産していたわけですが、特にコードの読みやすさなんかは全く意識したことがありませんでした。

改めて勉強をしていく中で読みやすくシンプルに書くことが最も大事なことに気が付いて、それまではやってこなかった共通処理の分割や、型を意識した変数宣言、命名などを意識してやるようになりました。

その結果、変数宣言の強制(Option Explicit)やプロシージャ内の繰り返し処理の共通化、それを独立した機能単位を外に切り出して、メインプロシージャを読みやすくするすことなどがこの１年ほどで習慣化されてきました。

なので今回紹介したコードでは同じ処理のコードセットが必要な数だけ書かれているところが最初に目について、うひゃ! なんじゃこりゃ～ になりました。

変数宣言に至っては、現在はOption Explicitがデフォルトで記述されるようVBEを設定しているため、変数宣言せずにコードが動くことはまずありえません。それだけに、それまでワークしていたハズのコードが変数宣言が理由でエラーになるということが理解できず一瞬思考が停止するという事態に陥るということになったのです。

またこの１年でイベントプロシージャの理解もかなり進みました。当時のコードではプロシージャの引数にわざわざデフォルトで準備されているTargetを使っていないのを見るとイベントコードをきちんと理解していなかったのがまるわかりですね(当時はイベントプロシージャの引数なんて民間人には理解不能な聖域だと思っていましたｗ)。

これまで学習してきた経緯を考えると、当時はまだ知らないこともたくさんあって、コードの読みやすさなど全く意識していなかったんだから、無理もないのかなと思います。ただ、当時の知識レベルを最大限に活用して書いて結構満足していたものが、今見ると甚だ残念な出来上がりだったというのはなんだか赤面しそうですね。

まとめ

今回はたまたま過去に書いたコードを見なおす機会があったわけですが、当時と今ではあまりにも書き方に違いがありすぎて、誰が書いたんだ？いや俺じゃねぇ位のギャップを感じました。一方でたった２年前だけれども、今では当たり前のようにできていることを当時は全くできていなかったんだなぁと、過去の自分を具体的に振り返ることができて新鮮でした。

コードは今の俺流に修正されましたが、２年後に見直したらまた、 "誰が書いたんだ？いや俺じゃねぇ" ってことになっているかもしれませんね。まぁ、それも貴重な成長記録ってことでｗ

このように、過去に書いたコードをロジックを変えずに見直すことをリファクタリングと言うそうです(3か月くらい前に初めて知った単語ｗ)。皆さんのお手元にも黒歴史的コードがある(ハズ)かと思いますが、一度勇気を出して見直してみるのもいいと思います。自分の成長が手に取るように分かって良いと思います。

因みに

今回のコードは以下のように修正されました。

Option Explicit
'----------------------------------------------------------'
Private Sub Worksheet_BeforeRightClick(ByVal Target As Range, Cancel As Boolean)

  Select Case Target.Row
    Case 9 To 11
      Call EntryCheck(Target, Range("C9:BM11"))
    Case 22 To 24
      Call EntryCheck(Target, Range("C22:BM24"))
    Case Else
  End Select
  Cancel = True
  
End Sub

'----------------------------------------------------------'
Sub EntryCheck(ByVal Target As Range, ByVal recordArea As Range)

  Dim entryNumber As Long
  entryNumber = Application.WorksheetFunction.CountA(recordArea)
  
  Dim entryCheckArea As Range
  Set entryCheckArea = Intersect(recordArea, Columns(Target.Column))
  
  Dim alterationCheck As Range
  Set alterationCheck = entryCheckArea.Cells(3).Offset(1, 0)

  With Target
    .Value = "O"
    If .Offset(0, -2) <> "O" Then
      If entryNumber > 1 Then
        Cells(alterationCheck.Row, .Column) = "O"
      End If
    End If
  End With
  
End Sub

かなーりシンプルになりましたねｗ
　

さて、2年後の管理人がどう思うのか、楽しみですねｗ

でわまた～￣▽￣)ﾉｼ