gotoを使わずに２

質問

やっぱり全体がないとわかりませんよね・・・
とりあえず、コピーします。見にくいとおもいますが・・
中置法を逆ポーランドに変換するプログラムです。

void CPorandoView::OnButton1()
{
// TODO: この位置にコントロール通知ハンドラ用のコードを追加してください
CString ss;
char st[30],z[30],tai,ap; CEdit* myED1=(CEdit*)GetDlgItem(IDC_EDIT1); //EDIT1のアドレスをmyED1の代入
CEdit* myED2=(CEdit*)GetDlgItem(IDC_EDIT2); //EDIT2のアドレスをmyED2の代入
CEdit* myED3=(CEdit*)GetDlgItem(IDC_EDIT3); //EDIT3のアドレスをmyED3の代入
myED1->GetWindowText(ss); //入力された中置法式を作業領域ssに代入

int np=0,stp=0,zp=0,a=999,i=0,sw=0; char ka[30],s=0,kp=0; float st2[30]; float x=0;
char p[100]="" ;
char *N = new char[ss.GetLength()+1]; //ダイアログ(中置法入力枠)に入力された式をNに入れ込む
strcpy( N, ss );
myED1->SetWindowText("");
myED2->SetWindowText(""); myED3->SetWindowText("");

if(N[0]==NULL){
int ret;
ret=AfxMessageBox("入力がありません。",MB_OK|MB_ICONEXCLAMATION);

goto end;

}
if(30<strlen(N)){
int ret;
ret=AfxMessageBox("入力文字数オーバーです。",MB_OK|MB_ICONEXCLAMATION);

goto end;
}
for(;a!='\0';np++){ a=N[np];
tai=0;
if( a>0x003a){
if(a<0x007e ){
if(a!='*' && a!='/' && a!='+' && a!='-' && a!='(' && a!=')'){
int ret;
ret=AfxMessageBox("指定外文字が入力されています。",MB_OK|MB_ICONEXCLAMATION);

goto end;
}
}
}
if(a==0x0030){
int ret;
　　　　　ret=AfxMessageBox("0は入力不可です。",MB_OK|MB_ICONEXCLAMATION);//入力が0の場合エラーを表示

goto end;

}
if(a>'0' && a<='9'){
　　　　　if(z[zp-1]>='0' && z[zp-1]<='10'){ 　　　　　z[zp]=',';
zp++;
}
z[zp]=a; 　　　　　zp++;
z[zp]='\0'; 　　　　　sw--;
}else if(a!='(' && a!=')'){
if(stp==0){
st[stp]=a;
stp++;
st[stp]='\0';
sw++;
}else if(st[stp-1]!='('){
if(st[stp-1]=='*' || st[stp-1]=='/'){
tai=2;
}else{
tai=1;
}
}else{
st[stp]=a;
stp++;
}
if(a=='*' || a=='/'){
ap=2; 　　　　　sw++;
}else{
ap=1;
sw++;
}

if(sw>1){
int ret; ret=AfxMessageBox("演算子が連続しているか、\n先頭、または最後尾に演算子が入力されています。",MB_OK|MB_ICONEXCLAMATION);
goto end;
}
if(tai!=0){

if(ap<=tai){
z[zp]=st[stp-1]; 　　　　　　　　　　　　　　 st[stp-1]=a;
zp++;
z[zp]='\0';
st[stp]='\0';
if(a==st[stp-2]){ 　　　　　　　　　z[zp]=st[stp-2];
zp++; 　　　　　　　　　st[stp-2]=st[stp-1];
z[zp]='\0';
st[stp-1]='\0';
stp--;
}
}else{
st[stp]=a;
stp++;
st[stp]='\0';
}
}
}else if(a=='('){
st[stp]='('; stp++;
st[stp]='\0';
}else {
while(st[stp]!='('){、
stp--;
if(st[stp]!='('){
z[zp]=st[stp];
zp++;
}
}
}
}
if(stp>=0){
stp--;
while(stp>=1){
z[zp]=st[stp-1];
zp++;
z[zp]='\0';
stp--;
}
}

if(sw<=0){
int ret;
ret=AfxMessageBox("数値は１～９までです。",MB_OK|MB_ICONEXCLAMATION);//演算子がないとエラー処理

goto end;
}

z[zp+1]='\0'; myED2->SetWindowText(z); end:
;
}

回答No.5

BLK314

例外を使うのも手だと思います。
MFCなので、CException 派生を投げるようにすれば、
ReportErrorでダイアログが表示できますので
あちこちでメッセージボックスを開く手間が省けます。

正常系とエラー系が分離できるだけでも
プログラムがすっきりできると思います。

もちろん、他の人が指摘されているように
関数化も重要です。

関数化して例外を投げてcatchでReportErrorさせれば、
ぐんとメンテナンス性が向上すると思います。

いかがでしょうか？

回答No.4

php504

end:
の後に何もないので全部
return;
でいいんじゃないですか

回答No.3

Alice022

今後使用していくプログラムであるなら、他の方々がアドバイスして下さっているように、関数化させたほうが良いと思います。

これがただ勉強問題としての利用だけなら、このソースコードを活かすようにするのであれば、
BOOL型で変数何か用意し、TRUEで初期化もしておき、TRUEかFALSEでエラー判断させたループ処理させるのはどうでしょうか。

最初の処理のSetWindowTextは処理が被ってしまうので、そこは直す必要がありますが。

if(N[0]==NULL)　以降から　if(sw<=0)　までの処理を直すとして
-----
BOOL　Flags = TRUE;　//変数宣言

do{
z[zp+1]='\0'; myED2->SetWindowText(z);
　↑すみません、C++良く知らないので、ウインドウ表示は最初とgoto endで表示させてるのと共通化させて纏めてください。

　if(N[0]==NULL){
　　int ret;
　　ret=AfxMessageBox("入力がありません。",MB_OK|MB_ICONEXCLAMATION);
　　//エラーであればフラグセット
　　Flags = FALSE;
　}
　・・・省略します。goto end;の部分は全てフラグ設定する。

}while( Flags == FALSE );
-----
こういった感じにすれば、もとのソースを崩さずにいけるとは思いますが、ざっと見た感じでなので、できなかったら、すみません。

回答No.2

zwi

ざっと見た感じは、
(1)gotoじゃなくてreturn;で事足りる。
(2)機能ごとに関数化が必要。
(3)再帰呼び出しを使えばスッキリする。
(4)"("の数の対応とか入力が間違っていたらバグにハマる可能性が大きい。
(5)半年後、このプログラムは読むの大変かも。
(6)スタックの処理は他のコードと混ざらないように関数化かマクロで分りやすくしたい。

回答No.1

titokani

これは関数にするときれいになるパターンですね。
func()
{
　処理A
　処理B
　if(..){
　　処理C
　　goto end;
　}
　処理D
　end:
　処理E
}
というところを
func()
{
　処理A
　sub();
　処理E
}
sub()
{
　処理B
　if(..){
　　処理C
　　return;
　}
　処理D
}
}
とする。