きれいなソースコードの書き方

[helloworld1853]

TCPでチャットするプログラムを作っていて
http://dixq.net/forum/viewtopic.php?f=3 ... =30#p90112
にて質問したものです。

このプログラムはcrypt関数で通信が暗号化され、
送信、受信データ先頭10バイトに
通信データのサイズが
0000000014
というふうに埋め込まれています。
このデータを使ってパケットが
分割されても
対応できるようになっています。

何とか完成したのですが、
ソースコードが読みにくいです。
以下がソースコードです。

http://s1.etowns.server-on.net/~yamada_ ... client.cpp
http://s1.etowns.server-on.net/~yamada_ ... server.cpp

このソースコードの改善点を交えながら
きれいなソースコードの書き方について
教えてください。
特に名前の付け方を・・・

よろしくお願いします。

[softya（ソフト屋)]

とりあえず、V2-client.cppを取り上げます。

気になることといえば、
・関数はＡ，名詞+動詞、Ｂ．動詞+名詞のAかBどちらかに全体を統一します。AとBがまじるのは良くないです。XstrncatとTAKE_SIZE_FROM_DATでは命名規則がバラバラです。
・大文字だけの識別子はマクロなどだけで使う様にして下さい。TAKE_SIZE_FROM_DAT。
・無闇に短い省略名を使わないでください。
・s1,s2などのように意味のない名前を使わないでください。dstStr,srcStrなど意味がある名前にしましょう。sourceとdestinationの略でよく使われます。
変数名で気になる所が沢山あります。名前に意味が無いと誤操作の可能性が高くなり可読性も下がります。
・関数の機能が直感的でないのは良くないです。
・strncatにバグ？文字列終端のナル文字処理が無いです。
それとs2_counter-1をしている理由が良く分かりません。汎用関数のよう名前で専用処理をしているのが問題だと思います。
・全体的にコメントが少ないでメリハリに欠け機能の分かれ目が分かりづらいです。
・BUFF_SIZEだったり、BUFF_SIZE-10だったりしますが説明もないのでなぜ-10の根拠が分かりません。
・TAKE_SIZE_FROM_DATA( recvbuff ); //データのサイズ取得
もそうですが機能名以上のことを内部で行なっています。こればプログラムの可読性を下がる大きな要因です。
データのサイズ取得以外にコピーをしていますが、それは関数にとっては蛇足です。
・やたらバッファコピーが多いです。
もっと少なく出来ますしコピーのバッファサイズチェックやら長さチェック込みで共通関数化したほうが良いです。
strcatとかstrncatとかを更に大きく包んだものです。

・潜在バグ
recvbuff[recv_size] = '\0';
はバッファを破壊する可能性があるのでガードが必要です。他にも似たような所が沢山あります。
サーバーでガードしていても、こちらでもガードしないと意味が無いです。

他にもあるのですが、とりあえずはこれだけをお願いします。

[helloworld1853]

少し出かけていて
返事が遅れました。
いま直しています。
ここで疑問に思ったのですが、
たとえばmain関数でソケットを作ったとして、
そのソケットをほかの関数の引数に渡し、
その関数でソケットをclosesocketを使って
閉じるということはできますか。

[softya（ソフト屋)]

少し出かけていて
返事が遅れました。
いま直しています。
ここで疑問に思ったのですが、
たとえばmain関数でソケットを作ったとして、
そのソケットをほかの関数の引数に渡し、
その関数でソケットをclosesocketを使って
閉じるということはできますか。

出来ないことはないですが、開くと閉じるが同じ流れに無い見渡しの悪いプログラムはキレイなプログラムとは言えないと思います。

[jay]

プログラムの書き方についてならばこの本をオススメします

プログラミングのセオリー

関数や変数の命名について気をつけるべきこと
処理を早くするためのコツ
分かりやすいコードを書くために心掛けるべきこと

などなど、色々参考になるお話がタップリでした
中古ならかなり安いみたいなので興味があればどうぞ～

[softya（ソフト屋)]

キレイなコードと関係の深いこんなのもあります。

