実際のコードで見る善し悪し(Oct.31)

01: ''---------------------------------------------------------
02: '' Function MakeCSVItem (ByVal sItem As String) As String
03: ''
04: ''  Generate '"' string
05: ''
06: Function MakeCSVItem (ByVal sItem As String) As String
07:     Dim i As Integer
08:     If IsNumeric(sItem) Then MakeCSVItem = sItem: Exit Function
09:     MakeCSVItem = ""
10:     If sItem = "" Then Exit Function    'I should return '""'?
11:     i = 1
12:     Do
13:        i = InStr(i, sItem, Chr$(&H22))
14:        If i <> 0 Then
15:            sItem = Left$(sItem, i) & Mid$(sItem, i)    'double the '"'
16:            i = i + 2
17:        Else
18:            Exit Do
19:        End If
20:    Loop While i <= Len(sItem)
21:    MakeCSVItem = Chr$(&H22) & sItem & Chr$(&H22)
22: End Function

1. 関数の脱出場所が複数ある(1つではない)
   一般に、関数のあちこちで終了するようなコーディングは良くないとされている。
   (しかし、GoToで飛ばすとか無意味にインデントを深くするか？この程度の関数で！？)
   だけどコメントとして「'==== EXIT here! ====」ぐらい付けておいてもバチは当たらなったかもしれない。
   
2. 同じ事(ロジック)なのに書き方が違う
   L08は：

   MakeCSVItem = sItem
   If IsNumeric(sItem) Then Exit Function
   

   と書けばL09,L10と同じ書き方になる。逆も可だが、コメントの関係で1行が長くなり過ぎる(しかし、Thenの後ろで改行すれば済む話ですな。これは私のミスですね。なお余計な事をしない、という点から評価すればL08のように条件がTrueの時のみに処理をするというのが正解と思う)
   
3. エラートラップがない
   …だがしかし。この関数でエラーが起こるとすればOut of memoryぐらいであって、トラップしたからとてどうなるものでもなかったりする(^^;
   (いつぞやと言うことが違う！と憤慨なさる向きは反論されたし)
   
4. Chr$(&H22)、要するに「"」だが、Const宣言した方が良い
   同感。少なくともコメントで「"」である。とか書いておくべきだった
5. Len(sItem)の評価を毎回している。時間の無駄
   これは実は迷った。でもループの終了条件に関る変数をループ中で変更するというのもまた極悪なので、こうなった。(極悪なのはiだけにして意味が分かりやすいようにしたかった)
   
6. IsNumeric()の引数はVariantであって、Stringではない
   この手の暗黙の型変換を期待していると思わぬ所で痛い目に合う(本当)。
   特に浮動小数点数でね……。まぁ、今回は目をつぶった。
   
7. コメント部にInput/Outputがまともに書いてない
   はい。手抜きです。済みませんm(_o_)m
   
8. 何でコメントが英語なのよ。日本人なら日本語で書け！
   Text形式でSaveしたソースをDOSから英語版BriefでEditする関係で、日本語のコメントは一切付けられません。VBのエディタも便利ですが、用途によってはゴミ以下ですから。例：
   
   1. 名前の一括変更
   2. 複数のプロジェクトを参照する必要がある場合
   3. 長大な関数を追跡する時(Briefは60行表示できます)
   4. VBのエディタでは不可能な操作をする時
9. 項目が文字列要素であって、内容が「""」だった場合、「""」が返らずに「」が返る
   そう。これがこの関数の最大の問題点。呼ばれた時にその引数は全て文字列としているから、元が数値なのか文字列なのか判断のしようがないのよね。
   でも、CSVで、「""」なんて出てくるのかな？調べてないのよね。実は。(こういういい加減な事をしては本当はいかんのだが……)
   

1. 引数をByValで宣言した点
   元の変数が変化しない事を明示した上で関数内部では、使いまくっている。文字列のByVal宣言はコスト高(*1)だが、Dim sTmp As Stringとか定義してsTmp = sItem として使うぐらいならこうした方が：
   
   1. 実行時のコストは変数宣言を別途するより同じかより低い
   2. 引数が関数実行後も変化しない事が明示(保証)できる
   3. 変数を増やす必要がない という意味でましです。
2. あくまで必要最小限の処理にした点
   処理中のアイテムがCSVの何番目か、とか数値項目か文字列項目か、などという判断は基本的に一切してません。与えられた物を、それだけを見て処理をしてます。
   呼び出す側が知っている筈の判断を呼び出された側でもまた処理すると言うのは余り賢い事ではないからです。
   
3. 改造するならここ、という点、L10にはコメントを入れている
4. ここが「ミソ」という点、L15とかにコメントを入れている(自己満足、とも言う)
5. L20、UntilではなくてWhileであること。まぁ趣味の問題という程度かも(Until=終わるのを待つ、While=できる間やる、とゆーニュアンス)