部屋割りマクロ(Excel)(3)
部屋割りマクロのリファクタリング
このときに作成した部屋割りマクロのコードを見直して修正した。
問題点その１
isFull_変数が無意味
Roomクラスの内部に、インスタンスが定員一杯であることを表すisFull_というPrivate変数を置いていたのだが、全く使用していなかったので、使うようにした。
そこで、
のリスト１のallocateメソッドのところを書き換える。
リスト１ クラスモジュール
Public Function allocate() As Boolean If isFull_ Then allocate = False: Exit Function '……(1)' capacityOf_ = capacityOf_ - 1 If capacityOf_ = 0 Then isFull_ = True '……(2)' allocate = True End Function
変えたのは(1)と(2)のところ。
(1)の
If isFull_ Then allocate = False: Exit Function
isFull_がTrueだったら部屋割り不能なので、Falseを返す。
isFull_がFalseの場合は、部屋割り可能なので、次の行の
capacityOf_ = capacityOf_ - 1
で定員を1減ずる。
で、その後(2)の
If capacityOf_ = 0 Then isFull_ = True
で、定員が0になっていたら、isFull_をTrueに切り替える。
これで、次回この部屋インスタンスに部屋割りしようとするとallocateメソッドがFalseを返すことになる。
問題点その２
部屋割り処理が不細工すぎる
これは、
のリスト２の
Dim isAllocated As Boolean
Dim n As Integer
n = 1
Dim Sh As Worksheet
Set Sh = targetRange.Parent
Do
For i = 1 To roomCount
With rooms(i)
If Not Sh.Rows(targetRange.Cells(n, 1).Row).Hidden Then
If .allocate Then
targetRange.Cells(n, 1).Value = .nameOf
n = n + 1
If n > targetRange.Count Then Exit For
End If
Else '……(*)'
i = i - 1
n = n + 1
If n > targetRange.Count Then Exit For
End If
End With
Next
Loop Until n > targetRange.Count
この部分。改めて見直すと、マジで何がしたいのか分からんｗｗｗ
行き当たりばったりでテキトーにコーディングするとこうなる。
個人的には、特に(*)のところがヒド過ぎると思う。Forループの中でイテレータ変数をデクリメントさせるとか、正気の沙汰ではないｗｗｗ
これは根本から考え直す必要があると思った。
で、修正したのがコチラ。
リスト２ 標準モジュール
Dim isAllocated As Boolean
Dim n As Integer
n = 1
Dim Sh As Worksheet
Set Sh = targetRange.Parent
Dim tmpCell As Range
For i = 1 To targetRange.Rows.Count
Set tmpCell = targetRange.Cells(i, 1) '……(1)'
If Not Sh.Rows(tmpCell.Row).Hidden Then '……(2)'
Do Until rooms(n).allocate '……(3)'
n = n + 1 '……(4)'
If n > roomCount Then n = 1
Loop
tmpCell.Value = rooms(n).nameOf '……(5)'
n = n + 1 '……(6)'
If n > roomCount Then n = 1
End If
Next
変更前は、部屋の方をForループで回していたのだけれど、これがそもそもの間違いだったわけですな。
部屋の方は定員一杯になるまで何周もするんですからね。
今回は、部屋割り対象セルを上から順にForで回す、というやり方に改めた。
まず、(1)の
Set tmpCell = targetRange.Cells(i, 1)
部屋割り対象セルを変数tmpCellにぶち込む。後々カンタンに記述するためだけの措置です。
で、(2)の
If Not Sh.Rows(tmpCell.Row).Hidden Then
で、tmpCellの行が非表示になっているかどうかを判定。
非表示でなければ、ブロック内の部屋割り処理に移る。
tmpCellが非表示になっていなければ、(3)からの４行
Do Until rooms(n).allocate n = n + 1 '……(4)' If n > roomCount Then n = 1 Loop
で割り振るべき部屋を探す。
入り口のところでRoomクラスのallocateメソッドを実行してTrueが返ってきたら、n 番目の部屋に部屋割り可能ということなので、Loopを抜けて次に進む。
allocateメソッドがFalseだったら、(4)からの２行
n = n + 1 If n > roomCount Then n = 1
で n をインクリメントする。ただし、 n が部屋数を超えてしまったら n を1に戻す。
で、次。
ここまで来たら、後は部屋割り対象セルに部屋の名前を書き込むだけなので、(5)の
tmpCell.Value = rooms(n).nameOf
で部屋の名前を書き込み、(6)からの２行
n = n + 1 If n > roomCount Then n = 1
で n をインクリメント、つまり部屋を次の部屋に進める。
おわりに
まあ、だいぶマシなコードになったとは思います。
