我正在尝试自己学习C ++。经过一些文字后，我查找了一个示例问题。虽然我希望有人查看我的代码。我基本上是在要求您打破它，以显示一些缺陷或我错过的某些事情。作为一个初学者，我竭尽全力对其进行改进，现在碰壁分析了代码的健壮性。

问题陈述：


在此挑战中，编写一个该程序采用三个参数，分别是：
开始温度（摄氏度），结束温度（摄氏度）和步长。以步长为单位打印出从开始温度到结束温度的表格；如果步长大小不完全匹配，您实际上不需要
打印最终最终温度。您应该执行输入验证：不接受
开始温度低于下限（您的代码应
指定为常数）或高于上限（您的代码
也应低于）指定）。您不应允许步长大于温度差。


#include <iostream>
#include <cstdlib>
#include <string>

#define LOWER_LIMIT 23.3
#define UPPER_LIMIT 256.3

using namespace std;

bool isnum(string s)
{
    //check if the string is a number
    //48 & 57
    int len = s.length();
    for(int i = 0; i < len; i++)    
    {
        //cout << s[i] << "\t" << int(s[i])<< endl;
        if(int(s[i])>=48 && int(s[i])<=57)
            return true;
        else
        {
            return false;
            break;
        }
    }
}

int main(int argc, char** argv)
//The int argc holds the argument count and the argv is a 2-D array4
// of characters
{
    double start,end,step_size;
    if(argc!=4)
    {
        cout<<"Please enter three intigers"<<endl;
        cout<<"celcius <start_temprature> <end_temprature> <step_size>"<<endl;
        cout<<"Last step may not be printed"<<endl;
    }
    else
    {
        //check if all the arguments are intigers
        if(isnum(argv[1]) && isnum(argv[2]) && isnum(argv[3]))
        {   
            cout<<argv[1]<<endl;
            cout<<argv[2]<<endl;
            cout<<argv[2]<<endl;
            start = atof(argv[1]);
            end = atof(argv[2]);
            step_size = atof(argv[3]);
            //calculate the table and print.
            if(start < LOWER_LIMIT || start >UPPER_LIMIT)
            {
                cout<<"The <start_temprature> does not meet the limit requirement ("<<LOWER_LIMIT<<"\u00B0"<<"C - "<<UPPER_LIMIT<<"\u00B0"<<"C)"<<endl;
                // The degree symbol to be printed on command line requires UTF-8 characters which has the degree symbol and the location is \u00B0
                return -1;
            }

            if(end >UPPER_LIMIT || end <LOWER_LIMIT)
            {
                cout<<"The <end_temprature> does not meet the requirement ("<<LOWER_LIMIT<<"\u00B0"<<"C - "<<UPPER_LIMIT<<"\u00B0"<<"C)" <endl;             
                return -1;
            }

            if (step_size  <1 || step_size >=(UPPER_LIMIT - LOWER_LIMIT))
            {   //zero or negetive stepsize checking
                cout<<"The step_size cannot be negetive, zero or greater than or equal to step_size"<<endl;
                return -1;
            }

            if(end<start) //swapping variables if start is greater than end
            {
                cout <<"Swapped! end and start values for simplicity" <<endl;
                double tmp = start;
                start = end;
                end = tmp;
            }
            cout << "start "<<start<<endl;
            cout << "end "<<end<<endl;
            cout << "step_size "<<step_size<<endl;
            int nend = (int)((end-start)/step_size);
            cout << nend <<"number of iterations"<<endl;
            for(int i = 0; i < nend ;i++)
            {
                cout << start << "\u00B0"<<"C  =  " << ((start*(9/5))+32) << "\u00B0"<<"F" <<endl;
                start += (double)step_size;
            }

        }
        else
        cout <<"All three input arguments must be positive numbers!" <<endl;
    }
}


评论

先生们&& / ||女士们！这是有史以来最启发性的代码审查。对我来说是第一个:)。我正在向最好的人学习。

