2009年12月11日

続・C言語プログラミング能力認定試験のサンプルがすごい件について

昨日のつづき。


ただ「ひどい」と言い放つだけなのも無責任なので、「もしこのソースを、誰かがレビューに持ってきたら」と仮定して、自分なら何を言うかな、と考えてみました。

もしこのプログラムを教育目的で使うなら、同じようにこソースが含んでいる問題点を考えてもらうほうが、普通に問題解くより為になるような気がする。


誰得な文章です。もう、読んでもらえることすら期待してない(笑
プログラムに興味のない人にはまったくわからないエントリーなので、スルー推奨です。

でももし、俺がおかしなことを言ってる部分があったら、指摘してもらえると泣いて喜びます。


たぶん著作権の問題がでるので、ソースは引用しないことにします。内容変えられちゃったら困るから、引用したいんですが、そうするとほとんど全行になりかねず、それは引用でもなんでもなくなってしまうので。

では続きは「続きをよむ」から。プログラミングが分からない方は、以下は無視したほうがよいかと思います。

・ソースの在り処
ttp://www.sikaku.gr.jp/js/side3/scon04.html

----------------




■ prog.h l.6

これは好みかもしれませんが、ここで標準ライブラリのヘッダを include するのはいかがなもんでしょうね。

ヘッダからヘッダを include するのは禁止といいたいわけではなく、あるファイルからinclude されるヘッダは、c ファイルでもヘッダファイルでも、そのファイルに必要だから、そのファイルで include するべきだと思うんですけどね。

prog.h の場合、標準ライブラリの構造体定義やマクロなんかを使っているわけではないから、このヘッダを他のソースからinclude したらまるで無駄かもしれない。 prog.c の方が、どうせこのヘッダで include しているからいいやとばかりにこの prog.h しか include していないので、正しいヘッダを include しているのかどうか、見通しが悪い。

この発想は、なんかいらん問題を引き起こす元だと思えます。


■ prog.h l.11

定数のマクロには () つけてほしい。。特に負の数。まぁ、四則演算に使うものではなさそうでなのでいいかもしれないけど、すごく気になる。

ついでに使われているところを確認すると、実はOK と NG と CANCEL がユニークである必要があるが、だったらわかりづらいので、enum にして欲しい。


■ prog.h l.19

構造体は typedef しろ。それに、タグ名が全部大文字なのは何故。

外部のファイル(バイナリ)にそのまま書き出してそのまま読む仕様のようなので仕方ないのかもしれませんが、日付の8文字のデータを格納するのが char [8] なのは、超怖い。使ってる関数では一応考慮している風ではあるけど、爆弾だよね、これ。

本来外部から読むデータ長やバイトオーダを意識するものなら、char(uchar)の配列とかint32_tで読んで内部表現にはめなおすとか、読む部分と使う部分を切り分けるべき。短時間で扱う試験用のソースではなくなってしまうかもしれないけど。

どうでもいいが、将来の拡張に備えて、この手のデータにリザープ用の特に意味のない領域を設けておくのはいいとして、なんでそのサイズが char[2] なんだろう。メモリ上でも使う型なのに。16bit のアーキテクチャなの?

このプログラムって、最後に「リターンキー」を押してくださいっていうメッセージを出してるんだけど、もしかしたら 16bit の Mac で動いてるのかね。そんなんあったっけ。


■ prog.h l.39

ヘッダで外部リンケージを持ったグローバル変数の宣言をするな。ヘッダでは extern 、実際の宣言は global.c みたいなソースファイルを別途作るか、せめて main() と同じファイルにまとめて書くべきだと思う。

そもそもグローバルである必要があるのかっていうデータばかりのように見えるが、そこは譲るとして、これが prog.c の頭に書いて static にしてあるなら妥協します。


--------------


■ prog.c l.21

ファイル名は、せめてマクロで定義してください。他の関数でもいちいちローカル変数で宣言しなおしてるし、意味がわからない。file1/file2っていう名前の付け方のセンスも素敵。


■ prog.c l.25

ファイルがなかったら作る、ということをやりたいために、いちいちopen/closeする必要がありますかね。ほとんどの場合は既にあるんだろうから、処理能力の無駄です。というか、他の関数とか見てると、read/write をしたいときにも同じファイルをいちいち開けたり閉めたり。オーバーヘッドをなめすぎでは。

特定のファイルのインスタンスは一つに決まってるのだから、この手のデータこそグローバル変数あたりにするべきでは? 今更マルチスレッドに対応した場合の話とかしちゃう?


■ prog.c l.48

個人的に TRUE が 0 FALSE が -1 とかでマクロ定義されている、ふざけたソースを何回か見ているので、激しく不安になります。これなら 1 と 0 を直接書いてください。stdbool.h は C99 だからダメなのかな。

ついでに、この処理がやりたいなら do while 文のほうが素直だと思いますが、まぁ、なぜか毛嫌いする人も結構いるので、好みの問題か。


■ prog.c l.68

scanf()。scanf()。

それに比べれば瑣末なことだけど、文字列用の領域を NUL 文字でフィルしたいなら、'\0'と書いて欲しい。

■ prog.c l.99

main の戻り値がありませんが。コメントのとおりとはいえ、大丈夫なんですかね。たまに見るんですよね。他の関数でこれをやってしまうと、スタックポインタがズレて破滅する可能性もあるはずですが、main() はいいの?

■ prog.c l.120

return に () つけるの止めない?


疲れてきた。

■ prog.c l.352

char の配列の「クリア」というコメントと共に 0x20(半角スペース)で埋めてるセンス。しかも fillerまで。そこは予備領域なんだから 0 ビットで保障にしとくべきでは。

この手の処理で sizeof 演算子使ってるときに、型名書くのか変数名書くのか、どっちかに統一してくれないかな。というか、変数名書けるなら変数名書いとけと思う。


■ prog.c l.466

このチェックの仕方は初めてみた。メリットがよく分からないのだけど、もしかしたら何かの定石なのかと思うぐらい、個人的にびっくり。普通ループして isdigit() とかじゃないだろうか。

atoi() ねぇ。個人的に後輩とかには、「atoi()は単独でエラーチェックができないから、もしかしたら使っているだけで舐められる可能性もあるので、特にポリシーがないなら strtol() 使え」って言ってるんだけど。ソースが納品物でない場合は、なんとなく面倒で自分でも使うこともあるんだけどさ。


■ prog.c l.644

この関数面白いなぁ。

フォーマット指定子の使い方を頑張って調べたんだねって感じで。関数作ってputc()あたりで一文字ずつ置いてくれる方が安心できるんだけど。

あと、ループを使わない意味がわからない。

細かいかもしれないけど、if else の書き方も気に食わない。短い else 節のほうが先にあったほうが見やすいし、else に return 書くなら、それこそ先に else の部分書いてそこで弾いて、printf の山のインデント外したほうがいいと思うけど。

この書き方だと、出力フォーマットを増やすだのなんだのの機能追加が入った時に、ここにどんどん if-else と printf の山が追加されて、ひどいことになりそうな気がする。


本気で疲れてきた。


■ prog.c l.1190

なんでここだけポインタ演算なんだ。同じ意味のことを配列で散々書いているんだから、統一しなさい。


■ prog.c l.1238

計算結果は関数の戻り値でいいのでは?


■ prog.c l.1297

細かいんだけど、このコメントだと普通のポインタ演算しようとして、間違えて配列のインデックス入れているように見えるので、ファイルポインタをポインタと略すのは止めて欲しい。


■ prog.c l.1411

俺の頭が確かなら、バブルソートに見えるのだが・・・まぁ、増えて何千件っていう仕様らしいから、別にいいかもしれんけど・・qsort() じゃダメなの? そもそも記録するときに既にソートされていてもかまわないように見えるから、挿入ソートでもよさそうな。わざわざ最低の選択肢選んでませんか。

なんか構造体の代入を使わない人っていますね。あと、それを知らない人も。この場合は値保障のために、もしかしたらあるかもしれないパディングの部分まで含めてコピーしたいのかもしれないけど、そこまで考えたのかどうか。



この手のプログラム作るなら、設計の時点でファイルのオープンクローズのタイミングも少なくなるように考えるべきだし、なんなら最初にメモリに全部読み込んじゃうかマッピングすることも検討するデータサイズだと思う。それが設計されてないのは、その時点でおかしい。

広域のグローバル変数みたいな静的な変数の持ち方も設計時点で決めるべき。それに対して、ローカルな関数の仕様なんかは、結構どうでもいい。

こんなしょうもない関数の名前を並べて、各関数の処理概要が並べられているだけの設計書持ってこられて、どうやって設計ミスを見つけろというのか。こんなドキュメントなら、ソースにちゃんとコメント入れるように決めておけば、ツールで自動的に生成できるだろうし、設計の時期にこんなドキュメント作るのに工数使われたらたまらんなぁ。

posted by VOT at 07:31| 東京 ☁| Comment(2) | TrackBack(0) | 日記 | このブログの読者になる | 更新情報をチェックする
この記事へのコメント
「半角カタカナのコメント」に対するツッコミはなしですか。
いつの時代のソースかと愕然としましたが。

しかもほとんどすべてのコメントが無用なコメントだし……
Posted by (ぱ) at 2009年12月15日 01:24
半角カナについては、仕様書に
多バイト文字を使うなみたいなことが書いてあったので
もう、仕様で仕方がないんだと判断しました^^;

資料の内容を見る限り2004年の問題らしいですが
なんでこんなことになっちゃったんですかね。
Posted by 主 at 2009年12月15日 11:22
コメントを書く
お名前:

メールアドレス:

ホームページアドレス:

コメント: [必須入力]

認証コード: [必須入力]


※画像の中の文字を半角で入力してください。
この記事へのトラックバックURL
http://blog.seesaa.jp/tb/135288247

この記事へのトラックバック
×

この広告は1年以上新しい記事の投稿がないブログに表示されております。