@Wander_nyankoposted at 2022-12-09

C言語　ネストの深い処理をリファクタする方法

Discussion

聞きたいこと

例えば下記のようなCのソースがあるとします。
こういったソースの可読性を向上させたい場合どのようにしたら良いのか、皆様のご意見をお聞きしたいです。
（変えないほうが可読性が高い、等の場合にもその理由を教えて頂きたいです）

if(0 < A) && (A < MAX){
 if(0 < B) && (B < MAX){
  if(0 < C) && (C < MAX){
   if(0 < D) && (D < MAX){
    if(0 < E) && (E < MAX){
     if(0 < F) && (F < MAX){   
      if(0 < G) && (G < MAX){
       if(XYZ != NULL){
        hoge();
       }
      }
     }
    }
   }
  }
 }
}

自分は基本的にアーリーリターンでネストを深くしないように努めているんですが、
上記のようなソースはアーリーリターンするにしても下記のようになってしまうので、
これはこれでどうなん？って思ってしまいます。

if(A <= 0) || (MAX <= A){ return; }
if(B <= 0) || (MAX <= B){ return; }
if(C <= 0) || (MAX <= C){ return; }
if(D <= 0) || (MAX <= D){ return; }
if(E <= 0) || (MAX <= E){ return; }
if(F <= 0) || (MAX <= F){ return; }
if(G <= 0) || (MAX <= G){ return; }
if(XYZ == NULL){ return; }
hoge();

気軽なコメントお待ちしています。

@SaitoAtsushi posted at 2022-12-09

どんな書き方も良いだけとか悪いだけということはありません。ある状況ではよい方法でもある状況ではよくないということは当然にあります。どれも悪い中で比較的マシなものを選ぶということもあります。結局のところは総合的に考える必要があるのでこうすれば改善するという単純なコツを示すことは出来ません。

あくまでもネストを避けようとするときに可能な選択肢のひとつとしてですが、配列として括り出すのは良いアイデアのひとつになる場合はあると思います。

// 各変数の型が int であると仮定してコードで事例を示します。
int *var_table[] = {&A, &B, &C, &D, &E, &F, &G};

for(size_t i=0; i<(sizeof var_table/sizeof int); ++i) {
  if(not ((0 < *var_table[i]) && (*var_table[i] < MAX))) return;
}
if(XYZ == NULL) return;
hoge();

ただ、同じような操作をするにも関わらず独立した変数がたくさんあるという状況が根本的な設計のまずさの表れである可能性を私なら考えます。本来的にひとまとまりのグループなのであれば (個々の場面でまとめるよりは) 最初から配列として管理したほうが良いでしょう。

逆にそれぞれの変数が意味的に全く独立しているのなら (結果的に同じ操作をするのであっても) 愚直に並べたほうが意味を理解しやすいです。

@Loofeht posted at 2022-12-10

本質とは関係ないとは思いますが,処理が等価でない気が...

if((0 < A) && (A < MAX))

をみたさない条件式は

if((A <= 0) || (MAX <= A))

であるかなと..

提示していただいたケースだと,
そもそも同じような判定処理をするのにそれがまとめられていないのがネストが深くなり,
見通しが悪くなっている原因のように思います.

上記指摘の通りそれぞれの変数が意味的に独立している場合は,
関数を作成して何をチェックしているかというのを,より明示的にするのもありではないかなと.

bool is_within(int value,int lower,int upper){
    return (lower<value) && (value<upper);
}
void func(){
    int A,B,C,D,E,F,G;
    void *XYZ=NULL;
     /*
     ここで上記変数を更新する?
     */
     const int lower=0;
     const int upper=MAX;
    
     if( !(is_within(A,lower,upper) &&
        is_within(B,lower,upper) &&
        is_within(C,lower,upper) &&
        is_within(D,lower,upper) &&
        is_within(E,lower,upper) &&
        is_within(F,lower,upper) &&
        is_within(G,lower,upper))
     ){
        return;
    }
    if(XYZ == NULL){
        return;
    }
    hoge();
}

@HalHarada posted at 2022-12-10

if(XYZ != NULL){
   if(0 < A) && (0 < B) && ...{   
      if (A < MAX) && (B < MAX)...{
          hoge();
      }
   }
}

