トップページ⇒tech
1001コメント291KB

ぱっと見て「ヘタだなぁ」と思うコード

■ このスレッドは過去ログ倉庫に格納されています
0001デフォルトの名無しさん2006/03/09(木) 10:16:55
基礎編
・サブルーチン(関数)が数百行
・ネストが深い
・マジックナンバーが使われまくり
・識別子(変数名、関数名)が意味不明
・グローバル変数使われまくり

↓応用編、詳細編どうぞ

0002デフォルトの名無しさん2006/03/09(木) 10:17:53
２げっとー！
0003デフォルトの名無しさん2006/03/09(木) 10:18:30
・コメントの装飾が妙に凝っている
・古いコードがコメントアウトで残してある(しかも履歴つき)

0004デフォルトの名無しさん2006/03/09(木) 10:25:01
＞・古いコードがコメントアウトで残してある(しかも履歴つき)
それが下手と関係あるのか？
0005デフォルトの名無しさん2006/03/09(木) 10:39:34
>>4
かなりヘタ。

0006デフォルトの名無しさん2006/03/09(木) 10:47:24
古いコードって、コメントしても元に戻すことってまずないよね
俺はコメントしても、時間が経ったらコメントごと消すようにしてる
0007デフォルトの名無しさん2006/03/09(木) 10:48:45
＞・古いコードがコメントアウトで残してある(しかも履歴つき)

究極は、#ifdef 0 でコメントアウト。
0008デフォルトの名無しさん2006/03/09(木) 10:49:10
イベントドリブンの環境で、「グローバル変数状態つかいまくり」状態が多いね。
イベントハンドラから呼ばれた入力データのチェックルーチンが直接コントロール
なり、フォームの入力を保持してる変数を直接見にいってるから、使い回しがきかない
とか。

0009デフォルトの名無しさん2006/03/09(木) 10:53:10
役満＼(＾o＾)／

これも置いておきますね。
・制御文が2種類しか使われない
0010デフォルトの名無しさん2006/03/09(木) 10:57:15
>>7
#if 0
ではないのか
0011デフォルトの名無しさん2006/03/09(木) 11:13:06
いまメンテしているC++のクラスのメソッド

Public:
int initialize()
int execute()
int finalize()

以上。
0012>>11 2006/03/09(木) 11:15:46
補足。

int initialize()・・・呼ばれていない
int execute()・・・約１２００STEP
int finalize()・・・呼ばれていない

orz
0013デフォルトの名無しさん2006/03/09(木) 11:17:21
>>11-12
下手なだけで、「int execute()・・・約１２００STEP」を読めなくは無い。
クラスにしてくれてるだけ、メソッド分離しやすいぉ。
0014デフォルトの名無しさん2006/03/09(木) 11:49:21
いやまあヘタなコードのスレですから
0015デフォルトの名無しさん2006/03/09(木) 11:51:58
修正可能なソースはヘタじゃないですぉ。
ニッチもサッチも逝かないのが、ハードコーディングですぅ。
0016デフォルトの名無しさん2006/03/09(木) 12:12:29
>>6
古いコードは残しておく。

別にパフォーマンスにも響かないし、最近のエディタなら
邪魔だと思ったらたたむことも出来る。

とくに最適化前のコードは絶対に残しておく。計測用に
切り替えて使うこともあるから。

一度書いたものはなんであれ財産。消すことは絶対許して
いない。
0017デフォルトの名無しさん2006/03/09(木) 12:17:29
>>16
なんか力、入ってますねー。
ソースコード管理システムを使ってれば、HEAD/trunkで消してもすぐ元に戻せるんですが。
自分はソースの見通しが悪くなるようなのは積極的に消します。
残すのはコメントでリビジョン書いとくくらい。
0018デフォルトの名無しさん2006/03/09(木) 12:27:33
人力でバージョン管理やってるところって、絶対「財産」を無駄にしてるよな。
一世代くらい前ならともかく、数世代まえになると、コメントアウトでコードを
残すなんてことをやってるところじゃ、復元不可能。
0019デフォルトの名無しさん2006/03/09(木) 12:29:13
つ CVS >>16