「CERT C セキュアコーディングスタンダード」
http://www.jpcert.or.jp/sc-rules/
「Ｃ言語のよくある間違い」
http://www.ncos.co.jp/products/cgi-bin/errorcall.cgi
「プログラミングの禁じ手Web版 C言語編(InternetArchiveよりサルベージ） - Akio’s Log」
http://d.hatena.ne.jp/elwoodblues/20090206/1233878763

これも参考になるでしょう。
「きれいなソースコードを書くために必要な、たったひとつの単純な事」
http://d.hatena.ne.jp/r-west/20090510/1241962864

[Dixq (管理人)]

softyaさんが指摘されていない部分のみ書きます。

①forの記述を改善する

コード:

int s2_counter = strlen(s2);
int counter = 0;
for(s2_counter -= 1 ; position > counter ; s1_counter += 1, s2_counter -= 1, counter += 1)

このforの初期状態の記述は普通書かない書き方なのでやめた方が良いと思います。
for(s2_counter -= 1 ;
とするなら
int s2_counter = strlen(s2) -1;
で良いかと思います。

②同じようなコードをまとめる

'0'
'1'
'2'
...
という部分は文字を数値に変換して書ければよいかと思います。変換エラーならエラー処理をすると良いでしょう。
同じコードが大量にあると不具合の元です。

③関数名のつけ方を一般的にする

Get○○(); ...何かの値を取得する関数
Set○○(); ...何かの値をセットする関数
Is○○(); ...状態などを問うtrueかfalseを返す関数

など、関数名の先頭には何をする関数なのかその動詞を付ける場合が多いですが、特にGet,Set,Isはこれに従って命名するのが良いでしょう。
また、既に上がっているように名前は意味のあるものにします。
例えばスコープが広い範囲でdataという変数名を使うと初めて読む人には意味が伝わりません。
(狭いスコープの中では変数名は簡略化して良いとされています)

④意味のある関数化を

関数化したら、その関数はそれ以上の仕事をさせないようにします。
また、たった一行であっても、その処理が特に意味のあるものであれば関数化することで可読性を上げることもできるでしょう。
複雑な計算が続き、パッと見、わけがわからないコードになりそうであれば、その処理一つずつを関数化して読みやすくすることもあります。
ただしオーバーヘッドや高速化の必要性と天秤にかける必要はあるでしょう。

⑤コーディングスタイルを一貫させる

if(){

だったり

while()
{

だったりして、カッコの位置が一貫していません。
どちらでもいいですが、一つのプログラムの中に色んなスタイルがあると読みにくいです。
スペースの入れ方についても同様です。

[helloworld1853]

http://s1.etowns.server-on.net/~yamada_ ... client.cpp
http://s1.etowns.server-on.net/~yamada_ ... server.cpp

まだまだ未完成ですけど
プログラムの流れを大幅に改善し、
なにより関数化できたので、
のせておきます。
関数化は不慣れなので、
少しおかしいところがあるかもしれません。
残りの名前の改善を
しています。

[softya（ソフト屋)]

またまた、目についた所から。

・startpositionは単語の切れ目が分かりづらいのでstart_positionかstartPositionをお薦めます。他変数も同様です。

・前から気になるのですがプログラムを組んだらデバッガで動作を確認していますか？strRANcatとか明らかにおかしな間違いがあります。

コード:

	for( ; startposition <= endposition; counter += 1, startposition += 1 ) {
		dstSTR[counter] = srcSTR[startposition];
	}

個々の部分の動作を具体的に日本語で書いてみてください。
あとやたら使う変数が多いです。

strncatの簡単な実装例。
http://www.bohyoh.com/CandCPP/C/Library/strncat.html

・何度も指摘されていますが関数名と内容がずれています。
CreateTcpClientSocket()はconnectまでしているので名前的にはconnectと分けるべきです。

・クライアントになぜCreateTcpServerSocketがあるのでしょうか？
本当はファイル分割したいが、その前段階と言うことでしょうか？

・関数内で完結できることは関数内で解決を。
maiにあるrecvbuff[len] = '\0';が違和感です。ReceiveAddedData内で記述できないでしょうか？
main関数としての機能を逸脱していると思います。

・if( strcmp( comment, "exit" ) == 0 ) {
の処理ですがbreakだけで良いのでは？

・ReceiveAddedData内でrecvbuffの扱いが１回目と２回目で違います。
同じような処理が沢山あるのでさらなる関数化を推し進めてください。
※　ReceiveAddedDataのサブ関数化ですね。

[helloworld1853]

ありがとうございます。
今直し始めて思ったのですが、
たとえば
定義と同時に
文字列を0に自動的に初期化
してくれるSCHARという
自作の型を作ることはできますか。

[softya（ソフト屋)]

定義と同時に
文字列を0に自動的に初期化
してくれるSCHARという
自作の型を作ることはできますか。

それは初期化を書けば良いことなので隠蔽すると返って分かり辛く成る危険性が高いです。
直感的である ＆ 何ヶ月後にも見て分かるかを基準にしてください。

[beatle]

ありがとうございます。
今直し始めて思ったのですが、
たとえば
定義と同時に
文字列を0に自動的に初期化
してくれるSCHARという
自作の型を作ることはできますか。

helloworld1853さんの書いたソースを読んだわけではないので的外れかもしれませんが、自動的に初期化してくれる文字列ってstd::stringのことでしょうか。std::stringはC++の標準ライブラリなのでC++で書いているなら使えます。

[helloworld1853]

夏休み中まったくプログラミングができず、ずいぶん間が空いてしまいました。
すみません。

久々に質問しますけど、
クライアント側が一気に400KB送信し、
サーバ側が1KBずつ受信していくと、
無事400KB受信できますか。
それとも1KB受信したところで、
途切れてしまうのでしょうか。
よろしくお願いします。

[softya（ソフト屋)]

質問のタイトルと全く無関係なので別途質問して欲しいです。
この内容に返信する気なら絶対に別質問にしてください。

４００ｋｂがエラーで途切れずに届くことは保証できないと思います。
何処でエラーで途切れるかは回線の状況次第なので絶対に届くことを前提に組んではいけません。
インターネットでよく有ることですが、途中でエラーになる事は十分にありえます。
エラーにならず届いている限りは大丈夫でしょう。

[helloworld1853]

よくデバッグするとき、
あるchar配列の中身が
半角の「フフフフフフ・・・」になっているときと
そうなっていないときがあります。
どうしてですか。

あと、ポインタがさす配列を'\0'で初期化する方法
はありませんか。

ソースコードを書く上で重要なので、よろしくお願いします。

[softya（ソフト屋)]

よくデバッグするとき、
あるchar配列の中身が
半角の「フフフフフフ・・・」になっているときと
そうなっていないときがあります。
どうしてですか。

スタック上の自動変数は何が入っているかは運次第ですがＶＣ++のデバッグビルドの場合に限り「フフフフフフ・・・」の初期化されます。
mallocした場合はＶＣ++のデバッグビルドの場合に限りﾍﾍﾍﾍﾍﾍﾍﾍと初期化されるはずです。
これはリリースビルドされると変わります。

それ以外の場合は、グローバル変数かstaticな変数ではないでしょうか？
その場合はナルで初期化されていると思います。

あと、ポインタがさす配列を'\0'で初期化する方法
はありませんか。

ソースコードを書く上で重要なので、よろしくお願いします。

空文字列をstrcpy(string,"");するとか、自分でstring='\0';とかでしょうね。

【補足】
何が入っているかはＣ言語/C++の規格書に書かれていないものは当てにすべきではありません。
明示的に初期化したほうが分かりやすいですし、安全で確実です。

[helloworld1853]

今プログラムのテスト動作をしたのですが、
ファイアウォールにひかかってしまいます。
upnpを使おうとしているのですが、
資料が少なくて困っています。
唯一見つけたサンプルコードが
http://ohwhsmm7.web.fc2.com/win/UPnP.cpp
なんですけど、
このソースコードのメイン関数に
PortOpen( "192.168.1.2", 12345 );
PortClose( "192.168.1.2", 12345 );
と書かれているのですが・・・
"192.168.1.2"の部分をグローバルIPにすると、
どうなるんですか？

[softya（ソフト屋)]