#1 楼

可自动检测的错误

首先，让编译器进行一些检查：

$ clang++ -Wall -c cr44821.cpp
cr44821.cpp:26:1: warning: control may reach end of non-void function
      [-Wreturn-type]
}
^
1 warning generated.


用户体验

我找到了程序令人气愤地使用。我最初几次尝试运行该程序均失败。我本来希望它们都能产生合理的输出。

$ ./cr44821 -40 +40 0.5
All three input arguments must be positive numbers!
$ ./cr44821 0 +40 0.5
All three input arguments must be positive numbers!
$ ./cr44821 0 40 0.5
0
40
40
The <start_temprature> does not meet the limit requirement (23.3°C - 256.3°C)
$ ./cr44821 23.3 +40 0.5
All three input arguments must be positive numbers!
$ ./cr44821 23.3 40 0.5
23.3
40
40
The step_size cannot be negetive, zero or greater than or equal to step_size


我终于成功地获得了不完整的答复。 24.3°C发生了什么？我认为，合理的下限是-273.15°C（绝对零），上限是1.4e32°C（绝对热）。 （std::numeric_limits<double>::max()为9.9e307，该值太大。）

问题分解

main()正在进行大量操作。温度转换计算本身不属于该范围。此问题因功能而引起

$ ./cr44821 23.3 25 1
23.3
25
25
start 23.3
end 25
step_size 1
1number of iterations
23.3°C  =  55.3°F


评论

\ $ \ begingroup \ $
我实际上希望看到您执行此代码，您听起来像是专业人士:)我也将有一个很好的例子来学习c ++。我是新手，所以我想知道如何解析有争议的数字/参数？您有解决问题的好方法，我想您将能够生成最健壮和完整的代码：P您接受挑战吗？ ：P :)
\ $ \ endgroup \ $
– Dexobox
2014-03-20 3:27


\ $ \ begingroup \ $
要求编写代码通常在“代码审阅”中是不合时宜的，但我已经上当了。
\ $ \ endgroup \ $
– 200_success
2014年3月20日5:00在
\ $ \ begingroup \ $
谢谢你的诱饵：P这会帮助我很多。至于输入验证，它看起来很完美！通过传递您在上面的注释中提供的参数来执行代码。代码巨大，可能需要一段时间才能完全消化（因为我是菜鸟）:)。我很感谢您的帮助，您真棒！我无法回复您提供的代码链接，因为显然我在CR中没有50的声誉。
\ $ \ endgroup \ $
– Dexobox
2014年3月20日5:15


\ $ \ begingroup \ $
@Dexobox 200_success的答案使用标准的strtod函数。选择和使用正确的标准功能很可能是正确/正确/最佳的方法。相反，我发布了另一个主题，关于如何自己解析它。 IMO表示，自己做（而不是重用标准功能）不是一件容易的事，并且需要一段时间才能正确完成。 （如果使用整数而不是双精度数字，会更简单：由于。和E字符，双精度数字的语法更复杂）。
\ $ \ endgroup \ $
– ChristW
2014年3月20日15:05

#2 楼

这个问题已经非常详尽地介绍了，但是我还有一点要补充。


始终使用间距。有时您执行if(x>y)有时您执行if(x > y)，有时您执行if (x >y)。选择一个（不是if(x >y)并坚持使用。就个人而言，我更喜欢if (x > y)，因为该间距使操作员易于查找。


声明尽可能接近使用的变量。例如，您可以同时声明和定义start，end和step_size，这样可以清楚地在何处使用变量并提供更好的上下文。


不应该编译（似乎只有最新版本的clang能够捕获它）： br />就像使用<语句一样，如果它们中有更多空间，则阅读endl语句要容易得多：



代替if，我可能会返回cout。std::swap没有特别定义的含义作为大多数系统。实际上，在Linux上，它将是一个相当大的正数。


（在我看来）最好对所有块使用大括号（表示-1，EXIT_FAILURE，-1等）。但是，至少要缩进。很清楚，一段代码属于其上方的控制流。再次，请保持一致：

