我不得不用C写一个RPN计算器,作为我的家庭作业问题之一,但是如果有人能批评我的代码并提出改进建议,那就太棒了!我没有添加任何溢出错误或输入错误,但我最终会添加。
我的代码从命令行获取操作数和操作符(例如,34-将返回-1),并将其发送到使用pop和push函数的解码函数。
#include <stdio.h>
#include <stdlib.h>
void push(float stack[], float value, int *currStack)
{
int i = *currStack;
while (i != 0)
{
stack[i] = stack[i-1];
i--;
}
stack[0] = value;
*currStack += 1;
}
void pop(float stack[], char operation, int *currStack)
{
int i;
switch (operation)
{
case '+':
stack[0] = stack[1] + stack[0];
break;
case '-':
stack[0] = stack[1] - stack[0];
break;
case '*':
stack[0] = stack[1] * stack[0];
break;
case '/':
stack[0] = stack[1] / stack[0];
break;
default:
printf("Invalid character.");
break;
}
for (i=1;i<*currStack;i++)
{
float temp = stack[i];
stack[i] = stack[i+1];
stack[i+1] = temp;
}
*currStack -= 1;
}
int decode(char **instring, float *outval, int size)
{
int i=0, currStack=0;
float stack[size/2];
for (i=1;i<size;i++)
{
if (atof(instring[i]))
push(stack, atof(instring[i]), &currStack);
else
pop(stack, *instring[i], &currStack);
*outval = stack[0];
}
}
int main(int argc, char *argv[])
{
float result;
decode(argv, &result, argc);
printf("The answer is: %.3f\n", result);
return 0;
}
发布于 2015-02-05 20:26:22
用于(i=1;i<*currStack;i++) {浮温=堆栈我;堆栈我=堆栈i+1;堆栈i+1=临时;}
这比需要的要复杂得多。
float temp = stack[1];
for ( i = 1; i < *currStack; i++ )
{
stack[i] = stack[i+1];
}
stack[*currStack] = temp;
此版本具有完全相同的效果,但*currStack + 1
分配是原始的*currStack * 3 - 3
赋值(尽管这可能会下降到由编译器进行良好注册管理的*currStack * 2 - 2
分配)。
我们可以做得更好,因为在我们开始之前,我们并不关心stack[1]
上的值。
for ( i = 1; i < *currStack; i++ )
{
stack[i] = stack[i+1];
}
for
循环就足够了。我们根本不需要和temp
混在一起。现在这只是垃圾数据。
注:我不是在争论vnp的观点:复制是不必要的。我只是说,如果您做了复制,您实际上不需要复制stack[1]
值。当然,您不必在堆栈中的每个元素中复制一次stack[1]
。
default: printf("Invalid character."); break; }
你实际上不需要break;
。让它自己脱离default
的情况是没有问题的。它不会伤害任何东西,但我认为让default
(而不是break
)稍微启动它是更容易读的。
int解码(字符**指示,浮点*外型,整数大小)
您说decode
应该是return
a int
,但是您从不返回任何东西。您可以声明此类型为void
。
void decode(char **instring, float *outval, int size)
那就没人会指望它会归还任何东西。
浮动堆栈大小/2;for (i=1;i
你也许应该评论一下为什么这是可行的。请注意,这仅仅是因为argc
比它所使用的参数数量更大。这在您从i=1
开始的方式中隐式地显示出来,但可能更清楚。通常情况下,无论你做什么聪明的事情,你都应该评论来解释它。否则,您必须再次聪明地阅读代码。
对于(i=1;i
在循环的每一次迭代中,都为*outval
分配一个值,但只使用一次。您可以在循环之外这样做,具有相同的最终效果。
for ( i = 1; i < size; ++i ) {
double temp;
if ( temp = atof(instring[i]) ) {
push(stack, temp, &currStack);
} else {
pop(stack, *instring[i], &currStack);
}
}
*outval = stack[0];
您也不需要计算两次atof(instring[i])
。第一次省省吧。
我在if
/else
中添加了围绕语句的大括号。单个语句版本容易受到很难诊断的错误类型的影响。
有一种观点认为++i
比i++
更快。这并没有太大的区别,一个好的编译器应该能够优化它,但是使用前缀表示法并没有什么坏处。
您的代码不允许0.0
的值,在这种情况下将显示“无效字符”。然而,这是一个有效的值。另一种可能是
if ( temp = atof(instring[i]) || ! strcmp(instring[i], "0.0") ) {
这将允许0.0
,虽然它只支持一种格式。另一种选择是is_zero
函数,它可以进行更彻底的检查。
检查数字的字符串操作(就时间而言)比检查操作符要花费更多。检查操作符只需要检查两个字符就可以确定它是运算符。
if ( ! instring[i][1] && is_operator(in_string[i][0] ) {
pop(stack, *instring[i], &currStack);
} else if ( temp = atof(instring[i]) || ! strcmp(instring[i], "0.0") ) {
push(stack, temp, &currStack);
} else {
printf("Invalid argument: [%s].", in_string[i]);
}
然后只需要一个is_operator
函数:
int is_operator(char c) {
switch (c) {
case '+':
case '-':
case '*':
case '/':
return 1;
default:
return 0;
}
}
atof
语句返回一个double
,但您只使用float
变量。最好让stack
、outval
等保存double
值,而不仅仅是float
值。这也将节省隐式转换。注意,%lf
在……里面printf
期望一个double
也是。
发布于 2015-02-05 17:22:54
首先,堆栈顶部为0。这是非常非常规的,并导致许多不必要的数据移动。在currStack
上设置堆栈顶部将使代码更加简单。例如,push
变成了
*currStack++ = value;
和pop
case '+':
currStack[-2] += currStack[-1];
--currStack;
break;
等。
其次,pop
的意思就是,从堆栈中弹出一个值。您的函数执行算术操作,应该相应地重命名,apply_operation
。
最后,我建议将decode
签名更改为float decode(char **, int)
。通过大小效应返回结果会使代码更难推理,而且必须有一个严肃的原因才能做出这样的决定。尽可能利用返回值。
发布于 2016-10-13 10:22:46
如果我这样运行你的程序会怎么样?
rpn 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
我猜它会崩溃,或者它可能会做任何事情,因为它会调用未定义的行为。
这就是为什么不应该使用像size/2
数组大小这样的技巧,检查每个数组访问是否有效,即0 <= index <= arraylength
。因为C程序不能要求任意数组的长度,所以必须将数组作为元组(start, size)
或类似的东西传递。
https://codereview.stackexchange.com/questions/79738
复制相似问题