C言語とC++言語コミュのこんなコードは書くな！

このコードは最近メンテナンスを頼まれているコードです。

// ● フルパスの設定
void CFileSpec::SetFullSpec(LPCTSTR spec)
{
　　Initialise();
　　//パスの分離、格納
　　_splitpath(spec,
　　　　(LPTSTR)(LPCTSTR)m_drive,
　　　　(LPTSTR)(LPCTSTR)m_path,
　　　　(LPTSTR)(LPCTSTR)m_fileName,
　　　　(LPTSTR)(LPCTSTR)m_ext);
　　UnLock();
}

//==============================================
　あれ、m_xxxって、CStringだったよなあ、なんか危険なコード書いているなあ、ちゃんと動くのか？
　う、ちょっと前後を見ると、

//==============================================
// ● バッファ確保
void CFileSpec::Initialise()
{
　　m_drive.GetBufferSetLength(_MAX_DRIVE);
　　m_path.GetBufferSetLength(_MAX_PATH);
　　m_fileName.GetBufferSetLength(_MAX_FNAME);
　　m_ext.GetBufferSetLength(_MAX_EXT);
}

// ● バッファ、メモリの開放
void CFileSpec::UnLock()
{
　　m_drive.ReleaseBuffer();
　　m_path.ReleaseBuffer();
　　m_fileName.ReleaseBuffer();
　　m_ext.ReleaseBuffer();

　　m_drive.FreeExtra();
　　m_path.FreeExtra();
　　m_fileName.FreeExtra();
　　m_ext.FreeExtra();
}

//==============================================
　おいおい、誰だこんなメンテナンスしづらいコード書いたやつは。確かに、GetBufferSetLength()のようなコードは、局所化しなさいと、教科書なんかには書いてあるはずだ。それは、ちゃんとReleaseBuffer()を呼び出すことを忘れないためである。それを見た目がきれいになるとか、局所化するば良いと別の関数にしちゃいけない。素直にこう書いてほしい。

void CFileSpec::SetFullSpec(LPCTSTR spec)
{
　　//パスの分離、格納
　　_splitpath(spec,
　　　　m_drive.GetBufferSetLength(_MAX_DRIVE),
　　　　m_path.GetBufferSetLength(_MAX_PATH),
　　　　m_fileName.GetBufferSetLength(_MAX_FNAME),
　　　　m_ext.GetBufferSetLength(_MAX_EXT))

　　m_drive.ReleaseBuffer();
　　m_path.ReleaseBuffer();
　　m_fileName.ReleaseBuffer();
　　m_ext.ReleaseBuffer();

　　m_drive.FreeExtra();
　　m_path.FreeExtra();
　　m_fileName.FreeExtra();
　　m_ext.FreeExtra();
}

　いやいや、もっと素直に、こう書けばよいだけでしょう。まったくもって、

void CFileSpec::SetFullSpec(LPCTSTR spec)
{
TCHAR drive[_MAX_DRIVE],path[_MAX_PATH];
TCHAR filename[_MAX_FILENAME], ext[_MAX_EXT];
　　//パスの分離、格納
　　_splitpath(spec, drive,path,fileName,ext)
　　m_drive = drive ;
　　m_path = path;
　　m_fileName = filename;
　　m_ext = ext;
}

コメント(37)

最初
全て
最新の40件

[2] mixiユーザー 03月30日 16:43

　何これブログ？　ﾐﾟ仝ﾟﾐ

[5] mixiユーザー 03月30日 19:33

>(LPTSTR)(LPCTSTR)

こういうキャストって、やりたくなる気持ちはわかるが、やっちゃだめだね！
CStringのGetBufferSetLengthって、基本的に使うべきではないと思います。

中途半端に中身をいじれてしまう、こういう手段を提供するMFCって、納得が出来ない部分もあるが、意外と好きです。

[6] mixiユーザー 03月31日 01:09

> それさえ守ればgotoでコードがこんなに美しくなることに、最近漸くきずいた。

何を築いた?

[7] mixiユーザー 03月31日 07:53

>何を築いた?

傷痛でねえの。ふつう。

[9] mixiユーザー 03月31日 08:59

> TCHAR drive[_MAX_DRIVE],path[_MAX_PATH];
> TCHAR filename[_MAX_FILENAME], ext[_MAX_EXT];
スタックにこんないっぱい積みたくないって人もいるよ。きっと…

