3

単純なコード ブロックの繰り返しが避けられない場合があります。説明のために、次のコード例を使用します。

注: このコードは説明のみを目的としており、実際のコードはより大きく複雑です。また、エラーが含まれている可能性がありますが、この質問のポイントはそれではありません。

switch(cmd) {
    case CMD_BLOCK_READ:
        if(current_user != key) {
            ERROR("Access violation - invalid key!");
            res = CR_ACCESS_DENIED;
            break; 
        }
        if(current_state < STATE_BUSY) {
            WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
            res = CR_NOT_PERMITTED;
            break;
        }
        if(ioctl(fd, HPI_CTL_BR) != 0) {
            WARN("Handshake failed (%s). Aborted!", strerror(errno));
            res = CR_TIME_OUT;
            goto post_resp;
        }
        if(block_read(id) != 0) {
            ERROR("Failed to read %d block (%s)! Aborted!", id, strerror(errno));
            res = CR_FAIL;
            goto send_nop;
        }

        res = CR_SUCCESS;
        break;
    case CMD_BLOCK_WRITE:
        if(current_user != key) {
            ERROR("Access violation - invalid key!");
            res = CR_ACCESS_DENIED;
            break; 
        }
        if(current_state < STATE_BUSY) {
            WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
            res = CR_NOT_PERMITTED;
            break;
        }
        if(ioctl(fd, HPI_CTL_BR) != 0) {
            WARN("Handshake failed (%s). Aborted!", strerror(errno));
            res = CR_TIME_OUT;
            goto post_resp;
        }
        if(block_write(id) != 0) {
            ERROR("Failed to write %d block - %s. Command aborted!", id, strerror(errno));
            res = CR_FAIL;
            goto send_nop;
        }
        res = CR_SUCCESS;
        break;
    case CMD_REQ_START:
        if(current_state < STATE_READY) {
            WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
            res = CR_NOT_PERMITTED;
            break;
        }
        state = STATE_BUSY;
        if(ioctl(fd, HPI_CTL_BR) != 0) {
            WARN("Handshake failed (%s). Aborted!", strerror(errno));
            res = CR_TIME_OUT;
            goto send_nop;
        }
        if(block_read(id) != 0) {
            ERROR("Failed to read %d block (%s)! Aborted!", id, strerror(errno));
            res = CR_FAIL;
            goto post_resp;
        }
        res = CR_SUCCESS;
        break;
    }

    /* The remaining 28 or so similar commands */
}

ご覧のとおり、小さな違いとbreak/gotoステートメントの多用により、関数やインラインを使用することはできません。私が通常行うことは、いくつかのマクロを定義することです。

/* NOTE: DO NOT USE these macros outside of Big Switch */
#define CHECK_KEY(_key) \
   if(current_user != (_key)) \
   { \
      ERROR("Access violation!"); \
      res = CR_ACCESS_DENIED; \
      break; \
   }
#define CHECK_STATE(_state) \
   if(current_state < _state) \
   { \
      WARN("Command %s is not allowed in this state!", cmd_name[cmd]); \
      res = CR_NOT_PERMITTED; \
      break; \
   }

#define HANDSHAKE(_fail) \
   if(ioctl(fd, CTL_BR) != 0) \
   { \
      WARN("Handshake failed (%s). Aborted!", strerror(errno)); \
      res = CR_TIME_OUT; \
      goto _fail; \
   }

#define BLOCK_READ(_id, _fail) \
   if(block_read((int)(_id))!= 0) \
   { \
      ERROR("Failed to read %d block (%s)! Aborted!", (int)_id, strerror(errno)); \
      res = CR_FAIL; \
      goto _fail; \
   }

#define BLOCK_WRITE(_id, _fail) \
   if(block_write((int)(_id)) != 0) \
   { \
      ERROR("Failed to write %d block - %s. Aborted!", (int)_id, strerror(errno)); \
      res = CR_FAIL; \
      goto _fail; \
   }

..それらを使用して同じコードを記述します。コードははるかに小さくなり、(ほぼ間違いなく) 読みやすくなります。