今プログラムのテスト動作をしたのですが、
ファイアウォールにひかかってしまいます。
upnpを使おうとしているのですが、
資料が少なくて困っています。
唯一見つけたサンプルコードが
http://ohwhsmm7.web.fc2.com/win/UPnP.cpp
なんですけど、
このソースコードのメイン関数に
PortOpen( "192.168.1.2", 12345 );
PortClose( "192.168.1.2", 12345 );
と書かれているのですが・・・
"192.168.1.2"の部分をグローバルIPにすると、
どうなるんですか？

どうなるんでしょう？　やってみたらどうでしょうか？
私に聞くよりも確実ですし、どうなったかをテストする方法も見つけないと行けませんよね。
こういうのは勉強しないと行けないことなので実践有るのみです。

それはそれとして前の質問はスルーなのでしょうか？

[helloworld1853]

＞どうなるんでしょう？　やってみたらどうでしょうか？
いつもは母のパソコンがあるので、それを使って
通信のテストができたのですが、
今は使えないので、質問させていただきました。
とりあえず自分のパソコンでテストしてみます。

＞それはそれとして前の質問はスルーなのでしょうか？
すみません。
お礼を申し上げ忘れていました。
string='\0';
を使って初期化します。

[helloworld1853]

うまくいきませんでした。
長引きそうなので、新しいトピックをたてます。
問題点が次から次へと出てきてくじけそうですが、
がんばります。