[10] mixiユーザー 03月31日 10:21

> スタックにこんないっぱい積みたくないって人もいるよ。きっと…

何バイト以上が「こんないっぱい積みたくない」になるのだろうか。

[12] mixiユーザー 03月31日 11:04

Initialise ｗｗ

[13] mixiユーザー 03月31日 11:16

_MAX_DIR: 256
_MAX_DRIVE: 3
_MAX_EXT: 256
_MAX_FNAME: 256
_MAX_PATH: 260

気にするようなサイズでもないっすね。

[14] mixiユーザー 03月31日 11:27

> _splitpath(spec, drive,path,fileName,ext)

fileName?

[15] mixiユーザー 03月31日 11:42

_MAX_DIR、_MAX_DRIVE、_MAX_EXT、_MAX_FNAME、_MAX_PATH
の5つのうち4つが使用されて、
_MAX_DIRが使用されていないのが胡散臭いな。

[16] mixiユーザー 03月31日 17:03

　本来、GetBufferSetLength()って、実行時まで長さが見当も付かないようなときに使用する関数だと思います。
　最大長さがある程度の範囲にあるなら、使うメリットはないと思います。

＞sock/vuelaさん
　私も間違いに気が付きませんでした。_MAX_PATHは、パス全体を示す定数でしたね。きっと間違えたのでしょうね。(^_^;)

＞スタックに積みたくなかった
　私がWinのコードでなくて、組み込みのコードを書いていたり、昔のMS-DOS以前のＣを書いているときなら、スタックを気にしますね。8086だとスタックは64KBどまりでしたから

[17] mixiユーザー 03月31日 17:36

> 　私がWinのコードでなくて、組み込みのコードを書いていたり、
> 昔のMS-DOS以前のＣを書いているときなら、スタックを気にしますね。
> 8086だとスタックは64KBどまりでしたから

違う環境での「それが良い」の常識は
他では通用しないどころか
「それが悪い」の常識として扱われることもあり。

[18] mixiユーザー 03月31日 19:24

＞「それが悪い」の常識として扱われることもあり。

　ゲームプログラムの世界では、一般的なプログラムとまったく違った常識があるようです。
　例えば、普通のプログラムでは標準化してメンテナンス性を良くしましょうということで、コードのコピーを作るということは、よくないこととされます。
　一方一度CD-ROMに焼いてしまえば、二度と再利用をしないゲームの世界では、そのシーンに特有の動きを作るためなら、95%のコードをコピーして、残り5%だけを修正するということをするそうです。
（CD-ROMやDVDによって供給するようになった最近の話ですが）

　仕様書もない世界ですしね。

[19] mixiユーザー 03月31日 20:01

なんだ、この冗談みたいな『m_』は。
飾り?

[20] mixiユーザー 03月31日 21:34

LPCTSTR の変数を _splitpath() に渡してるのも気になるね。

MSはジェネリックテキストを推奨してるけど、個人的にはcharかwchar_tかどっちかきめうちのほうがいいと思う。

[21] mixiユーザー 03月31日 23:43

>19
『m_』は、クラスの「メンバ変数」を表す接頭辞として結構一般的に使われてますね。

>20
わからないでもないですね･･･
このトピの主題に則って言うならば、

TCHARとかLPCTSTRとかLPTSTRを使う以上は、

(1)文字は、L'x'
(2)文字列は、_T("xxxx")
(3)文字列を扱う関数は、_t系関数
を使うべき。即ち、

TCHAR s[]="abcde";
とは書くな、
TCHAR s[]=_T("abcde");
としろ。

strcpy( s2 , s1 );
とは書くな、
_tcscpy( s2 , s1 );
としろ。

switch(toupper(c)){
case 'Y':
･･･
break;
case 'N':
･･･
break;
･･･
}
とは書くな、
switch(_totupper(c)){
case L'Y':
･･･
break;
case L'N':
･･･
break;
･･･
}
としろ。

となると思いますね。

[22] mixiユーザー 03月31日 23:57

> TCHARとかLPCTSTRとかLPTSTRを使う以上は、
>
> (1)文字は、L'x'

本気ですか？

[23] mixiユーザー 04月01日 00:48

