たまに「じゃんぬねっと」が生存確認をする日記

ヘタレプログラマなんですが、おもしろそうなのでちょっと参加させて頂きます。

A1.
> Private Sub KoshinShori()
プロシージャ名から処理内容が予想しにくいのでもっと具体的な名前にすべき。

> On Error Resume Next
コモンダイアログをキャンセルしたときの為なんでしょうが
エラーをトラップするなら On Erro GoTo ～ を使うべき。

> Dim CMDLG
> Dim FNAME, DNAME As String
CMDLGとFNAMEがVariant型。変数名が全て大文字なのもHorror。
ハンガリアン記法を採用すべきか？

> Set CMDLG = CreateObject("MSCOMDLG.COMMONDIALOG")

> ' コモンダイアログの設定
> CMDLG.Filter = "カンマ区切りファイル(*.csv)|*.csv|すべてのファイル(*.*)|*.*"
> CMDLG.FilterIndex = 1
> CMDLG.DialogTitle = "ファイルを選択"

> MAINFRM.VA_SPREAD.Row = ROW
> MAINFRM.VA_SPREAD.Col = 1
> MAINFRM.VA_SPREAD.Action = 0
MAINFRM（多分暗黙にインスタンス化されたフォーム）に標準モジュールからアクセス
すべきじゃない。カプセル化すべき。ここが一番のHorror。
あとROWって変数（定数？）はどこに宣言されてるんだろうか？

もしMAINFRMはグローバルスコープのForm型変数だったりしたらもっとHorror。


> CMDLG.ShowOpen

> コモンダイアログをキャンセルしたときは空にする仕様なのか？
MAINFRM.VA_SPREAD.Text = CMDLG.Filename

> End Sub

そもそもこの関数はコントロールの操作しかしてないのでフォーム内に書くべきですね。
CreateObjectしなくてもコモンダイアログ使えるし。

A2.
得意な言語は無さそう（^^:
あえてあげるとすればVBScriptかな？

A1. おおむね正解だと思います。

> CreateObjectしなくてもコモンダイアログ使えるし。

これが何故まずいのか説明して頂きたかったです。
いわゆる、アーリーバインディングでないのは何故? です。

A2. についてはちょっとニヤニヤしてしまいました。

気になった部分を追加します＾＾；
A1.
>Dim CMDLG
型宣言できるものは型を宣言しておかないとコンパイル時に
型エラーが発見できないですね。
ってreですでに書いてるけど＾＾

あと、変数はプロシージャの先頭で宣言する必要もないのでは？と
変数の有効範囲は最小化したいと僕は思います。
#でも、カウンタ変数をForの直前で宣言して先輩に注意された経験有^^;


>Dim FNAME, DNAME As String
使っていない変数を宣言する必要がないんではないかなと

>Dim CMDLG
変数名（もしくは型）で察しはつくのですが、
重要な変数にはコメントがほしいです。

>CMDLG.Filter = "カンマ区切りファイル(*.csv)|*.csv|すべてのファイル(*.*)|*.*"
>CMDLG.FilterIndex = 1
>CMDLG.DialogTitle = "ファイルを選択"
>MAINFRM.VA_SPREAD.Row = ROW
>MAINFRM.VA_SPREAD.Col = 1
>MAINFRM.VA_SPREAD.Action = 0
>CMDLG.ShowOpen
>MAINFRM.VA_SPREAD.Text = CMDLG.Filename

同じ変数の処理はできるだけまとめるほうが読みやすいですよね。

With CMDLG
.Filter = "カンマ区切りファイル(*.csv)|*.csv|すべてのファイル(*.*)|*.*"
.FilterIndex = 1
.DialogTitle = "ファイルを選択"
.ShowOpen
End With

With MAINFRM
.VA_SPREAD.Row = ROW
.VA_SPREAD.Col = 1
.VA_SPREAD.Action = 0
.VA_SPREAD.Text = CMDLG.Filename
End With

#なんとなくWithは好きではないのですが・・・

A2.
大文字ばかり＆変数名を8文字以内にする言語・・・？
COBOLとかFortranくらいしか思いつかないのです

RSSリーダー上で、書いてみました。メモリを忘れる＆お休みで、ずいぶんで遅れてしまった。。。
↓↓↓↓↓
"On Error Resume Next" って、いかんだろ
変数 CMDLG に、型が宣言されていない
Module でありながら Private
意味のないコメント（コモンダイアログの設定）
っつうか、スプレッドの設定をしちゃいかんだろ
ってか、MAINFRM って・・・まぁ、大目に見るか
英語とローマ字の入り交じった命名
中途半端に省略された変数名
とりあえず、「すべて大文字の変数名」は、VB なので大目に見る
コモンダイアログが「キャンセル」されたときの処理がない
っつうか、戻り値見ろよ
"KoshinShori" って、何を更新するのさ？
っつうか、ファイルを指定して、何をしてるの？
CreateObject の成否も見るべきかな？
で、DNAME って、必要なの？
ROW って何さ？

> Private Sub KoshinShori()

何を更新するの?
関数名は UpdateXxxx にして明示化すべきじゃないの?
成功の可否の戻り値は?

> On Error Resume Next

エラーを無視するなら細かいエラー処理は実装しましょうよ。
というか、全体で無視しないの。
無視するのは CommonDialog を ShowOpen する直前ですよね。

> Dim CMDLG

型指定していない。
バリアント厨ですか。
っていうか、何で全部大文字変数?
定数だと思われちゃわない?

> Dim FNAME, DNAME As String

FNAME は型指定しているつもり?
この 2 つの変数は使用していないよね?
それと、変数名は省略しないこと。

> Set CMDLG = CreateObject("MSCOMDLG.COMMONDIALOG")

何故にレイトバインディング?
コンパイル解決もできていない。
戻り値が取得できたかどうかのチェックもない。

> ' コモンダイアログの設定

無意味なコメント。(見ればわかるわい)
それよりも書くことがあるはず。
コメントは緑の彩りを揃える飾りではないよ。

> MAINFRM

何ですか、これは?
参照を持っているのか、暗黙のインスタンスなのか?
どちらにしても、プライベート (リアルではグローバルだった orz) 以上で定義すべきものなの?

> MAINFRM.VA_SPREAD.Row = ROW

ROW はどこから来たの?
ローカル変数でないところを見ると、
プライベートな... あれ? 全部大文字だから定数なのかな?

> MAINFRM.VA_SPREAD.Col = 1

スプレッドの Columns 指定は列挙体でやるべき。
マジックナンバーの使用は保守性を悪くする。

> MAINFRM.VA_SPREAD.Action = 0

定数を使うべき。

> CMDLG.ShowOpen

キャンセルの処理がなされていない。
事前に On Error Resume Next として、
Err.Number を見る必要がある。

ここで、ShowOpen するなら、この直前で CommonDialog を生成すべきでは?
いや、フォームに貼り付けるべきだけど。

デフォルト ディレクトリ設定しなくていいの?
デフォルト ファイル名設定しなくていいの?

> MAINFRM.VA_SPREAD.Text = CMDLG.Filename

というか、フォームの処理をモジュールでやっちゃだめでしょ。
フォームの外からこういうことするから、カプセル化の概念が崩れ、
1 つ単位だけで管理できずに、苦ソース化する。

> End Sub

結局、更新処理でも何でもなくない?