[softya（ソフト屋)]

うまくいきませんでした。
長引きそうなので、新しいトピックをたてます。
問題点が次から次へと出てきてくじけそうですが、
がんばります。

少なくともプログラム歴１年から２年の人が手を出すべきは無い範囲まで手を出しているので苦労するしか無いと思います。
※　それだけ難易度が高く知識が必要という意味です。

【補足】
マルチポストされていたので、記載しておきます。
「uPnPでポート開放するプログラムを作っているのですが、 Yahoo!知恵袋」
http://detail.chiebukuro.yahoo.co.jp/qa ... 1193911026

ルール違反ですので即時の対処をお願いしたい所です。

[helloworld1853]

すみません。
今気づきました。
知恵袋に補足しようとしましたが
投票中でだめでした。
投票中が終わって補足できるようになるまで
新しいトピックを作るのはやめます。
以後気をつけます。

http://s1.etowns.server-on.net/~yamada_ ... unctions.h
http://s1.etowns.server-on.net/~yamada_ ... r_main.cpp
http://s1.etowns.server-on.net/~yamada_ ... t_main.cpp

さて　ソースコードを自分なりに改善しました。
動作確認済みです。
for文の初期化式を使わないようにしていますが、
読みやすくするために仕方がなく一部つかっています。
「なぜ、このように書いたのか」と問われれば
答えられるように書きました。

ご指導よろしくお願いします。

[softya（ソフト屋)]

for云々かんぬんより、ヘッダにプログラムを書くのはやめたほうが良いと思います。
ちゃんとfunctions.cppを作りましょう。

読みづらくさせている要因を列挙します。
1)functions.h インクルードガードがstdio.hなどの単位である。　→　stdio.h内でガードしているので余計なお世話です。
2)functions.h ほとんどコメントがない。
3)client_main.cpp マジックナンバー →　scanf_s("%s", Comment,BUFF_SIZE-11);
4)client_main.cpp 関数化の不統一性。　send()は直接呼ぶのにReceiveAddedData_Cryptは関数で内包している。
5)functions.h これはバグ。数値HeaderLenサイズがある前提の処理でナル文字の事がContentStrで考慮されていない。動いているなら偶然。
ContentStrにどう文字列が格納されているか書いてみたら分かります。
6)functions.h strcat_s(DestBuff, BuffSize, SrcBuff);はガードになっていない。
7)functions.h recv等にエラーチェックがない。あるいはチェックするタイミングが遅すぎる。
8)functions.h BuffSizeが何のバッファサイズか分かりづらいのでDestBuffSizeにした方が良いでしょう。
9)functions.h TotalRcvdSizeがBuffSizeを超えた時のガードをしていない。

[asd]

オフトピック

本題からは逸れますが、指摘しておきます。
少し厳しい言い方にしています。
なお、私へのリプライ(返信)は不要です。

すみません。
今気づきました。
知恵袋に補足しようとしましたが
投票中でだめでした。
投票中が終わって補足できるようになるまで
新しいトピックを作るのはやめます。
以後気をつけます。

この数日間で初めてここを利用し始めたわけではないですよね。
ということは今まで利用規約を読まず（または理解せず)に利用していたということですか。
(マルチポストをする際の約束ごとが書かれています)

ちなみにYahoo!知恵袋は投票期間終了後、ついている回答に投票がされている場合、
一番投票の多い回答がベストアンサーとなり解決済み状態に移行します。
解決済みとなった質問には補足をすることはできません。

そして、仮に投票がない(またはベストアンサーとなる回答がないの投票が一番多い場合)にはその質問は取り消しとなります。
つまり、投票状態になってしまった回答にはもう二度と補足をすることはできないわけです。

Yahoo!知恵袋には回答をつけてくれた方がいますが、結果としてその方の回答を無視してしまったわけで。

他にも本題と大きく逸れた質問をしてみたり、目の前に試せる環境があるのに試さず質問してみたりと他にもいいたいことはありますが、
今後はよく注意してください。

[helloworld1853]

http://s1.muryo-de.mydns.jp/~yamada_mam ... ctions.cpp
http://s1.muryo-de.mydns.jp/~yamada_mam ... r_main.cpp
http://s1.muryo-de.mydns.jp/~yamada_mam ... t_main.cpp

すべて修正しました。
アドバイスありがとうございます。

[softya（ソフト屋)]

とりあえずコメントが中途半端です。細かくは言いませんのでよく考えてみてください。
１年放置した時に内容がすぐ理解出来るかどうかがポイントです。