>> (1)文字は、L'x'
>本気ですか？

ですね。
ちなみに、これはWindowsでの話です。

[24] mixiユーザー 04月01日 01:10

L だと常にリテラルをwchar_tに置き換えてしまう。
リテラルをTCHAR に置き換えたいなら TEXT() か _T() ではなかったか。

それはそれとして、_tprintfで全角文字が出なくて困ってるときに、「既定のコードページ」に変換するようにしたい時はsetlocaleに何て書けばいいんだろ？

[25] mixiユーザー 04月01日 02:07

>L だと常にリテラルをwchar_tに置き換えてしまう。
>リテラルをTCHAR に置き換えたいなら TEXT() か _T() ではなかったか。

その通りですね・・・失礼しました。

1バイト文字ならば結局アルファベットの場合UnicodeでもAsciiでもコードは同じなので問題ないが、2バイト文字の場合は問題。

long a1='A';
long a2=L'A';
long a3=_T('A');
long b1='山';
long b2=L'山';
long b3=_T('山');

で試してみれば一目瞭然ですね。。。

[26] mixiユーザー 04月01日 03:25

>>24
vc++のライブラリだと、setlocale(LC_ALL, "")
#LANGでもなんでもお好きなように
とすれば、OEMコードページを使ってlocaleを設定していた
と思います。

[27] mixiユーザー 04月01日 09:28

少し調べてみました。

-------------
setlocale( LC_ALL, "" );
ロケールを既定値に設定します。これはシステムの既定の ANSI コードページで、オペレーティングシステムから取得します。

setlocale( LC_ALL, ".OCP" );
ロケールをオペレーティングシステムから取得した現在の OEM コードページに明示的に設定します。

setlocale( LC_ALL, ".ACP" );
ロケールをオペレーティングシステムから取得した ANSI コードページに設定します。
-------------
System.Diagnostics.Process もデフォルトでCP_ACPを使うようだし、".ACP" を指定しておくのが無難そうですね。

[28] mixiユーザー 04月01日 09:52

> long a3=_T('A');
　ぱっと見、顔文字かと思いました。

[29] mixiユーザー 04月02日 01:32

>>27
あ、OEMコードページじゃなくてANSIコードページでしたか。
失礼しました。
setlocaleの第二引数に .コードページ番号という表記の文字列が
使えるのは知っていましたが、.ACPとか.OCPとかいうのもあったんですね。
勉強になりました。

参考までにどこにあったのか教えていただけないでしょうか?
MSDNのどこかだろうとは思うのですが。

[30] mixiユーザー 04月02日 19:10

　_T("ABC")は、分かってはいるけど、あまり煩いことは言わないでやっています。unicode系とw_char系をプロジェクトの途中で切り替えることは稀ですから

[31] mixiユーザー 04月11日 00:18

【プログラマの格言】

プログラマーの格言１
一日は２４時間ある。
今日中という意味は明日の朝までという意味である。

プログラマーの格言２
プログラムは思った通りに動かない。書いた通りに動く。

プログラマーの格言３
要求仕様はプログラム完成後に完結する。
基本仕様は完成品を顧客が見てから決定される。
詳細仕様は使用者がプログラムを動かしてから固まる。

http://mixi.jp/view_community.pl?id=1772737

※不適切であれば、消去おねがいします。

[32] mixiユーザー 04月11日 00:38

> ※不適切であれば、消去おねがいします。

無責任の好例

[33] mixiユーザー 04月12日 07:48

>>29
setlocale、_wsetlocale (CRT)
http://msdn2.microsoft.com/ja-jp/library/x99tb11d(VS.80).aspx
でした。遅レス失礼

[34] mixiユーザー 04月12日 07:49

( はURLに使える文字のはずですが、mixiの自動リンクは間違った処理をしていますね…

[35] mixiユーザー 04月12日 08:25

かと言って括弧まで含めると
それはそれで発言者が意図しないリンクが増えそうな（笑）

[36] mixiユーザー 04月12日 11:08

> それはそれで発言者が意図しないリンクが増えそうな（笑）

それは意図が間違いであり、
その間違いは明確に立証されるのだから何も問題ない。

[37] mixiユーザー 04月12日 11:09

> ( はURLに使える文字のはずですが

%28で逃げるとか。