Print out table with start/end temperatures and step size

Question

I am trying to learn C++ by myself. I looked up a sample question after going through some text. Though I would like someone to review my code. I'm basically asking you to break it to show some flaws or some thing I missed. As a beginner I tried exhaustively to improve it and now hit a wall to analyse robustness of the code.

Problem statement:

In this challenge, write a program that takes in three arguments, a start temperature (in Celsius), an end temperature (in Celsius) and a step size. Print out a table that goes from the start temperature to the end temperature, in steps of the step size; you do not actually need to print the final end temperature if the step size does not exactly match. You should perform input validation: do not accept start temperatures less than a lower limit (which your code should specify as a constant) or higher than an upper limit (which your code should also specify). You should not allow a step size greater than the difference in temperatures.

#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;
    }
}

Answer 1

Automatically detectable bugs

First, let the compiler do some reviewing:

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

User experience

I found your program to be infuriating to use. My first few attempts to run the program all failed; I would have expected all of them to produce reasonable output.

$ ./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

I finally succeeded in getting an incomplete response. What happened to 24.3°C?

$ ./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

My frustration appears to be partly a consequence of poor number parsing, and partly a consequence of artificial temperature limits. Reasonable limits, I think, are −273.15°C (absolute zero) for the lower bound, and 1.4e32°C (absolute hot) for the upper bound. (std::numeric_limits<double>::max() is 9.9e307, which is too large.)

Problem decomposition

There is plenty of action going on in main(). The temperature conversion calculation itself does not belong there. This problem cries out for a function

double c_to_f(double celsius) {
    return celsius / 5 * 9 + 32;
}

Answer 2

This question has been covered very thoroughly, but I have a bit more to add.

---

Use spacing consistently. Sometimes you do if(x>y) sometimes you do if(x > y) and sometimes you do if (x >y). Pick one (that's not if(x >y) and stick with it. Personally, I prefer if (x > y) as the spacing makes the operator easy to find.

---

Declare variables as close to use as possible. For example, start, end, and step_size could all be declared and defined at the same time. This makes it clear where the variables are used and gives a better context.

---

You have a typo that actually shouldn't compile (seems only the newest version of clang catches it):

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

There's a missing < before the endl.

---

Just like with your if statements, it's much easier to read cout statements if they have more space in them:

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

---

Prefer std::swap over manual swapping. It's both clearer and cleaner.

---

Instead of -1, I would probably return EXIT_FAILURE. -1 does not have particularly well defined meaning as a return value on most systems. In fact, on linux, it will a rather large positive number.

---

Preferably (in my opinion) use braces for all blocks (meaning for if, else, while, etc). At a very minimum though, make sure to indent. It makes it clear that a piece of code belongs to the control flow above it.

---

Once again, be consistent:

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

Should be

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

---

Generally standard out (typically accessed through cout in C++) should be reserved for output and standard error (cerr) should be used for error messages or debugging. This allows for easier output handling for programs or users using your program.

---

isnum is not guaranteed to have a return value if s is empty.

---

Returning and then breaking is redundant. The return will kill the loop already.

---

Since s isn't modified, it should be passed by const reference. As isnum is now, it will copy the string passed to it.

---

The magic numbers 48 and 57 are confusing and can be avoided by doing s[i] >= '0' && s[i] <= '9'. There's also no need to cast to an int since this will be comparing a char to a char.

Note that the standard does not mandate the ASCII charset or something compatible be used. This means that in theory a charset with non contiguous characters could be in use. For this reason (and because it's cleaner/clearer), you should use std::isdigit instead of a range check.

---

Your loop in isnum is broken. You return true if the first character is a digit and return false if the first character is not a digit. Otherwise, the return is undefined. This is not the behavior you desire, and you need to restructure your loop. In psuedo-code:

for each character:
    if character is not a digit:
        return false
    return true

Answer 3

Things you could improve:

Bugs

• Your initial printing of the input command line arguments looks buggy.

  cout<<argv[1]<<endl;
  cout<<argv[2]<<endl;
  cout<<argv[2]<<endl;
  

  It looks like you meant to print argv[3] here, but accidentally put a 2.

Syntax/Styling

• Please don't use using namespace std;. It's considered a bad practice, and is a bad habit to form.

• Use std::isdigit() instead of your isnum() function.

• You don't need to include the <cstdlib> or <string> headers (you will have to include <string> if you implement std::isdigit()).

• Put the variable declarations to separate lines.

  double start,end,step_size;
  

  From Code Complete, 2d Edition, p. 759:

  With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

Loops

• You could use more loops throughout your program.

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

  Using a simple for loop here to reduce the code down a bit.

  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
  }
  

Answer 4

The program calls for upper and lower bounds, but these:

#define LOWER_LIMIT 23.3
#define UPPER_LIMIT 256.3

...strike me as rather strange choices.

using namespace std;

I doubt I'm the first one to mention it, but this is generally considered a poor idea, and better avoided.

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;
        }
    }
}

There are a number of ways to do this more cleanly. For one example, you could use something like:

static const char digits[] = "0123456789";

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

Another possibility would be to use std::isdigit from <cctype> (or <ctype.h>). I would also (strongly) consider supporting negative numbers, as negative temperatures are fairly common on the Celsius scale.

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

Sort of a technicality, but argv isn't really a 2D array--it's an array of pointers, each holding the address of a string. Though they can be used in some of the say ways, there are other ways of using one that don't apply to the other. They can be easy to confuse, but doing so can lead to problems later.

{
    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;

Nit-picky perhaps, but it's worth putting some effort into getting the spelling in your strings correct.

        //check if all the arguments are intigers

Spelling in comments counts too. :-) The compiler may not care, but you should normally plan on writing primarily for human readers, and only secondarily for a compiler.

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

For "errors" like this that don't really effect the normal operation of the program, I consider it better (at least as a rule) to not print out an error message at all.

                double tmp = start;
                start = end;
                end = tmp;

You might want to use std::swap to do the swapping though.

cout << start << "\u00B0"<<"C  =  " << ((start*(9/5))+32) << "\u00B0"<<"F" <<endl;

I'd try to separate the "\u00B0" out and give it a meaningful name:

static char const *degree_symbol = "\u00B0";

cout << start << degree_symbol << "C = " // ...

Answer 5

I won't review it all: just one comment as follows.

Have you tested it and verified whether the output is correct?

Some known-good test results (which you can assert your code should produce) are: 0C is 32F; -40C is -40F; and 100C is 212F.

I think this is a bug:

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

The value of (9/5) is 1 (integer division rounding error). Instead you want

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

or

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

Answer 6

• #define macros are more common in C than in C++, and you should instead use constants:

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

• main() is doing too much. Ideally, you should have it handle input/output and function calls. For everything else, put them into separate functions with descriptive names. This will help make your code look cleaner and easier to follow.

• You're doing a C-style cast here:

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

  This can be problematic in C++ and is generally unneeded.

  In cases such as these, you would use static_cast<>:

  int nend = static_cast<int>((end-start)/step_size);
  

  See here for more information.