逆ポーランド記法電卓

Question

改善点はありますか？

#include <stdio.h>
#include <string.h>
#define MAX 100

int stack[MAX]; //stackは配列を利用して実現
int top = 0;    //stackは最上部（データ実際に格納する場所）

int push(int x);
int pop();

//配列を利移用して、スタックデータ構造を実現する
// push : データの格納
int push(int x){
  if(top == MAX) return -1; //満杯なら-1をエラーとして返す
  stack[top] = x;   //データの格納
  top++;            //次に格納する位置へtopを移動（1つ増やす）
  return 1;
}

//pop : データ取り出し
int pop(){
  --top;              //現在データが格納されているところにtopを移す
  return stack[top];  //最上部のデータを取り出して返す
}

//operator : 演算子
int operator(mozi){
  int a,b;
  switch(mozi){
    case '+':
      a=pop();
      b=pop();
      push(a+b);
      break;
    case '-':
      a=pop();
      b=pop();
      push(a-b);
      break;
    case '*':
      a=pop();
      b=pop();
      push(a*b);
      break;
    case '/':
      a=pop();
      b=pop();
      push(a/b);
      break;
  }
}

int main(){
  char *p_push = "push";
  char *p_pop  = "pop";
  char *p_operator = "operator";
  char input[10];
  int su,mozi;

  for(int i=0; i<10; i++){
  printf("Input operation: push/pop/operator:");
     scanf("%s", input);
   if(strcmp(input, p_push) == 0){
     printf("Input data : ");
     scanf("%d", &su);
     push(su);
   }else if(strcmp(input, p_pop) == 0){
      printf("popped data : %d\n",pop(su));
    }else if(strcmp(input, p_operator) == 0){
        printf("Input data : ");
        scanf("%s", &mozi);
        operator(mozi);
      }else{
        printf("Stack is empty! ");
        break;
     }
  }
}

Answer 1

改善点はありますか？

質問が漠然としているので、一般的なコードの書き方だけ回答します。
実装が正しく動くかどうか、といったことは自力でテストして、
詰まったときに、具体的にまた質問してください。

---

さて、コードを見た第一印象ですが、
前半部はスッキリしてるのに、
後半部のmain関数、forループの部分が
ややゴチャゴチャしてます。

まあ電卓ならこれくらい大した問題ではないでしょうが、
もっと本格的な言語処理系の作成に進むときに、
分かりにくく、直しにくくなっていきます。

とくにC言語は手続き型言語なので、
深い入れ子が分かりにくくなりやすい原因です。
入れ子の数が五重、十重に増えたところをイメージしてください。

何重になったらリファクタリングするのか、意見は分かれますが、
少なくとも、際限なく入れ子が増えたら
理解不可能になるのは異論ないと思います。

またコードが小さいので、ずいぶん気が早いと感じられるかもしれませんが、
コードが大きくなった後で、壊さないように直すのは非常に大変なのです。
現実的には「直すより、一から書き直した方が早い」になりがちです。

---

if(a == 0){
  func_a();
}else if(b == 0){
  func_b();
}else if(c == 0){
  func_c();
}else{
  func_d();
}

では、具体的にどうすればいいのか。
上の例のように下請けの関数に切り出していくのが、
単純で分かりやすいと思います。

それから、else ifはインデントを掘り進まないで、
同じ高さにそろえてある方が読みやすいと思います。

さらに判定部分を関数に切り出して、
returnと組み合わせると、elseは消せます。
分岐が排他的でなくなった場合にとくに有効です。

func_select(){
  if(a == 0){
    func_a();
    return;
  }
  if(b == 0){
    func_b();
    return;
  }
}

このほか、関数を呼ぶ代わりに、変数にフラグを立てて、
後でスイッチ文で切り分ける手法もあります。
これは、1回関数を呼ぶだけではなく、複数回呼ぶときに、
フラグを再利用できる場合にはスッキリします。

行数が増えるのが気になるかもしれませんが、
分かりにくくてデバッグで詰まる時間の方が、
貴重だと考えています。

小さい関数に切り出していくと、個別にテストしやすくなります。
とくに本格的な言語処理系は複雑で難解になりやすいので、
部品化が有効になるジャンルだと思います。

---

最後に、電卓から先の、発展的な話題を言いますと、
本格的な言語処理系では、たとえば再帰下降構文解析（LL(1)）では、
言語仕様のBNFに合わせて、関数を書きくだすと分かりやすい。

また、オブジェクト指向で書くならビジターパターンで、
走査と処理の責務を分けたりしますし、
関数型のパーサコンビネータなどは、部品化の極地です。

そもそも言語処理系は、言語を単語に分解して、
解読していく仕組みなので、処理を分解して、
単語などの言語の単位と処理の単位を一致させると、理解しやすくなるのです。

Comments

[kakakaaka]

ありがとうございます。