cout<<"The <end_temprature> does not meet the requirement ("<<LOWER_LIMIT<<"\u00B0"<<"C - "<<UPPER_LIMIT<<"\u00B0"<<"C)" <endl;


应为

std::cout << "..." << x << "..." << ...



通常应保留标准输出（通常在C ++中通过if访问）作为输出，并应使用标准错误（else）错误消息或调试。这样可以简化程序或使用程序的用户的输出处理。


如果while为空，则不能保证cout具有返回值。


返回然后中断是多余的。返回将已经杀死循环。


由于未修改cerr，因此应将其传递给isnum。就像现在的s一样，它将复制传递给它的字符串。也不需要强制转换为int，因为这将把char与char进行比较。这意味着理论上可以使用具有不连续字符的字符集。因此（并且更干净），应使用s而不是范围检查。


const reference中的循环已损坏。如果第一个字符是数字，则返回true；如果第一个字符不是数字，则返回false。否则，返回值是不确定的。这不是您想要的行为，因此您需要重组循环。在伪代码中：

for(int i = 0; i < nend ;i++)


评论

\ $ \ begingroup \ $
荣誉。伟大的代码审查伙伴。在我的第一个程序中，我学到了很多东西：)。我将牢记从现在开始一切的间隔。我喜欢这个网站和创造性的批评:)
\ $ \ endgroup \ $
– Dexobox
2014年3月20日4:20


#3 楼

您可以改进的地方：

错误




您最初打印的输入命令行参数看起来有问题。 
cout<<argv[1]<<endl;
cout<<argv[2]<<endl;
cout<<argv[2]<<endl;



看起来您要在这里打印argv[3]，但不小心放了2。


语法/样式
/>

请不要使用using namespace std;。这被认为是一种不好的做法，也是一种不良习惯。
使用std::isdigit()代替isnum()函数。
您不需要包括<cstdlib>或<string>标头（如果需要，则必须包括<string>您实现std::isdigit()）。

将变量声明放在单独的行中。 


double start,end,step_size;



摘自Code Complete，第2版，第3页。 759：


语句各行，代码从上至下读取，
而不是从上至下和从左至右。当您查找特定的代码行时，您的眼睛应该能够跟随代码的左
页边距。它不必仅仅因为每一行可能包含两个语句而深入每一行。



循环

>

您可以在整个程序中使用更多循环。


