その他にも、気になる個所はいっぱいあります。以上に述べたことほど重要ではありませんが、 でも、相当におかしい点です。プログラムの流れに沿って調べて行きましょう。
■引数の書き方■66行を見てください。 関数call_melsecの第2引数はcccです。cccという名前のつけ方は実にいい加減ですね。 unsigned char ccc[200];としていますが、文字列引数にサイズを直接書くのは相当「異常」と言えるでしょう。ふつうは、 unsigned char ccc[];または unsigned char *ccc;と書くところでしょう。
■signed と unsigned■char型を使用するとき、必ず unsigned char として使っています。 char型は、signed になるか、unsigned になるかはコンパイラによって異なります。ただし、通 常の文字処理では、どちらになるか困ることは普通はないはずですが、困った経験をしたので、つ ねに unsigned char と書くようになったのでしょうね。 たぶん、char型が signed char になっているコンパイラを使用しているのでしょう。すると、 charの範囲は、-128 から +127 になります。ハード関係をやっていると、unsigned の方が都合が 良いことが多いのは事実です。でも、なくても良いものは、ない方がプログラムが見やすくなりま す。それではいやだというのなら、 typedef unsigend char uchar;という uchar 型を定義して、unsigned char と書く代わりに、たんに uchar と書いた方が見やす いでしょう。
■文字列定数■74〜80行で文字列定数が変数に代入されています。この部分は、極めて不思議な部分です。 たとえば、オリジナルでは static char ww[7] = "00FFWW";と、配列のサイズの7が直接「定数値」として書かれています。文字列の長さが6で、最後に0が 入るので、7文字分の文字配列を用意しなければいけないので7としたのでしょう。でも、実はもっ と楽に書けるのです。つまり、 static char ww[] = "00FFWW";という風に書けば良いのです。この書き方ですと、文字列の長さを数えなくても、コンパイラが自 動的に調べてくれます。ですから、文字列の長さが変わっても、文字列部分だけを変更すれば済み ます。元の書き方で、文字列が長くなったのに、サイズが元のままですと、コンパイルエラーが発 生します。 では、この配列は、どこで、どのように使われているのでしょうか。ww は、138行で、 strcpy(send_d,ww);として使われているだけです。これなら、わざわざ変数wwを使ったりせず、 strcpy(send_d,"00FFWW");と書いた方がすっきりしませんか。それが嫌なら、 #define WRITE_WORD_STYLE "00FFWW"としてから、 strcpy(send_d,WRITE_WORD_STYLE);とでもすべきでしょう。もちろん、ww,bw,wr,br の全部をそのようにすると、ずっとシンプルにな るでしょう。
■突然の定数はやめよう■このプログラムには、いたるところに突然、定数がでてきます。コメントのある個所は、その数 値の意味が分かるのですが、そうでない個所は意味が分かりづらいです。 意味のある定数には、意味を表す名前を考えて、マクロの記号定数を利用しましょう。たとえば、 116行に if(c == 0x1b) break;とあります。0x1bは制御文字のエスケープですが、これを、ファイルの先頭で、 #define ESC 0x1bと定義し、 if(c == ESC) break;と書き換えると、より分かりやすくなるでしょう。 制御文字のようなものは固定的ですが、文字配列のサイズなどは後で変更したくなるものです。 たとえば、 char buf[100];と直接サイズを書くのではなく、 #define BUFSIZE 100とマクロ定義し、 char buf[BUFSIZE];によって配列宣言をするのです。こうしておくと、サイズの変更は、マクロ定義の数値を直すだけ で済みます。このようにすると、「仕様変更に強い」プログラムに自然になっていきます。
■文字列処理■Cの文字列は、メモリ上の各バイトに文字データが並んでいて、文字\0(ヌル文字、値が0)で 終わりを示します。 これば極めて簡潔な決め方で、慣れてくると大変重宝するのですが、ひとつ間違えると「暴走」 の原因になります。この暴走の原理を理解せずCの文字列関数を使うのは無茶苦茶なことです。 今回は、104行で使用している strset について、なぜ「危険」かを述べます。104行は、 strset(send_d,0x00);で、文字配列 send_d を初期化したいらしいのです。実際にsend_dにデータを入れるのは、135 行目から並んでいる strcat で文字列コピーするので、本当は初期化の必要はありません。(じつ は、このswitch文のいずれかのcaseが成立します。) それでも安全のために初期化したいのなら、文字配列の先頭に0を入れればいいので、 send_d[0] = 0x00;としておけば問題はなかったのです。ところが、関数strsetを使ったので、思わぬ落し穴に落ちる ことがあるのです。必ず落ちるのではなく、「落ちることがある」という、メモリ内容による偶然 に左右されるバグに出会うのです。 関数の書式は、 strset( char *str, chr ch )で、文字列strの全ての文字を、文字chに置き換えます。 ところで、文字配列send_dは内部変数(自動変数)であり、関数呼び出しのときには初期化され ません。だから、send_dには何が入っているか何も分かりません。全部が0かもしれないし、全部 が0でないかもしれないのです。配列send_d中に0が含まれていれば、strsetは最初の0までをゼ ロクリアします。 もし、全部が0以外だったらどうなるでしょうか。send_dの内容を全部0にし、send_d[249]の 次以降のメモリも、その内容が0でなければゼロクリアを続けます。もう、メモリ上のどこを破壊 し続けるか分かりません。send_dは250バイトもあるので、たぶんその中には0のところもある ので暴走しなかったのでしょうね。
■static宣言■このプログラムには、あちらこちにstatic宣言が目立ちます。そして、望ましいところになかっ たりします。 static宣言は、つぎの2種類があり、その違いを十分に理解して使用しなければなりません。
(1)外部変数と関数のstatic宣言プログラムが大きくなって、ソースが複数のファイルになってくると、各ファイル中で使われて いる外部変数名や関数名の衝突が発生してきます。Cでは、static宣言をしないと、外部変数や関 数名は、全ソースのどこからでも共有されてしまいます。でも、あるファイルの中だけでしか利用 しないちょっと作った局所的な変数や関数までもが、ソース全体で名前が衝突しないようにするの は大変です。 コンパイルの単位、つまりソースファイルの内部だけに名前を限定し、他の部分から「隠す」と 便利です。外部変数と関数の先頭にstaticをつけるとこの隠す機能が働きます。 したがって、他に見せるべきでないものには、どんどんstatic宣言をしましょう。特に大きなプ ログラムを開発するときには重要です。
(2)内部変数のstatic宣言関数の内部の変数は、もともと局所的です。内部変数にstatic宣言をするのは、(1)の意味です るのではありません。 関数内部の変数は、その関数が呼び出された時点で必要な領域(変数の実体)がメモリ上に確保 され、関数の実行終了時点で領域は解放されてしまいます。したがって、何らかのデータを関数実 行後にも関数内部の変数に残そうとしてもできません。結果を残すためには、外部変数にでも入れ なくてはなりません。 原則は以上のようになっていますが、内部変数にstatic宣言をすると、関数実行後も変数を残す ことができます。 次の関数は、呼び出された回数をstatic変数として関数内部に確保しているので、呼ばれるたび に呼び出し回数を関数値として返します。 int increment() { static int num = 0; return ++num; }再帰呼び出しのとき、ふつうの内部変数は呼ばれるたびに別のメモリ領域が割り当てられますが、 static変数になっていると、再帰呼び出しされても変数は同じメモリ上にあるので、変数の内容が 知らない間に壊された、なんてことに悩んだりすることがあるので注意しましょう。
■フラグを使うな■「goto文は使うな」とよく言われるでしょう。でも、それと同様に悪いのが、フラグの多用です。 良く使う例は、ループの終了判定にフラグを使いますよね。ちょっとここでフラグを立てておいて、 後で参照しようという、例のやり方です。その上、フラグだからということで、たった1文字の変 数をよく使うのです。 関数input_paraの431行からの大きなwhileループが、その典型です。制御変数として、変数 c を使っています。この c は、ループの最後(538〜545行)の部分で、yes/no の質問に y と答え たときだけ1がセットされ、ループを抜け出します。フラグをセットする部分と、参照する部分が 離れれば離れるほどわかり辛くなります。 元のプログラムを直すことにしましょう。 まず c などという訳の分からぬ名前はやめましょう。yes_noという変数名に変えました。そし て、プログラムの流れからして、 do { 文 } while( 式 );を使うのが自然です。while と for は皆さんよく使っているようですが、do - while はなかなか 使っていないようです。これだけで、ずいぶん見通しが良くなったはずです。 でも、元のプログラムは、あまり変なフラグは使っていないですね。
■初期化■関数 watch_menu(354〜389行)を見てください。メニュー表示のためのデータを、配列 bi と koumoku に、延々と代入しています。一見きれいに見えますが、実は問題だらけの個所なのです。 まず、表示したい位置と表示データとは一体のものなので、別の配列にデータを入れてしまうの は不自然です。ですから、両者を一体化した構造体を作りましょう。 struct menu{ int x, y; char *message; };データの初期化に、メンバー毎に代入文で行なうのは非常に面倒臭く、やってられませんね。配 列の初期化は、宣言の場所で一気に行なうことができます。(リスト1−3)
各データは、x,y,messageの3つのデータでできているので、この3つを { と }で囲みます。 そして、必要なだけのデータを書いて、全体をまた{ と }で囲みます。 もとのソースでは、配列のサイズを直接指定しています。一応マクロを使って、 #define MENU_N 7としていますが、完全に無意味です。ちゃんとしているように見えてダメなので、最悪といえます。 サイズは直接指定せず、初期化データ自体にサイズを決めさせてしまうと、サイズは自動的に適正 な値に決まります。 配列宣言のとき、サイズ部分を書かずに [ ] としてしまう方法です。これは文字列定数の初期 設定と同じです。 メッセージの表示をするには、メッセージの数だけループするのですが、ループ回数の決め方に は2通りあります。 一つは、リスト1−3のように、データの最後に無効なデータを入れておき、無効なデータのと きにループから抜けます。ほんのちょっとだけ「ムダ」なデータを用意すると、簡単になります。 ムダの効用をしっかり身につけておきましょう。 もう一方は、配列のサイズを求めて、その回数分ループする方法です。サイズの求め方は、次の マクロを利用します。 #define Num(a) sizeof(a)/sizeof(a[0])配列全体のバイト数を、1要素のバイト数で割りサイズを求めます。これは、Xウィンドでは、標 準のマクロ(XtNumber)として入っています。 さて、以上の変更を加えたのと、元の2つのプログラムに対して、メッセージ数が増減したとき のプログラム修正量を考えてみてください。特に元のプログラムで困るのは、watch_menu の内部 だけを変更したのではちゃんと動作しないことです。配列のサイズがプログラムの先頭でdefineさ れているためです。ちょっと変更したいのに、プログラムのあちこちを変更しなければいけない典 型的な例ですね。 ここで紹介したような方法は、いろいろな場面で応用できるはずです。「楽なコーディング」を する方が、バグの入る可能性も低く、後のメンテナンスもしっかりできるでしょう。
■malloc を使うな■動的記憶管理関連の関数である、malloc、calloc、free は、初心者のうちは使わない方が良い でしょう。そもそも、「動的記憶」の意味を説明できないようでは、使用することは危険です。本 当に動的記憶にたよらなければいけない、大量のデータを扱うときに使いましょう。普通は配列で 十分です。配列ではどうしても問題があるときに限って使いましょう。 元のプログラムの417〜420行にmallocがあります。 tnn = (unsigned char *)malloc(3); tnnr = (unsigned char *)malloc(3); dwrud1= (unsigned char *)malloc(5); dwrud2= (unsigned char *)malloc(5);ここでは、たった3バイトと5バイトの領域確保のためにmallocを何度も呼び出しています。この部 分は、事前に、 unsigned char tnn[3], tnnr[3], dwrud1[5], dwrud2[5];と宣言しておけばで十分ですね。元のプログラムでは、これらのデータ領域は、16進数表示などの ために苦し紛れの努力のために使われたもののようです。sprintfを使えば、これらの領域を使う 必要すらなくなります。関数 input_para(391〜559行)は、ずいぶん悪戦苦闘した形跡がありま す。本当にご苦労様。でも、「悪あがき」ですね。
■ハードコーディング■「ハードコーディング」という言葉を聞いたことがありますか。石頭なコーディングのことです。 頭の固い人に困ってしまうことはよくあるでしょう。直接の上司がそうだったりしたら、悲劇です よね。ハードコーディングとは、プログラムが石頭で、ちょとやそっとの努力では絶対に直せない、 改良できない、機能追加できないようなプログラムのことをいいます。 この説明では、あまりに抽象的ですね。元のプログラムでいうと、プログラムのあらゆるところ に、数値が直接書かれているようなことをいいます。例えば、 locate(13,15);のような書き方が、プログラム全体にばらまかれていることです。このプログラム、もしある一群 のメッセージの開始位置を1文字右に寄せようとすると、いっぱい数字を書き替えないといけませ ん。もちろん、このような書き方では、ユーザがメッセージ表示位置やメッセージ内容の変更など の「カスタマイズ」が絶対にできません。 とりあえず、メッセージ表示の基準になるような位置などをマクロ定義します。派生的な位置は、 この基準位置から計算で出すようにします。 今回の手直しでは、関数input_paraとinput_batch中のlocate関数のx,y位置をマクロを参照して 求めるようにしました。 もう一方の完全にカスタマイズ可能な方法は、ちょっと面倒ですし、ここで紹介している規模の プログラムでは不要と思います。ですから、考え方だけを紹介しておきましょう。 まず、メッセージなどの表示位置、メッセージ内容など、変更可能にしたい部分を、アスキーファ イルにします。たとえば、リスト1−4のようにします。
プログラム起動時に、このファイルを読み込み、蓄えておきます。プログラムのメッセージ表示 をしたいところで、 output_message( "mess_wait" );のように書くと、引数の"mess_wait"に対応するファイル中に書いた位置にメッセージ内容を関数 output_messageが表示するように作ります。 これは出力だけですが、入力もこのようにしてしまうと、相当カスタマイズ可能になってきます。
|
オープンソース |