バージョンにブランチ（枝）すると、複数の分岐で開発出来るお。
で、良いやつを本枝にマージすると完璧。
0020デフォルトの名無しさん2006/03/09(木) 12:53:55
>>6
バックアップ取れば良いじゃん
0021デフォルトの名無しさん2006/03/09(木) 12:58:44
マジックナンバーって何だっけ
度忘れちゃた
0022デフォルトの名無しさん2006/03/09(木) 13:02:09
ここはツールでソース管理されてないインターネッツですね。
0023デフォルトの名無しさん2006/03/09(木) 13:05:50
お前ら、あ、愛してる。だからﾊﾞｰｼﾞｮﾝ管理ｋｗｓｋ教えて。
0024デフォルトの名無しさん2006/03/09(木) 13:10:49
Cで、NULLと0と'\0'の使い分けができてないやつとか。

文字列のクリアで
memset(str, NULL, sizeof str);
みたいな。
(memset()使ってるってのもあれだし。本人は手間をかけて安全なコーディングを
してると思ってるっぽいけど)
0025デフォルトの名無しさん2006/03/09(木) 13:13:08
>>24
NULLは意味的に間違いとしても、
memsetが何でダメ?

組み込み系ソースのfor文で回して0にしてたり、
メモリのアロケートやり直しで初期化してるつもり（本人から確認）のコードの方が驚いた。
0026デフォルトの名無しさん2006/03/09(木) 13:32:19
>>25
初期化子使えとかそういうことでは？
0027デフォルトの名無しさん2006/03/09(木) 13:35:26
>>25
strcpy(hoge, "初期値"); か hoge[0] = '\0'; で十分なところを、
memset()で全部クリアしてるってこと。
0028デフォルトの名無しさん2006/03/09(木) 13:49:45
>memset()で全部クリアしてるってこと。

これ、C++とかじゃなくて、C言語とかだと結構セキュアに動作するお。

C言語の開発中バグでメモリ破壊したときとかに。
0029デフォルトの名無しさん2006/03/09(木) 14:03:54
ﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌ・・・が見れなくなるのでさびしい
0030デフォルトの名無しさん2006/03/09(木) 14:06:42
>ﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌﾌ・・・
って難だっけ？

メモリカードとかはFFで初期化したような。
0031デフォルトの名無しさん2006/03/09(木) 14:10:53
Cの文字列を入れる変数で
hoge[0] = '\0'
ならバグるところを
memset()
ならバグらないって状況を、セキュアと呼んでいいのだろうか。。。
0032デフォルトの名無しさん2006/03/09(木) 14:14:10
28自体もスレタイに沿ったコードの例のつもりで書き込んだということだろう。
0033デフォルトの名無しさん2006/03/09(木) 14:51:24
>>31
開発中にアプリがどっかーんといくときに、
ＯＳ吹っ飛ばさない、その方がボロクソデバッガでも変数とか把握しやすい、ということ。

Win16時代にC言語プログラミングしてない若造には分からんこと。
0034デフォルトの名無しさん2006/03/09(木) 14:53:13
そこでOpenBSDですよ。
0035332006/03/09(木) 14:53:14
ちなみにC++ではヌルターミネイト自体古いというか、もうやらんじゃん。

＞hoge[0] = '\0'
を評価しても、Ｃ言語ではふつー、Ｃ＋＋ではダサダサ。
0036デフォルトの名無しさん2006/03/09(木) 14:55:03
perl で

`rm $foo`;
`mv $bar $bar.bak`;

とか。
0037デフォルトの名無しさん2006/03/09(木) 14:57:37
>>33
バグが表面化しないのくらべたら、OSごとドッカーンといくほうがかなりマシというか。
0038デフォルトの名無しさん2006/03/09(木) 15:01:37
どうも37は分かってないようだが、C言語はC++と違って大局的な書き方じゃないから、
OSがドッカーンと逝かない（普通逝かない。Win16以前しかそんな話無いし、汎用機がドッかｰﾝと逝ったら大騒ぎで該当ツール使用禁止令も出る）アプリをバグ無しアプリと言うんだが。
0039デフォルトの名無しさん2006/03/09(木) 15:03:58
不毛だなぁ
0040デフォルトの名無しさん2006/03/09(木) 15:04:08
>>33
安全策をとるならば、文字列の終了位置であるはずのところに
'\0'を上書きしとくべきなのでは？
0041デフォルトの名無しさん2006/03/09(木) 15:07:25
>>40
　>>35
0042412006/03/09(木) 15:09:11
で、C言語時代にはＯＳとかアプリ実行環境が不安定にならないことに最重点を置いたから、
ヌルターミネイトを大目にとmemsetとした。
0043デフォルトの名無しさん2006/03/09(木) 15:10:45
>>1
ヘタなスレ立てだな・・・
最近立つのはゴミスレばっかり
0044デフォルトの名無しさん2006/03/09(木) 15:12:06
if (hoge != false)
なんてのを見つけると、そこから先を読むのが厭になる。