if(isnum(argv[1]) && isnum(argv[2]) && isnum(argv[3]))
{   
    cout<<argv[1]<<endl;
    cout<<argv[2]<<endl;
    cout<<argv[2]<<endl;
    ...



在此处使用简单的for循环即可减少代码。

for (int i = 0; i < 3; i++)
{
    if (std::isdigit(*argv[i])) std::cout << argv[i] << std::endl;
    else return -1; // or re-ask for user input
}




评论

\ $ \ begingroup \ $
哇！这是最好的方法的一些很好的见解：）
\ $ \ endgroup \ $
– Dexobox
2014年3月20日，3:16

#4 楼

该程序要求有上限和下限，但这些限制是：

#define LOWER_LIMIT 23.3
#define UPPER_LIMIT 256.3


...让我感到相当奇怪的选择。 

using namespace std;


我怀疑我是第一个提到它的人，但这通常被认为是一个糟糕的主意，最好避免。 
bool isnum(string s)
{
    //check if the string is a number
    //48 & 57
    int len = s.length();
    for(int i = 0; i < len; i++)    
    {
        //cout << s[i] << "\t" << int(s[i])<< endl;
        if(int(s[i])>=48 && int(s[i])<=57)
            return true;
        else
        {
            return false;
            break;
        }
    }
}


有很多方法可以使操作更简洁。例如，您可以使用类似以下内容的方法：

static const char digits[] = "0123456789";

return s.find_first_not_of(digits) == std::string::npos;


另一种可能性是使用std::isdigit中的<cctype>（或<ctype.h>）。我也（强烈）考虑支持负数，因为负温度在摄氏温度范围内相当普遍。

int main(int argc, char** argv)
//The int argc holds the argument count and the argv is a 2-D array4
// of characters

一个2D数组-它是一个指针数组，每个指针保存一个字符串的地址。尽管可以以某些方式使用它们，但是还有其他使用方式不适用于另一种方式。它们可能很容易混淆，但是这样做可能会导致以后出现问题。字符串正确。 

{
    double start,end,step_size;
    if(argc!=4)
    {
        cout<<"Please enter three intigers"<<endl;
        cout<<"celcius <start_temprature> <end_temprature> <step_size>"<<endl;
        cout<<"Last step may not be printed"<<endl;


注释中的拼写也很重要。 :-)编译器可能并不在意，但通常您应该计划主要为人类读者编写，而其次才是针对编译器。

        //check if all the arguments are intigers


对于“错误”，例如这并不会真正影响程序的正常运行，我认为（至少通常）最好完全不打印错误消息。

            if(end<start) //swapping variables if start is greater than end
            {
                cout <<"Swapped! end and start values for simplicity" <<endl;


您可能想使用argv进行交换。 > 
                double tmp = start;
                start = end;
                end = tmp;


评论

\ $ \ begingroup \ $
在温度范围内，我真的很困惑，因为问题陈述没有指定任何限值。我只是想看看代码是否会在某些限制下运行。有关错误处理的有趣见解，以及unicode格式的技巧。非常感谢你。
\ $ \ endgroup \ $
– Dexobox
2014年3月20日4:27在

#5 楼

我不会全部审查：仅发表以下一条评论。

您是否测试了它并验证了输出是否正确？您可以断言应该产生的代码）是：0C为32F； -40C是-40F;而100C是212F。我认为这是一个错误：

((start*(9/5))+32)


(9/5)的值是1（整数除法舍入误差）。相反，您需要

(((start*9)/5)+32) // less rounding error: a good approximation


或

((start*(9.0/5))+32) // floating point arithmetic: will have decimal places


评论

\ $ \ begingroup \ $
你说得对！代码给出了错误的输出值。 TY ..仍然在寻找可以破坏代码的东西。应用了最后一个修复程序以获得正确的输出:)
\ $ \ endgroup \ $
– Dexobox
2014年3月20日下午3:06

#6 楼



#define宏在C语言中比在C ++中更常见，您应该改用常量：

const float LOWER_LIMIT = 23.3;
const float UPPER_LIMIT = 256.3;


main()做得太多。理想情况下，您应该让它处理输入/输出和函数调用。对于其他所有内容，请将它们放入带有描述性名称的单独函数中。这将有助于使您的代码看起来更简洁，更易于遵循。

您正在这里进行C样式转换：

int nend = (int)((end-start)/step_size);


此在C ++中可能会出现问题，通常是不需要的。

在这种情况下，您可以使用static_cast<>：信息。



评论

\ $ \ begingroup \ $
这真是好东西。在编码c时，我要记住很多事情。你抓到我了:)我经常使用“ C” :)
\ $ \ endgroup \ $
– Dexobox
2014年3月20日，3:21
\ $ \ begingroup \ $
为什么要一个int参与其中？只需说出您的意思，然后使用while（start <= end）作为终止条件即可。
\ $ \ endgroup \ $
– 200_success
2014-3-20的3:23
\ $ \ begingroup \ $
@ 200_success：到底在哪里？
\ $ \ endgroup \ $
– Jamal♦
2014年3月20日，3:25
\ $ \ begingroup \ $
我说nend没有意义。
\ $ \ endgroup \ $
– 200_success
2014年3月20日，3:26
\ $ \ begingroup \ $
@ 200_成功：我知道。我指出了强制转换并将其用作示例。
\ $ \ endgroup \ $
– Jamal♦
2014年3月20日，3:28