Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
4  
Gentlemen && / || Ladies! This has been the most enlightening code review ever. A first for me :) . I am learning from the best. –  Dexobox Mar 20 at 4:29
add comment

6 Answers 6

up vote 13 down vote accepted

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;
}
share|improve this answer
    
I would actually like to see you do this code, you sound like a pro :) also i would have a great example to start with for learning c++. I am a newbie so i would like to know how would i parse the negetive numbers/arguments ? You have a great approach to the problem , i thing you will be able to generate the most robust and complete code :P Do you accept the challenge? :P :) –  Dexobox Mar 20 at 3:27
1  
Asking for code to be written is normally off-topic on Code Review, but I've taken the bait. –  200_success Mar 20 at 5:00
    
Thankyou for taking the bait :P This is gonna help me soooooooooooo much. As for the input validation it looks PERFECT! based on just executing the code by passing the arguments you supplied in your comments above. The code is huge might take a while before i can digest it whole (because i am a noob) :). I appreciate you helping me out, you are AWESOME!I cannot reply on the code link you provided because apparently i dont have 50 reputation on CR. –  Dexobox Mar 20 at 5:15
1  
@Dexobox 200_success's answer uses the standard strtod function. Choosing and using the right standard function is very probably the right/correct/best way to do it. In contrast I posted another topic on how to parse it yourself; IMO it shows that doing it yourself (instead of reusing a standard function) not trivial, and takes a while to get it right. (It would have been simpler if had been integers instead of double-precision numbers: double-precision numbers have a more complicated syntax, because of the . and E characters). –  ChrisW Mar 20 at 15:05
add comment

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
share|improve this answer
    
Kudos ... great code review mate. I am learning so much in my very first program :). I will keep in mind the spacing of everything from now. I love this website and the creative criticism :) –  Dexobox Mar 20 at 4:20
add comment

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
    }
    
share|improve this answer
1  
WoW! that is some good insight on the best way to do this :) –  Dexobox Mar 20 at 3:16
add comment

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 = " // ...
share|improve this answer
    
I was really confused at the temperature ranges as the problem statement did not specify any values for the limits. I just wanted to see if the code would run on some limits. Interesting insight on error handling, and the unicode format tip. Thank You Very much. –  Dexobox Mar 20 at 4:27
add comment

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
share|improve this answer
    
You are right! the code was giving wrong output values. TY.. still looking for something that can break the code though. Applied the last fix to get correct output :) –  Dexobox Mar 20 at 3:06
add comment
  • #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.

share|improve this answer
    
This is really good stuff. I have a long list of things to keep in mind while coding c. You caught me :) i use "C" a lot :) –  Dexobox Mar 20 at 3:21
1  
Why should any int be involved at all? Just say what you mean, and use while (start <= end) as the termination condition. –  200_success Mar 20 at 3:23
    
@200_success: Where, exactly? –  Jamal Mar 20 at 3:25
    
I'm saying that nend is pointless. –  200_success Mar 20 at 3:26
    
@200_success: I know. I was pointing out the casting and used that as an example. –  Jamal Mar 20 at 3:28
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.