>>40
それをしないのが、下手糞が下手糞たる所以。
0045デフォルトの名無しさん2006/03/09(木) 15:12:36
たとえば
(1)バグがある
(2)コードを追いかけたりしない
(3)とりあえず文字列をmemset()で0クリアしてみよう
(4)直った!
これは直ったわけじゃなくて、表面化しなくなっただけ。
こういうことをしてる新人がいたら、デバッグをやり直させるでしょ。

memset()で前もってクリアしておこうってのは、いきなり(4)の状態になる
可能性があるわけで、バグの存在さえ認識できない可能性がある。

0046デフォルトの名無しさん2006/03/09(木) 15:13:11
そういえば、Ｃ言語時代は構造体をmemsetってもう決まりだったお。
charも安心、intとかfloatもintelでは０になるし、実は他CPUでも０になる。
どのソース見てもmemsetの嵐だお。
0047デフォルトの名無しさん2006/03/09(木) 15:14:12
45=しつこい

どちらにせよ、
＞hoge[0] = '\0'
おまいはＣ＋＋時代のダサダサﾌ用人間。
0048デフォルトの名無しさん2006/03/09(木) 15:14:34
int foo[10];
for(int i = 1 ; i < 11 ; i++)
foo[i - 1] = 0;
0049デフォルトの名無しさん2006/03/09(木) 15:16:05
>>41
いや、それは違うだろ。
0050デフォルトの名無しさん2006/03/09(木) 15:16:24
>>45
＞memset()で前もってクリアしておこうってのは、いきなり(4)の状態になる
＞可能性があるわけで、バグの存在さえ認識できない可能性がある。

Ｃ言語時代にはこれを目指してmemset使われまくり（当時の慣習）でしたが、何か？
0051デフォルトの名無しさん2006/03/09(木) 15:17:21
正直、今の時代において何の役にもたたない昔話はどうでもいい。
0052デフォルトの名無しさん2006/03/09(木) 15:17:31
49=stringを知らない無知を無知と分からない
0053デフォルトの名無しさん2006/03/09(木) 15:18:16
>>44
if (hoge == true)
に比べたらまだましではなかろうか。
0054デフォルトの名無しさん2006/03/09(木) 15:18:24
>>51
なら、
＞hoge[0] = '\0'
こんなコードを書く香具師は頃してしまえ。
0055デフォルトの名無しさん2006/03/09(木) 15:18:35
こちらのスレへどうぞ
http://pc8.2ch.net/test/read.cgi/tech/1127283408/
0056デフォルトの名無しさん2006/03/09(木) 15:18:55
>>53
同意。

trueは不定、falseは0固定だもんね。
0057デフォルトの名無しさん2006/03/09(木) 15:21:02
C言語コードは構造体と初期化の嵐だから、
「C言語＋memset」
「C++＋stringでヌル無視」
の組み合わせが良い。

＞hoge[0] = '\0'
↑
こいつはこれだ！ >>ttp://pc8.2ch.net/test/read.cgi/tech/1127283408/
0058デフォルトの名無しさん2006/03/09(木) 15:21:25
>>50
OSのカーネルとか、データベースとか信頼性が要求されるコードでは、
そういうの見たことないなぁ。
0059デフォルトの名無しさん2006/03/09(木) 15:21:29
瞬時に糞とわかるコード
・mainメソッドの先頭３０行くらい全部変数宣言だけになっているとき
0060デフォルトの名無しさん2006/03/09(木) 15:21:34
>>50
それは君の周りだけでは？
0061デフォルトの名無しさん2006/03/09(木) 15:23:02
>>58
　>>60
0062デフォルトの名無しさん2006/03/09(木) 15:24:04
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
↑
こいつヴぁか杉！！！！！！！！！！！！！！！！！！！！！！！！！！！！
0063デフォルトの名無しさん2006/03/09(木) 15:26:01
そっとしといてやれよ。
きっと、デスマーチが当たり前な悲惨な現場で育ったんだよ。
0064デフォルトの名無しさん2006/03/09(木) 15:27:07
日中こんなスレに常駐して暇潰してる奴の生産性をどうにかしてください。
0065デフォルトの名無しさん2006/03/09(木) 15:28:15
>>62
そういう暴れ方しても、言ってることに説得力でないよ。
0066デフォルトの名無しさん2006/03/09(木) 15:28:27
現代においてあんなことを言う56のコードも下手なんだろうなぁ。
0067デフォルトの名無しさん2006/03/09(木) 15:29:25
説得力が無くて結構。

