4
\$\begingroup\$

My code compiles and executes. I would like to hear any comments or suggestions pertaining to my use of C++.

#include <iostream>
#include <math.h>

bool error_check(){
    if(!std::cin){
        std::cin.clear();
        std::cin.ignore(256,'\n');
        return true;        
    }
    else 
        return false;
};

int main(){

//This line outputs directions for the user to follow.
std::cout << "\nEnter real numbers for side lengths.\n" <<
                 "\nEnter zero for an unknown side length.\n" << 
                 "\nTo exit enter zero for all side lengths.\n" << std::endl;

while(true){

    //This line prompts the user to enter a side length.
    std::cout << "side a: "; 
    //This line initializes and stores user input into a variable for calculation.
    float sidea = 1; std::cin >> sidea;
    //This condtional branch handles bad user input which is in this case anything but real numbers.
    if(error_check()){
        std::cout << "\nYou must enter a real number.\n" << std::endl;      
        continue;
    }

    //This line prompts the user to enter another side length.
    std::cout << "side b: ";
    //This line initializes and stores user input into a variable for calculation.
    float sideb = 1; std::cin >> sideb;
    //This condtional branch handles bad user input which is in this case anything but real numbers.
    if(error_check()){
        std::cout << "\nYou must enter a real number.\n" << std::endl;      
        continue;
    }

    //This line prompts the user to enter a final side length.
    std::cout << "side c: ";
    //This line initializes and stores user input into a variable for calculation.
    float sidec = 1; std::cin >> sidec; 
    //This condtional branch handles bad user input which is in this case anything but real numbers.
    if(error_check()){
        std::cout << "\nYou must enter a real number.\n" << std::endl;      
        continue;
    }

    //This conditional branch calculates and outputs the unknown side length.
    if(sidea == 0){
        sidea = sqrt(pow(sidec,2) - pow(sideb,2));
        std::cout << "side a= " << sidea << "\n" << std::endl;
    }
    else if(sideb == 0){
        sideb = sqrt(pow(sidec,2) - pow(sidea,2));
        std::cout << "side b= " << sideb << "\n" << std::endl;
    }
    else if(sidec == 0){
        sidec = sqrt(pow(sidea,2) + pow(sideb,2));
        std::cout << "side c= " << sidec << "\n" << std::endl;
    }
    else{
        std::cout << "\nYou must enter zero for an unknown side length.\n" << std::endl;
        continue;
    }

    //This conditional branch checks to see if the user wants to exit. 
    if(sidea == 0 and sideb == 0 and sidec == 0)
        break;
    else
        continue;

}
return 0;
};
\$\endgroup\$
3
\$\begingroup\$

Holy comments Batman!

You have too many useless comments which makes your code harder to follow.

Comments should explain why something is done, not what is done.

Good code makes it obvious what is done, if you feel that you need to comment what is being done then your code can be improved instead.

I'm going to link an answer to another question that explains commenting in a very good way (IMHO).

Prefer && to and

This code if(sidea == 0 and sideb == 0 and sidec == 0) had me do a double take, the and totally threw me off as I'm very used to see && used instead.

Keep it simple

One simplification you can make is to change bool error_check() into float read_real(). You should also extract your exit critera into a function and use it as a loop condition instead of breaking out of an infinite loop. This makes it much more clearer and removes the need for the comment. You can also extract computations from the conditional block when calculating the unknown side.

Like this:

float read_real(){
    float ans;
    std::cin >> ans;
    if(!std::cin){
        std::cin.clear();
        std::cin.ignore(256,'\n');
        throw std::invalid_argument("Please enter a real number");
    }
    return ans;
};

bool should_exit(float a, float b, float c){
    return a == 0 && b == 0 && c == 0;
}

...

float sidea = 1.0f;
float sideb = 1.0f;
float sidec = 1.0f;
while(!should_exit(sidea, sideb, sidec)){
    try{
        std::cout << "side a: "; 
        sidea = read_real();

        std::cout << "side b: ";
        sideb = read_real();

        std::cout << "side c: ";
        sidec = read_real();

        // The unknown side must be 0, so this will work for all cases.
        float unknown = sqrt(abs(sidea*sidea + sideb*sideb - sidec*sidec));
        if(sidea == 0){
            std::cout << "side a= " << unknown << "\n" << std::endl;
        }
        else if(sideb == 0){
            std::cout << "side b= " << unknown << "\n" << std::endl;
        }
        else if(sidec == 0){
            std::cout << "side c= " << unknown << "\n" << std::endl;
        }
        else{
            throw std::invalid_argument("You must enter zero for an unknown side length.");
        }
    } catch (std::invalid_argument e){
        std::cout<<"\n Error: "<<e.what()<<std::endl;
    }
}
\$\endgroup\$
  • \$\begingroup\$ The absolute value is important because you need to compute \$a^2 + b^2\$ sometimes and \$c^2 - a^2\$ other times. \$\endgroup\$ – twohundredping May 12 '15 at 14:35
2
\$\begingroup\$
  1. All prompt messages could be replaced by predefined strings holding them:

    Instead of:

    //This line outputs directions for the user to follow.
    std::cout << "\nEnter real numbers for side lengths.\n" <<
                 "\nEnter zero for an unknown side length.\n" << 
                 "\nTo exit enter zero for all side lengths.\n" << std::endl;
    

    you could define:

    std::string prompt="\nEnter real numbers for side lengths.\n
                        \nEnter zero for an   unknown side length.\n
                        \nTo exit enter zero for all side lengths.\n";
    

    and then in main just write:

    //This line outputs directions for the user to follow.
    std::cout << prompt;
    
  2. Avoid the overuse of endl as it does much more (flushes buffers, etc.) than \n. If you need just new line, the latter, (\n), is enough. (this is: \n\n better than: ...\n"<< std::endl;)

  3. All trivial comments, especially on self explanatory, one line functions are overkill. Keep them for complex functions, expressions that cannot be figured out with one simple look.

  4. Skip empty lines, at least, after block openings and between function declaration and first line of their body code.

\$\endgroup\$
  • \$\begingroup\$ As this application is an interactive application, \n will also cause a flush every time so the behavior is identical. Please don't blindly give out this advice. See here for more information. \$\endgroup\$ – Emily L. May 10 '15 at 21:29
  • \$\begingroup\$ @Emily L. Apologies for the late response. As far as I know '\n', "\n" are just character and string literals, respectively (they are executed as part of the generated opcode). As for std::endl is a stream manipulator (a function that needs to be called, much slower). Regarding your remark: "When a stream is line buffered, characters are intended to be transmitted to or from the host environment as a block when a new-line character is encountered." , I get it, but again, the fact that both \n and endl have the same function doesn't mean they have same execution time. \$\endgroup\$ – Ziezi May 13 '15 at 19:45
  • \$\begingroup\$ Rule #1: Do not prematurely optimize. Rule #2: When faced with two roughly equal options (and they are in this case), choose the clearer one. In my opinion std::endl is much clearer. Now std::endl is a template with a small body, your compiler will inline it so you don't get the function call anyway. But rule #1 and #2 above should be the main motivator here. \$\endgroup\$ – Emily L. May 13 '15 at 21:38
  • \$\begingroup\$ @Emily L. Agreed! \$\endgroup\$ – Ziezi May 14 '15 at 5:35
1
\$\begingroup\$

In addition to the answers above, including

using std::cout;
using std::cin;
using std::endl;

at the beginning of your code would save you time and space by not having to retype 'std::' every time!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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