switch(cmd) 
{
case CMD_BLOCK_READ:
   CHECK_KEY(key);
   CHECK_STATE(STATE_BUSY);
   HANDSHAKE(post_resp);
   BLOCK_READ(id, send_nop);
   res = CR_SUCCESS;
   break;
case CMD_BLOCK_WRITE:
   CHECK_KEY(key);
   CHECK_STATE(STATE_BUSY);
   HANDSHAKE(post_resp);
   BLOCK_WRITE(id, send_nop);
   res = CR_SUCCESS;
   break;
case CMD_REQ_START:
{
   CHECK_STATE(STATE_READY);
   state = STATE_BUSY;
   HANDSHAKE(send_nop);
   BLOCK_READ(id, post_resp);
   res = CR_SUCCESS;
   break;
}
/* The remaining 28 or so similar commands */
<..>

このコードは、古き良きCというよりもある種のスクリプト言語に似ており、非常に見栄えが悪いですが、読みやすさのためにそれを犠牲にしても構わないと思っています。

問題は、同様の状況にどのように対処するかです。よりエレガントなソリューションとベストプラクティスは何ですか?

PS一般的に、マクロとgotoステートメントは悪い設計の兆候であることを認めます。したがって、それらがどれほど悪いか、または私のプログラミング スタイルがどれほど貧弱であるかについて炎上する必要はありません。

4

3 に答える 3

4

Python のソース コードが組織の模範であると主張するつもりはありませんが、複雑なコードを単純化するために使用されているマクロの (IMHO) 良い例が含まれています。

Python のメイン ループは、バイトコードを実行するスタックベースの VM を実装します。これには、Python がサポートするオペコードごとに 1 つのケースを持つ巨大なスイッチケースが含まれています。オペコードのディスパッチは次のようになります。

case STORE_ATTR:
    w = GETITEM(names, oparg);
    v = TOP();
    u = SECOND();
    STACKADJ(-2);
    err = PyObject_SetAttr(v, w, u); /* v.w = u */
    Py_DECREF(v);
    Py_DECREF(u);
    if (err == 0) continue;
    break;

ここでTOPSECONDSTACKADJはすべて、スタック オブジェクトで動作するマクロとして定義されています。一部のマクロには#define、デバッグを支援するために使用される別の があります。すべてのオペコードはこのように記述されており、この種のミニチュア スクリプト言語でロジックを表現することで、各オペコードの実装をより明確にするのに役立ちます。

私の見解では、マクロを注意深く、賢明に、限定的に使用することで、コードの可読性が向上し、ロジックがより明確になります。あなたの場合、マクロがいくつかの小さいが重要な機能を隠している場合、マクロを使用して実装を標準化し、更新するコードの同じスニペットの複数のコピーがないことを確認すると便利です。

于 2013-02-05T12:55:12.063 に答える
1

投稿したコードでは、関数を使用できなかった理由はありません。これは、「Extract Function」リファクタリング パターンです。goto を処理するには、それらをメイン関数に残して、関数の結果に基づいて呼び出すかどうかを指定します。

http://www.refactoring.com/catalog/extractMethod.html

また、渡されていないマクロで変数を使用することで、本当に混乱を招きました。これは、それらを簡単に再利用できないことを意味し、すべてを長い手で書くよりも間違いなく悪いことです。マクロで使用されるすべてのものを渡した場合は、より便利です。次に、効果的に使用できるダックタイピング スタイルのコーディングを取得します。

また、C を使用しているため、マクロを「避ける」べきではありません。これらは、主にコード生成に非常に役立ちます。(つまり、文字列化と連結) 多くの C++ や、「マクロは悪」と言う人もいます。これは C です。マクロは悪ではありません。

于 2013-02-06T02:05:43.627 に答える
1

このような状況では、通常、ケースがデータで合理的に記述されているかどうかを検討し、データは単一の共通コード ブロックで処理されます。もちろん、いつでもできるわけではありませんが、可能な場合が多いです。

あなたの場合、次のような結果になる可能性があります。


#define IO_NOOP    0
#define IO_READ    1
#define IO_WRITE   2

struct cmd_desc { 
   int check_key;     /* non-zero to do a check */
   int check_state;
   int new_state;
   void* handshake_fail;
   int io_dir;
   void* io_fail;
};

const struct cmd_desc cmd_desc_list[] = {
   { 1, STATE_BUSY,  -1,         &&post_resp, IO_READ,  &&send_nop },  /* CMD_BLOCK_READ */
   { 1, STATE_BUSY,  -1,         &&post_resp, IO_WRITE, &&send_nop },  /* CMD_BLOCK_WRITE */
   { 0, STATE_READY, STATE_BUSY, &&send_nop,  IO_READ,  &&post_rep }   /* CMD_REQ_START */
};

const struct cmd_desc* cmd_desc = cmds[cmd];

if(cmd_desc->check_key) {
   if(current_user != key) {
      ERROR("Access violation - invalid key!");
      return CR_ACCESS_DENIED;
   }
}

if(cmd_desc->check_state != -1) {
   if(current_state check_state) {
      WARN("Command %s is not allowed in this state!", cmd_name[cmd]);
      return CR_NOT_PERMITTED;
   }
}

if(cmd_desc->new_state != -1)
   state = cmd_desc->new_state;

switch(cmd_desc->io_dir) {
   case IO_READ:
      if(block_read(id) != 0) {
         ERROR("Failed to read %d block (%s)! Aborted!", id, strerror(errno));
         res = CR_FAIL;
         goto *cmd_desc->io_fail;
      }
      break;

   case IO_WRITE:
      if(block_write(id) != 0) {
         ERROR("Failed to write %d block (%s)! Aborted!", id, strerror(errno));
         res = CR_FAIL;
         goto *cmd_desc->io_fail;
      }
      break;

   case IO_NOOP:
      break;
}

res = CR_SUCCESS;

注 移動ラベルには gcc の "Labels as Values" 拡張を使用しました ( http://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html )。標準 C では、代わりに関数ポインターを使用できますが、それにはコードの再編成が必要であり、そのための十分な情報がありません。

于 2013-02-05T13:30:57.893 に答える