至上最強の恥ずかしさ： hoge[0] = '\0'

C言語解説本の出だしでしか見た事無いぉ。
0068デフォルトの名無しさん2006/03/09(木) 15:30:40
>>66
現代においてもtrueが不定という定義は普遍ですが、何か？

逆に古い実装のtrueがdefineされてる場合にのみ正常動作。
0069デフォルトの名無しさん2006/03/09(木) 15:31:56
>>68
俺はC++のつもりで話をしたが、どうやらお前はC++とは別の言語の話のようだ。
すまんな、俺の勘違いで。
0070デフォルトの名無しさん2006/03/09(木) 15:32:35
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
＞hoge[0] = '\0'
↑
氏んだ方がいいよwwwwwwww
0071デフォルトの名無しさん2006/03/09(木) 15:33:01
69=至上最低のプログラマ二人目

C言語ならTRUEがdefineされてるから正常動作だが、
C++言語なら言語仕様のtrueだから値がますます不定になる。
0072デフォルトの名無しさん2006/03/09(木) 15:44:12
>>44 >>53 >>71
？
あえて聞きたいのだが、不定でも別に問題ないんじゃないか？
例えば↓のようなコードが合っても、
if( b==true ) ;
代入時でどうせ↓のようにしてるんだから問題ないんじゃないの？
bool b = true;

↓コレだったらまあわかるんだけど。
int i = 1;
if( i == true );

↓この記述が悪いっていってる意味があまりわからん。
if( b==true ) ;
0073デフォルトの名無しさん2006/03/09(木) 15:44:40
bool b = true;
int *p = reinterpret_cast<bool*>(&b);

こうでもすれば*pの値はたしかに不定だろうが、static_cast<int>(true)は1だ。
JIS X3014:2003の4.7の4節。
0074デフォルトの名無しさん2006/03/09(木) 15:46:04
>>72
>代入時でどうせ↓のようにしてるんだから問題ないんじゃないの？
>bool b = true;

Win32APIをご存知無い？
返す値がBOOLでありながら不定（実体はハンドル。メモリポインタ）だお。
0075デフォルトの名無しさん2006/03/09(木) 15:47:19
>>74
？
それはWin32APIのインターフェースでTRUEとFALSEという定数で比較するようにすべきだから、
C++のtrueとfalseで比較するほうが間違ってるんじゃないか？
0076デフォルトの名無しさん2006/03/09(木) 15:47:29
>>72
そういえば、mem〜、str〜系の関数も失敗で０を返しても、
成功はポインタ返してtrueの代わりにしてるね。

0077デフォルトの名無しさん2006/03/09(木) 15:48:57
>>75
現実は真偽値の定義をとって、trueの場合は０以外の不定値をふつーに返す関数が溢れながら、
それを知らずにとは。

>代入時でどうせ↓のようにしてるんだから問題ないんじゃないの？
>bool b = true;

