C/C++でのコードの書き方について

Question

##１、C/C++でのコードの書き方について

最近C/C++の業務に移ったのですが、下記のようなメソッドを多く見かけます。

// ログ等処理1

while(1)
{
    if( IsValid1() == FALSE )
    {
        // エラーログ1
        break;
    }
    if( IsValid2() == FALSE )
    {
        // エラーログ2
        break;
    }
    if( IsValid3() == FALSE )
    {
        // エラーログ3
        break;
    }
    if( method4() == FALSE )
    {
        // エラーログ4
        break;
    }
    if( method5() == FALSE )
    {
        // エラーログ5
        break;
    }
    
    // やりたい処理6
    break;
}

// ログ等処理2

上記のような実装はC/C++では一般的な書き方なのでしょうか？

##２、メソッドの分割について
また、処理が長いメソッドも多いのですが、メソッドを細かく分けると実行速度が遅くなる弊害が発生しやすいなどC/C++としてそういう傾向にあったりするのでしょうか？
細かく分けると遅くなる気はあまりしていないのですが…。

Answer 1

こんにちは。

上記のような実装はC/C++では一般的な書き方なのでしょうか？

C言語では比較的よく見かける記述のように感じます。昔のC言語は bool型がありませんでしたので少しでも意味を伝えようと FALSE と比較していました。
FALSEと比較すると２重否定になりがち(xx != FALSE)ですので TRUE と比較したくなりますが、C言語の定義上、!=0が真ですから間違ってTRUEと比較するとバグりますのでFALSEとの比較が推奨されていました。

しかし、その辺りのサポートが強力なC++ではあまり好ましくない書き方と思います。
C++には 最初から bool 型がありますから、素直に true と比較した方が分かりやすいです。
更に if文の条件式はbool型を期待しますから、わざわざif (xxx == true)などと書かず、if (xxx)と書いた方がスマートに見えます。
もし、各関数の戻り値が bool型でない場合は、戻り値をenum 型（scoped enumなら更に望ましい)で定義するとバグをコンパイラが検出しやすくなります。

また、処理が長いメソッドも多いのですが、メソッドを細かく分けると実行速度が遅くなる弊害が発生しやすいなどC/C++としてそういう傾向にあったりするのでしょうか？

細かいメソッドは、inline定義すると速度は落ちないです。
ただ、メソッドの粒度には要注意です。ロジックを整理できていない複雑なメソッドや、ロジックを整理できていたとしても細かすぎるメソッドは可読性を劣化させますので。

Comments

[mitama_rs]

最近C++17の業務をやっているんですが、variantを使うといい感じになる（業務でC++17を使っているところがツチノコレベルで珍しい）。

[reasoku]

>Chironianさん
ご回答ありがとうございます。

上の方の回答の方にも書きましたが、流用の本当に大本のソースがCで実装されていたようです。
Cにはそんな歴史があったんですね。
実際、関数の返却値を0を失敗1を成功としているものと0を正常1を異常としているものともあったりしてかなり混乱しました。
C#をずっと書いてきたので、あまり失敗と成功をメソッドの返却値で返すイメージがなかったので。
ちょうど単体試験のフェーズなので記載していただいたような修正は、出来る範囲で入れていこうかと思います。

>EnumHackさん
C++17ですか…。そこまで新しくすることは今の業務だとなさそうですね。。。VisualStudio2010で開発している環境ですので…。

Answer 2

ソースが元々CだったかC上がりの人が書いたのでは？
結構古くからある方法でgoto有害論のおかげでgotoは悪だ！使用禁止！けどエラー時は特定の場所にまとめて飛びたい、というので生まれた手法だったかと。
使う使わないは人によると思いますが、パターンとしてそこまで珍しい書き方でもないかなー、という印象です。

C/C++としてそういう傾向にあったりするのでしょうか？
細かく分けると遅くなる気はあまりしていないのですが…。

言語としての傾向ではないと思います。
組み込みとのことなので「理由あって」関数を分けてないなら使用しているマイコンのためじゃないでしょうか？
・メモリが小さくてスタックがあまりとれないからあえて関数化をあまりしない
・割り込みとかタイムクリティカルな処理で関数を飛ぶ時間も惜しい
というのがぱっと思いつく理由です。

あとはまぁ、動いてる部分を変えたくないから少しずつ拡張を重ねたせいでダラダラ大きくなった、というのもよくある理由ですね。
大抵の場合はちゃんと関数化したほうがいいと思います。

Comments

[reasoku]

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

流用の大本のソースがCで書かれていたようでした。goto回避で生まれたんですね。

なぜそうなったか原因はわからないのですが、おそらく挙げて頂いたような理由ではなさそうなので、出来る範囲で読みやすく改善はしていきたいと思います。

[Chironian]

たぶんgoto回避のためのwhile-breakと混乱していると思います。
質問文にご提示のソースはgoto回避構造にはなっていません。

例えば、以下の方法ならgotoや例外、余分な関数を使わなくてもエラー処理をまとめる事ができます。
int success=FALSE;
do
{
　　if(エラー１)
break;
　　if(エラー２）
break;

　　success=TRUE;
　　正常処理;
}
while(0);
if (success == FALSE)
{
　　エラー処理
}

しかし、エラー処理を１箇所にまとめるための goto に反対する勢力はだんだんと少なくなっていった印象を受けています。却ってプログラムが読みにくくなり、本末転倒だったからと思います。

[toki_td]

単にbreakで飛ぶ手法がgotoを使いたくないがために生まれて、(良いか悪いかはさておき)経験上こういった書き方もそこまで珍しいとは思わない、ということを言いたかっただけです。
例示のオリジナルを書いた人は関数を抜ける際のログを残したいのでreturnの代わりとして使ったんじゃないですかね？
組み込みなのでprintfデバッグの名残じゃないかと。

Answer 3

全体が見えないのでなんとも言えませんが、通常エラー処理は例外で捉えるようにすると思います。
また、すべての処理でbreakするならwhile()はいらないと思うのですが？

「追記」

２、メソッドの分割について

私は可能な限り小分けする方ですね。その方が見通しが良くなります。処理に結果は一つｄ＾＾
小分けして遅くなるような処理って、ドライバや組み込みでもなければ、今のCPUやコンパイラなら問題にならないと思います。

例えば、上の例だとIsValid()とmethod()で纏めて処理にして何が失敗したか復帰値で分かるようにするとか・・・

Comments

[reasoku]

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

プロジェクトとして基本的なエラーは例外で扱う仕組みにはしていないようで、本当に致命的なエラーだけ捕捉する作りになっています。

while(1)はループ目的ではなく、どの箇所のbreak後にも同じログ処理のメソッドを呼びたいらしく、このような実装にしているようです。
for(;;)で行っているものもありました。
なぜこのような実装をしているのかわからなかったため、C/C++のお作法的な書き方としてそのようなものが存在するのかを確認したく、このような質問をした次第です。

組み込み系なのですが、その場合はあまり細かくしないほうが良いのでしょうか？
cateye様がおっしゃっている通り、処理に結果がひとつであるべきだと思っています。
正直今プロジェクトに存在するコードが読みづらく、まず担当分のコードのメソッド分割からだけでも読みやすく直していければと思っておりまして…。

[cateye]

組み込み系であれば、割り込みやキュー制御などクリティカルな部分もあると思いますが、それ以外はあまり気にする事もないと思います。まず、ちゃんと動くコードを最適化は（くれぐれもデグレ起こさないように）その後で・・・私も最初は人が読めないようなコードを（いっぱいーー；）書いてましたが、ソースは読みやすいのが一番ですｄ＾＾