orで判定するほうが少し早いかも？
やはり、可読性を考えるなら、関数を作成することが王道なのでしょう！

if (0 < min(A, B, C, D, E, F, G)) {
    if (max(A, B, C, D, E, F, G) < MAX) {
        if(XYZ != NULL){
             hoge();
        }
    }
}

@etsuhisa posted at 2022-12-10

下限が0または1、上限が2^N-1(255や1023など)のケースに限れば、ビット演算を使う方法があります。
可読性はわかりませんが、短くはなります。

r1は、すべての変数をORビット演算したものと、MAXの反転したビットをANDビット演算することで、符号ビット、MAXを超えた部分のビットに1があるかを判定します。
r2は、下限が1の場合で0を除外するため、いずれかの変数に0が含まれているか判定します。

#define MAX 63 //最大値は2^N-1に限る
int func(int A, int B, int C, int D, int E, int F, int G)
{
        int r1 = (A | B | C | D | E | F | G) & (~MAX); //負の値、MAXを超える値が含まれるか
        int r2 = !(A && B && C && D && E && F && G); //0が含まれるか
        return r1 || r2;
}

@hon_no_mushi posted at 2022-12-11

自分は基本的にアーリーリターンでネストを深くしないように努めているんですが、
上記のようなソースはアーリーリターンするにしても下記のようになってしまうので、
これはこれでどうなん？って思ってしまいます。

ここで何が引っ掛かっているのか、次第ではございますが…

考慮するべきポイントは、このあたりでしょうか。

(可読性) ネストが深くなりすぎて、読みにくくならない。
(保守性) hoge()が呼び出されてない時、原因が突き止めやすい
(保守性) 条件が追加・削除されたときに、修正しやすい
(性能) パフォーマンスへの影響が小さい

これらのどれを優先するのか次第で、より適切なコードは変わってしまいます。

商業的な（業務的な）プログラミングという観点でいうと、下記のどちらかですかね…

改善案1

それぞれ、A,B,Cごとに判定をしながら、それらのANDを取っていく。

もし判定を満たさなくなった場合に、どこの行で問題が起きたのかをトレースしやすい、というメリットがあります。

  // 改善案1
  bool ret = true;
  ret  = (( 0 < A ) && ( A < MAX ));
  ret &= (( 0 < B ) && ( B < MAX ));
  ret &= (( 0 < C ) && ( C < MAX ));
  ret &= (( 0 < D ) && ( D < MAX ));
  ret &= (( 0 < E ) && ( E < MAX ));
  ret &= (( 0 < F ) && ( F < MAX ));
  ret &= (( 0 < G ) && ( G < MAX ));
  ret &= ( XYZ != NULL );
  if ( ret == false )
  {
    // fprintf(stderr, "[TRACE][%d], A=%d, B=%d, C=%d, D=%d, E=%d, F=%d, G=%d, XYZ=%p\n", __LINE__, A,B,C,D,E,F,G,XYZ);
    return;
  }
  hoge();

改善案2

改善案１だと、A～G,XYZを全部判定しないとretが確定しません。これが非効率となる場合もあります。

仮にAを判定した時点でfalseだとわかれば、それ以後B,C,D...の判定は本来不要です。

よって、1行に判断をまとめてしまえば、それだけ判定は早められます（短絡評価）。

短絡評価についてはこちらをご覧ください。

https://qiita.com/gyu-don/items/a0aed0f94b8b35c43290

  // 改善案2
  const bool ret =
    (( 0 < A ) && ( A < MAX )) &&
    (( 0 < B ) && ( B < MAX )) &&
    (( 0 < C ) && ( C < MAX )) &&
    (( 0 < D ) && ( D < MAX )) &&
    (( 0 < E ) && ( E < MAX )) &&
    (( 0 < F ) && ( F < MAX )) &&
    (( 0 < G ) && ( G < MAX )) &&
    ( XYZ != NULL );
  if ( ret == false )
  {
    // fprintf(stderr, "[TRACE][%d], A=%d, B=%d, C=%d, D=%d, E=%d, F=%d, G=%d, XYZ=%p\n", __LINE__, A,B,C,D,E,F,G,XYZ);
    return;
  }
  hoge();