↑
こいつプロジェクトメンバから外せ。
007872=752006/03/09(木) 15:49:01
あるぇ？BOOLの比較ってTRUEかFALSEでいいんだっけ？
Win32APIなんて懐かしいものはもう忘れた orz
0079デフォルトの名無しさん2006/03/09(木) 15:49:29
>>78
TRUE比較はダメ。
0080デフォルトの名無しさん2006/03/09(木) 15:49:34
こういうスレって必死に揚げ足取ったり重箱の済みつついたりに終始して
得る物がないんだよな。
0081デフォルトの名無しさん2006/03/09(木) 15:50:13
80=常識を知らず叩かれた香具師
0082デフォルトの名無しさん2006/03/09(木) 15:51:21
おまえら、さっさとバグつぶせよ
0083752006/03/09(木) 15:51:49
>>77
＞現実は真偽値の定義をとって、trueの場合は０以外の不定値をふつーに返す関数が溢れながら、
そういう戻り値は大概intだろ？boolのtrue、falseと比較する意味がわからん。
それにそういう関数の仕様を読めば何と比較するか書いてあるだろ。trueとfalse以外で比較することが。
それをtrueとfalseで比較するのか君は。
0084デフォルトの名無しさん2006/03/09(木) 15:51:59
TRUE,FALSE,true,falseの区別がむつかしいなら
C++なんて窓から投げ捨ててCやれば良いじゃん
0085デフォルトの名無しさん2006/03/09(木) 15:52:09
>>82
戦死にもたまには休息が必要だ
0086デフォルトの名無しさん2006/03/09(木) 15:53:44
バグと戦い2chでも戦い…
大変だねPGって
0087デフォルトの名無しさん2006/03/09(木) 15:53:46
すげー伸びてるなあ
クソスレかと思ったら良スレだったんだな
0088デフォルトの名無しさん2006/03/09(木) 15:54:20
>>83
おまい、超無知杉。

char *pc = getPt();

if (!pc) throw exception("Error");

とかやるじゃん。
0089882006/03/09(木) 15:55:19
char *pc = getPt();

if (pc = false) throw exception("Error");
はおｋでも、

if (pc != true) throw exception("Error");
はダメだろ。
0090デフォルトの名無しさん2006/03/09(木) 15:56:21
スレを読まずにカキコするが、

おまえら　こういう話　大好きだよな
0091デフォルトの名無しさん2006/03/09(木) 15:56:49
どっちにしても、bool値と比較してる時点で「ヘタだなぁ」と思うよ
0092752006/03/09(木) 15:57:21
>>88
いや、あの、それくらいは知ってるんだが…。

俺が言いたいのは↓のような間違ったtrue、falseの使い方を例示もせず
char *pc = getPt();
if (pc==false) throw exception("Error");

↓のコードだけ例示して==false使う奴はアホとかいってるのがよくないといいたいの。
if (pc==false) throw exception("Error");
0093デフォルトの名無しさん2006/03/09(木) 15:57:48
>>87
もしそう見えてしまうならまともな書籍買ってきて目を通したほうがいい。
こんな糞スレにいるより10倍の量と精度の知識が身につくだろう。
0094562006/03/09(木) 15:59:53
>>92=ゴミ
結論。
おまいの読み間違い。
>>44 >>53 >>56
の流れだから、TRUEとの比較はダメの流れ。

89でも同じ文章を書いている。
0095752006/03/09(木) 15:59:54
>>89
いや、ポインタをfalseで比較するのは駄目かと・・・。NULLで比較すべきかと・・・。

その元のgetPt関数だって、エラーの場合は戻り値でreturn NULL;とか書いてるはずだろ。
return false;じゃ型の不一致でコンパイルエラーだとおもうぞ。
0096デフォルトの名無しさん2006/03/09(木) 16:00:17
必死に反論してる奴見てると笑える
0097442006/03/09(木) 16:02:03
いや、true の値なんかどうでもいいんだが。
hoge が真理値のつもりなら、なんでいちいち等価演算子
使うんだ？っつー話。
0098デフォルトの名無しさん2006/03/09(木) 16:02:04
>>95

いや、Win32/C言語の全盛期はFALSEがDEFINEされてて、

if (XXX != FALSE) { //ハンドルが無効で無い
　//処理
}

と説明本でも書かれてる。
0099デフォルトの名無しさん2006/03/09(木) 16:05:13
>>98
いつの時代の話だよ。 つーか氏ねよ
0100752006/03/09(木) 16:05:28
>>44
おー、そういうこと？例えば↓こうとか？
int a=100;
int condition = (a==100);
if( condition == false ) ;
それなら確かに。
==とかの比較演算子はたしか偽の場合０で真の場合１でいいんだっけ？
それをtrue、falseではたしかにまずいな。
01011002006/03/09(木) 16:06:18
やべ>>97だった。
■ このスレッドは過去ログ倉庫に格納されています