returnと条件式内の代入

質問

Cでmemcmpライクな関数が必要になり作りました。(正確には中でmemcmpを何回か呼び出して比較をする関数)
そのときに出た疑問なのですが、ループ内でif文→returnとするのと、ループの条件にif文の条件を織り込んで、関数の最後でreturnするのとどちらが良いのか。
作った後で、いくつかのサイトでreturnはまとめるほうがいいとありましたので２つめの関数も考えました。しかし、条件式に=（代入）を書くのは好ましくないとする考えもあるようで、もう一つ作りました。しかし、初期化直後のretを比較するのは気分が悪いのです。悩んでいます。つまり、次のような関数でより好ましいのはどれでしょうか？ (いずれも質問用にmemcmpに書き直してます)

int mymemcmp(const void *a, const void *b, size_t cnt)
{
　　const unsigned char *ap = a, *bp = b;
　　int i, ret;
　　for (i=0; i<cnt; i++)
　　{
　　　　ret = ap[i] - bp[i];
　　　　if (ret != 0)
　　　　{
　　　　　　return ret;
　　　　}
　　}
　　return 0;
}

int mymemcmp(const void *a, const void *b, size_t cnt)
{
　　const unsigned char *ap = a, *bp = b;
　　int i, ret=0;
　　for (i=0; i<cnt && (ret = ap[i] - bp[i]) == 0; i++)
　　　　;
　　return ret;
}

int mymemcmp(const void *a, const void *b, size_t cnt)
{
　　const unsigned char *ap = a, *bp = b;
　　int i, ret=0;
　　for (i=0; ret == 0 && i<cnt; i++)
　　{
　　　　ret = ap[i] - bp[i];
　　}
　　return ret;
}

回答No.4

tatsu99 · ベストアンサー

個人的には１を推奨します。
＃２のかたが、述べているように、ソースをみたときのわかりやすさを優先すべきです。その意味では１と３が候補になります。
それで、１と３のどちらが良いかというと、この状態では、どちらでも良いでしょう。しかしながら、もし、forのネストが３重とかになったときに、一つの命令でループを抜ける方法は、Ｃ言語の場合、return文以外にありません。
その意味で、今後のことを考えると、１を推奨します。
今回の関数の要件は、「値が異なることが判明した時点で、処理終了となり、かつ、値が異なることを結果として返す。」ことが求められています。
もし、for文が３重になるような、ループで、上記の要件を満足する場合、１の方法が、良いと考えます。
以下、一般論になりますが、もし、処理中に異常が判明し、それ以上処理をする必要がないなら、即リターンする形式の方が、ソースがみやすくなります。
どちらのソースが簡潔か比較して下さい
-------------------------
以下ソース１
Aの判定
if Aのエラー発生 return;
Bの判定
if Bのエラー発生 return;
Cの判定
if Cのエラー発生 return;
全て正常な場合の処理

-------------------------
以下ソース２
Aの判定
if Aが正常{
　Bの判定
　if Bが正常{
　　　Cの判定
　　　if Cが正常{
　　　　　全て正常な場合の処理
　　　}
　}
}

お礼

たしかに、異常系はその場でreturnするのがシンプルで良いですよね。
ただ、今回は異常系とも微妙に違うので悩みどころです。

回答ありがとうございました。

回答No.3

tanma3

>returnはまとめるほうがいい
>条件式に=（代入）を書くのは好ましくない

確かにその通りなので以下提案です。
１番目の以下の部分をこう変えたら、上２つの条件が満足すると思います。
int mymemcmp(const void *a, const void *b, size_t cnt)
{
　　const unsigned char *ap = a, *bp = b;
　　int i, ret;
　　for (i=0; i<cnt; i++)
　　{
　　　　ret = ap[i] - bp[i];
　　　　if (ret != 0)
　　　　{
//　変更箇所　return ret;
break;
　　　　}
　　}
// 変更箇所 　　return 0;
　　return ret;
}

後、質問点とはまったく関係ない提案なのですが、キャストがcharだと長文の場合処理速度が遅くなるんでint型も利用して速度向上をめざす。
という具合でどうでしょう。

お礼

回答ありがとうございます。なぜか考えているときにbreakを忘れてたんですよね。混乱してたのがだいぶ落ち着きました。

本来の（この疑問が生まれた）関数では、さらにライブラリのmemcmpを呼び出しています。しかし、このmymemcmpを書いたときにはint型なんて忘れてました。ありがとうございました。

回答No.2

aigaion

なにをもって「良い」と判断するのかで答えは変わると思います。
私の場合は処理速度にそれほど関係しない部分であれば
後から見て何をやっているのかがわかりやすいことを持って「良い」プログラムと考えます。
ですので、3番目あたりを良いプログラムであると判断します。
1番目はループの終了条件のi<cntとret==0の関係が分かりにくい。
2番目はループ内で何をしているかがわかりにくい。
3番目であればループ内で何をしているか終了条件が何であるかが分かりやすいです。

処理速度を考えれば2番目が最速かと思われますが、そのあたりはコンパイラの最適化が
どうとでもしてくれる範囲であると思いますし、実際の処理時間もミリ秒の差もでないでしょう。

お礼

回答ありがとうございます。ソースの見易さについての質問でした。質問文に書き忘れていました。

回答No.1

JaritenCat

好みは人それぞれでしょうが、僕ならたぶんこう書きます。
　　for (i=0; i<cnt; i++) {
　　　　ret = ap[i] - bp[i];
　　　　if (ret != 0) {
　　　　　　break;
　　　　}
　　}
　　return ret;
forの中を複雑にしたくないのと、出力は最後にまとめたいからです。

お礼

回答ありがとうございます。なるほど、単にbreakでもいいですね。