サンプルコード（クリックすると展開されます）

// https://qiita.com/Wander_nyanko/questions/dfc4ff1dc20be5710745
// gcc main.c -o a.out

#include <stdio.h>
#include <stdbool.h>

#define MAX 100

static bool is_hoge_called = false;
#define SET_HOGE_CALLED(v) { is_hoge_called = v; }
#define GET_HOGE_CALLED()  (is_hoge_called)

void hoge()
{
  SET_HOGE_CALLED( true );
}

void callHoge( int A, int B, int C, int D, int E, int F, int G, void* XYZ )
{
#if 0
  // 従来方法1
  if ( ( 0 < A ) && ( A < MAX) ) {
    if ( ( 0 < B ) && ( B < MAX) ) {
      if ( ( 0 < C ) && ( C < MAX) ) {
        if ( ( 0 < D ) && ( D < MAX) ) {
          if ( ( 0 < E ) && ( E < MAX) ) {
            if ( ( 0 < F ) && ( F < MAX) ) {
              if ( ( 0 < G ) && ( G < MAX) ) {
                if ( XYZ != NULL ) {
                  hoge();
                }
              }
            }
          }
        }
      }
    }
  }
#elif 0
  // 従来方法2
  if ( ! (( 0 < A ) && ( A < MAX )) ) { return; }
  if ( ! (( 0 < B ) && ( B < MAX )) ) { return; }
  if ( ! (( 0 < C ) && ( C < MAX )) ) { return; }
  if ( ! (( 0 < D ) && ( D < MAX )) ) { return; }
  if ( ! (( 0 < E ) && ( E < MAX )) ) { return; }
  if ( ! (( 0 < F ) && ( F < MAX )) ) { return; }
  if ( ! (( 0 < G ) && ( G < MAX )) ) { return; }
  if ( XYZ == NULL ) { return ;}
  hoge();

#elif 0
  // 改善案1
  bool ret = true;
  ret  = (( 0 < A ) && ( A < MAX ));
  ret &= (( 0 < B ) && ( B < MAX ));
  ret &= (( 0 < C ) && ( C < MAX ));
  ret &= (( 0 < D ) && ( D < MAX ));
  ret &= (( 0 < E ) && ( E < MAX ));
  ret &= (( 0 < F ) && ( F < MAX ));
  ret &= (( 0 < G ) && ( G < MAX ));
  ret &= ( XYZ != NULL );
  if ( ret == false )
  {
    // fprintf(stderr, "[TRACE][%d], A=%d, B=%d, C=%d, D=%d, E=%d, F=%d, G=%d, XYZ=%p\n", __LINE__, A,B,C,D,E,F,G,XYZ);
    return;
  }
  hoge();
#else
  // 改善案2
  const bool ret =
    (( 0 < A ) && ( A < MAX )) &&
    (( 0 < B ) && ( B < MAX )) &&
    (( 0 < C ) && ( C < MAX )) &&
    (( 0 < D ) && ( D < MAX )) &&
    (( 0 < E ) && ( E < MAX )) &&
    (( 0 < F ) && ( F < MAX )) &&
    (( 0 < G ) && ( G < MAX )) &&
    ( XYZ != NULL );
  if ( ret == false )
  {
    // fprintf(stderr, "[TRACE][%d], A=%d, B=%d, C=%d, D=%d, E=%d, F=%d, G=%d, XYZ=%p\n", __LINE__, A,B,C,D,E,F,G,XYZ);
    return;
  }
  hoge();
#endif
}


typedef struct testcase{
  bool expectedResult;
  int val[7];
  void* xyz;
} testcase_t;

bool unitTest( testcase_t *_testcase )
{
  SET_HOGE_CALLED( false );
  callHoge(
    _testcase->val[0], // A
    _testcase->val[1], // B
    _testcase->val[2], // C
    _testcase->val[3], // D
    _testcase->val[4], // E
    _testcase->val[5], // F
    _testcase->val[6], // G
    _testcase->xyz     // XYZ
  );

  return GET_HOGE_CALLED() == _testcase->expectedResult;
}

int main(void)
{
  int dmy_xyz;
  testcase_t TestCase[] = {

    // Common
    { true,  {  1,  1,  1,  1,  1,  1,  1}, &dmy_xyz },

    // Test for A
    { false, {  0,  1,  1,  1,  1,  1,  1}, &dmy_xyz },
    { true,  { 99,  1,  1,  1,  1,  1,  1}, &dmy_xyz },
    { false, {100,  1,  1,  1,  1,  1,  1}, &dmy_xyz },
    // Test for B
    { false, {  1,  0,  1,  1,  1,  1,  1}, &dmy_xyz },
    { true,  {  1, 99,  1,  1,  1,  1,  1}, &dmy_xyz },
    { false, {  1,100,  1,  1,  1,  1,  1}, &dmy_xyz },
    // Test for C
    { false, {  1,  1,  0,  1,  1,  1,  1}, &dmy_xyz },
    { true,  {  1,  1, 99,  1,  1,  1,  1}, &dmy_xyz },
    { false, {  1,  1,100,  1,  1,  1,  1}, &dmy_xyz },
    // Test for D
    { false, {  1,  1,  1,  0,  1,  1,  1}, &dmy_xyz },
    { true,  {  1,  1,  1, 99,  1,  1,  1}, &dmy_xyz },
    { false, {  1,  1,  1,100,  1,  1,  1}, &dmy_xyz },
    // Test for E
    { false, {  1,  1,  1,  1,  0,  1,  1}, &dmy_xyz },
    { true,  {  1,  1,  1,  1, 99,  1,  1}, &dmy_xyz },
    { false, {  1,  1,  1,  1,100,  1,  1}, &dmy_xyz },
    // Test for F
    { false, {  1,  1,  1,  1,  1,  0,  1}, &dmy_xyz },
    { true,  {  1,  1,  1,  1,  1, 99,  1}, &dmy_xyz },
    { false, {  1,  1,  1,  1,  1,100,  1}, &dmy_xyz },
    // Test for G
    { false, {  1,  1,  1,  1,  1,  1,  0}, &dmy_xyz },
    { true,  {  1,  1,  1,  1,  1,  1, 99}, &dmy_xyz },
    { false, {  1,  1,  1,  1,  1,  1,100}, &dmy_xyz },
    // Test for XYZ
    { false, {  1,  1,  1,  1,  1,  1,  1}, NULL }
  };

  for ( int i = 0 ; i < sizeof(TestCase) / sizeof(TestCase[0]); i++ )
  {
    bool ret = unitTest( &(TestCase[i]) );

    printf("[%-6s] Expected %c : %4d %4d %4d %4d %4d %4d %4d %p \n",
      ( ret?"OK":"FAILED" ),
      ( TestCase[i].expectedResult?'T':'F'),
      TestCase[i].val[0], TestCase[i].val[1], TestCase[i].val[2], TestCase[i].val[3],
      TestCase[i].val[4], TestCase[i].val[5], TestCase[i].val[6],
      ( TestCase[i].xyz )
    );
  }
  return 0;
}

@culage posted at 2022-12-12

ifネストより早期リターンの方が可読性が高い理由は、ifネストだと以下のようになっている可能性を考えながらコードを読まなければならないからです。
以下サンプルのように1画面に収まるならともかくhoge();の部分が数十行あって、ifブロックが閉じられた後どうなっているか目に入らない状態でコードを読まねばならないとしたらゾッとしますよね。

if(0 < A) && (A < MAX){
 if(0 < B) && (B < MAX){
  if(0 < C) && (C < MAX){
   if(0 < D) && (D < MAX){
    if(0 < E) && (E < MAX){
     if(0 < F) && (F < MAX){   
      if(0 < G) && (G < MAX){
       if(XYZ != NULL){
        hoge();
       }
      }
     }
    } else {
      fuga();
    }
   }
   piyo();
  }
 }
}

C言語　ネストの深い処理をリファクタする方法

聞きたいこと

改善案1

改善案2

Your answer might help someone💌

Popular Questions

C言語 ネストの深い処理をリファクタする方法

聞きたいこと

改善案1

改善案2

Your answer might help someone💌

Popular Questions

C言語　ネストの深い処理をリファクタする方法