6
\$\begingroup\$

I made a program in C++ where it calculates the distance between two coordinates. Is there anything to improve? Add? Make it more user-friendly?

Code:

 #include <iostream>
 #include <cmath>


using namespace std;

int ch;
double x;
double y;
double a;
double b;
double answer;

double distanceBetweenTwoPoints(double x, double y, double a, double b);

int main(){

        cout << "Enter the points for the coordinates";
        cout << endl;
        cout << "Point x for first coordinates: ";
        cin >> x;
        cout << endl;
        cout << endl;
        cout << "Point y for first coordinate: ";
        cin >> y;
        cout << endl;
        cout << endl;
        cout << "Point x for the second coordinate: ";
        cin >> a;
        cout << endl;
        cout << endl;
        cout << "Point y for the second coordinate: ";
        cin >> b;
        cout << endl;
        cout << endl;
        answer = distanceBetweenTwoPoints(x, y, a, b);
        cout << "The answer is " << answer;
}

double distanceBetweenTwoPoints(double x, double y, double a, double b){
return sqrt(pow(x - a, 2) + pow(y - b, 2));
}
\$\endgroup\$
  • 2
    \$\begingroup\$ For a beginner (first assignment) that has only just started it is fine. It can be made much better though but it will depend on what you have been taught so far. Do you know about structures? Do you know about stream state. Do you know how to overload functions. Do you know the difference between global and local variables. Do you know \$\endgroup\$ – Martin York Oct 18 '16 at 21:39
  • 1
    \$\begingroup\$ @Loki Astari: ... what a metric space is? :P \$\endgroup\$ – Dair Oct 19 '16 at 5:44
  • \$\begingroup\$ You seem to mix up the two word "point" and "coordinate". You have two points, each point has two coordinates x and y. \$\endgroup\$ – Stig Hemmer Oct 19 '16 at 7:32
12
\$\begingroup\$

I think @john has made some excellent comments. I agree with them, but you've already gotten them, so I'll try not to repeat the same.

Global variables

Variables defined outside any function, like these:

int ch;
double x;
double y;
double a;
double b;
double answer;

...are generally called "global", because anything, anywhere in the world program can depend on them and/or modify them.

When some parts of the code modify them and other parts depend on them, it can become difficult to figure out how the code will act under any given circumstances, how to get it to to exactly what you really want, and so on.

Usually, you want to restrict each variable to (approximately) the smallest scope necessary for it to do its job. None of these variables is used outside of the main function, so we can define them there to assure that only the code in main can use them:

int main(){
    double x;
    double y;
    double a;
    double b;
    double answer;

        cout << "Enter the points for the coordinates";
        cout << endl;
        cout << "Point x for first coordinates: ";
        cin >> x;
        cout << endl;
        cout << endl;
        cout << "Point y for first coordinate: ";
        cin >> y;
        cout << endl;
        cout << endl;
        cout << "Point x for the second coordinate: ";
        cin >> a;
        cout << endl;
        cout << endl;
        cout << "Point y for the second coordinate: ";
        cin >> b;
        cout << endl;
        cout << endl;
        answer = distanceBetweenTwoPoints(x, y, a, b);
        cout << "The answer is " << answer;
}

Avoid std::endl

Although it's used in a lot of tutorials and such, std::endl is almost never what anybody really wants. Although it won't make any real difference as you've used it here, using std::endl when (for example) you're writing lots of data to a file can easily make your program run 10x slower than it really needs to1.

Use functions

You may not have learned (much) about functions yet, but they can make your life much easier, largely by reducing repetition. For example, quite a bit of the code in main consists of a number of repetitions of the same basic sequence:

Ask user for input
Read the input

You can make your life considerably simpler by moving repeated actions like this into functions:

double get_value(char const *prompt) { 
    double value;

    std::cout << "\n" << prompt;
    std::cin >> value;
    return value;
}

Then the code in main to get values can be shortened a bit, to something like this:

cout << "Enter the points for the coordinates\n";
x = get_value("Point x for first coordinate: ");
y = get_value("Point y for first coordinate: ");
a = get_value("Point x for second coordinate: ");
b = get_value("Point y for second coordinate: ");

This not only reduces the amount of typing involved, but when you make a mistake (and we all do) it improves the chance that you've only made it in one place, so when you fix it, it'll be fixed in general.

Mathematics

You should usually think twice about computing a hypotenuse at all. In a lot of cases (e.g., comparing one distance to another) you can add the squares of the X/Y coordinates, but not take the square root of the result. For a lot of common cases like "find which point is closest to the one I clicked", the square of the distance works just as well as the actual distance, but is much faster to compute (sqrt is often relatively slow).

If you do need to compute a hypotenuse, consider using std::hypot instead of re-implementing it yourself. In the worst case, this will just be a thoroughly tested and debugged version of the code you'd write on your own. In a better case, it might use a different algorithm to compute the result. For example, you might have a case where both your \$\Delta x\$ and \$\Delta y\$ are within range and the final answer would be within range, but \$\Delta x^2 + \Delta y^2\$ is out of range. In this case, the simplistic approach will overflow, but some others won't. Depending on your hardware and/or required precision, alternatives may also be faster.

Avoid using namespace std;

This using directive brings a huge number of names into scope, which can lead to all sorts of problems from colliding with names you try to define. Almost nobody really knows all the names it defines, so it's often hard to avoid them.

It's (by far) easier in the long term to bite the bullet and put up with typing things like std::cout when you need to use them.

Note that this warning applies primarily (maybe even exclusively) to the std namespace. Something like using namespace std::chrono; is often perfectly fine.

If you are going to have a using namespace std;, you usually want to restrict it to the scope of a single function:

int main() { 
     using namespace std;

}

This can still lead to the same basic problems, but at least you've restricted the scope, so when a problem arises, cleaning up the mess is less work.


1. In case you care about the reason: making an operating system call to write data to a stream usually has a fair amount of overhead, so each call takes a fair amount of time, even to write a small amount of data. To avoid this, the code in the library stores data in a buffer. It only calls the OS to write the data to the external file once in a while, when the buffer gets full.

std::endl writes a new-line to the output stream (which you want) and it flushes the stream's buffer--forces an immediate OS call to write the data in the buffer, even if it's not full (which you almost never want). For interactive input like this, it won't actually hurt much, but as a simple rule of thumb, I'd just avoid using it at all. In the (rare) cases that you really do want to flush a stream's buffer, use std::flush to do it.

\$\endgroup\$
  • 2
    \$\begingroup\$ @Hurkyl: std::cin and std:cout are "tied" to each other by default, so if you write to cout then read from cin, cout will be flushed before the read from cin takes place. This happens automatically, without your using endl, flush or anything else to make it happen. So, while it's fairly harmless in this case, it's even less likely than usual to be useful. If you have other streams with a similar relationship, use std::ios::tie to enforce it instead of trying to flush at the right times (which is nit-picky/bug prone/un-maintainable). \$\endgroup\$ – Jerry Coffin Oct 19 '16 at 1:16
  • 1
    \$\begingroup\$ @Hurkyl: While it's true that if you want to force some output to be displayed, and you're not reading input, and not doing anything else that could force a flush, it may make sense to do something to explicitly force a flush. However, those are extremely narrow conditions, and when they do apply, it's (by far) best to use flush() to do the job and make it apparent to any reader that you're flushing the buffer intentionally. \$\endgroup\$ – Jerry Coffin Oct 19 '16 at 2:53
  • 1
    \$\begingroup\$ As far as tie "not working", you don't seem to have understood the implication of what it's saying. underflow() not being called happens if (and only if) there's already data in the input buffer to be read. So, in the code here, if the user enters "1.2 3.4 5.6 7.8" at the first prompt, the "3.4" can be read for the second input, without necessarily flushing the second prompt to display it. In this case, the user has already entered the data that's going to be read anyway, so it makes no real difference whether the second (and subsequent) prompts are displayed immediately or not. \$\endgroup\$ – Jerry Coffin Oct 19 '16 at 2:53
  • 2
    \$\begingroup\$ I'm trying to teach him to write what he needs. And no, this isn't premature optimization at all. It's simply avoiding a useless pessimization that hurts both readability and performance. I realize you've been preaching favor of using endl for years, so no, there's little chance of getting you to see the light--but at the very least, I can attempt to counteract your bad advice with better advice, and avoiding std::endl is exactly that. \$\endgroup\$ – Jerry Coffin Oct 19 '16 at 3:13
  • 4
    \$\begingroup\$ I'll note for the record, that this isn't just my idea either. It's nearly a direct quote of SL50, from the C++ Core Guidelines, which reads simply: "Avoid endl." In case you're willing to listen to argument from authority, since you've outright rejected merely solid reasoning, note that it was added by none other than Bjarne Stroustrup. But I should probably quit trying to convince you--you undoubtedly know C++ a lot better than he does. \$\endgroup\$ – Jerry Coffin Oct 19 '16 at 3:19
10
\$\begingroup\$

It's not completely horrible, but I do see a few things I'd suggest.

You should review your code, after calling it "finished" and remove any unused variable or routines. Just go back and tidy up a bit. So, I'd remove int ch;.

Why?

Two reasons: It wastes some memory, and it leads to questions in the reader's mind. Anyone who reads the code will think, "What happened to ch?" Did I miss it? Do I have all the code?

You don't want to create questions in the mind of your reader. You want it to be as crystal clear as you can make it.

So, along those lines, you've named your variables x, y, a, b. It's clear what X and Y are, but A and B are not typical names for points on the Cartesian Plane. Again, you'll create questions in the mind of your reader.

Go back to what you're trying to accomplish. You're trying to get the X and Y co-ordinates of two points: Point1 and Point2.

A better set of names would be:

double point1_x;
double point1_y;
double point2_x;
double point2_y;

After you get some experience, you'll realize that some concepts, like points on the Cartesian plane, lend themselves well to being something we call Objects.

It might be overkill for your skill level at this point, but you should look into some examples. Google "point Cartesian Plane c++"

I'd also try and tidy up main a bit.

Combine the I/O statements like this:

cout << "Enter the points for the coordinates" << endl;

etcetera...

\$\endgroup\$
  • \$\begingroup\$ To let you know why the int ch variable was there is because originally it was going to be a whole bunch of tools for mathematics. \$\endgroup\$ – BoeNoe Oct 18 '16 at 21:45
  • \$\begingroup\$ And thanks for your help. I really appreciated it! \$\endgroup\$ – BoeNoe Oct 18 '16 at 21:45
2
\$\begingroup\$

For simple squaring, you may want to avoid pow(x, 2) and instead use just x*x. The former implements a general exponential and may well be slower than a single multiplication.

\$\endgroup\$
2
\$\begingroup\$

This isn't bad for a beginner by no means. However, your code could use some improvements. I've commented on some of the things I think could be improved. Correct me if there are any mistakes or if there's something that needs elaboration:

#include <iostream>
#include <cmath>

// Avoid "using namespace" as it can conflict 
// with other dependencies. 

/* For example, if I had a function that passed a parameter for a std::Vector but
 * also had an external library that used the same keyword (such as SFML's sf::Vector),
 * there would be confusion about which one is being used in context.
 * 
 * 
 * void doSomething(Vector vec){
    // Which Vector is being used here? Is it "std::Vector" or "sf::Vector"?
    }
 * 
 * */

using namespace std;

// This can be deleted...there's no need to declare variables that won't be used.
// Also, it's best to avoid using global variables as they can be manipulated unintentionally.
// For this scenario, it would be better to declare in a local scope, such as the 
// main function. Also, consider using more descriptive variable names. Try not to
// leave any ambiguous to someone reading your code.
int ch;
double x;
double y;
double a;
double b;
double answer;          // This variable isn't necessary if you use a function. See comment below.


// This is the function prototype/declaration. It is necessary for the function be declared BEFORE
// it is called in the main function.
double distanceBetweenTwoPoints(double x, double y, double a, double b);

int main(){

        // All input can be handle in a function call...say inputValues().
        // You can also use escape characters instead of using std::endl.
        cout << "Enter the points for the coordinates";
        cout << endl;
        cout << "Point x for first coordinates: ";
        cin >> x;
        cout << endl;
        cout << endl;
        cout << "Point y for first coordinate: ";
        cin >> y;
        cout << endl;
        cout << endl;
        cout << "Point x for the second coordinate: ";
        cin >> a;
        cout << endl;
        cout << endl;
        cout << "Point y for the second coordinate: ";
        cin >> b;
        cout << endl;
        cout << endl;
        answer = distanceBetweenTwoPoints(x, y, a, b);
        cout << "The answer is " << answer;
}

// If you know about data structures, I would highly advised you to use them. They're very 
//helpful for organization
// If this was a method (class function), there would not be a need for parameters nor
// an "answer" variable. The function can be used in the variables place. Also, consider
// using raw "x*x" calculations opposed pow(). Using functions like these can cause
// some overhead (although it may be negligible here). 
double distanceBetweenTwoPoints(double x, double y, double a, double b){
return sqrt(pow(x - a, 2) + pow(y - b, 2));
}

Here's an attempt at an improved version using a class to hide data. I haven't tested it but I think it gets the general idea across:

#include <iostream>
#include <cmath>

// These can be used instead of "using namespace"
using std::cout;
using std::cin;

class DistanceBetweenTwoPoints;

int main(){

    // Create an instance of the DistanceBetweenTwoPoints
    DistanceBetweenTwoPoints distance;

    // Gets input
    distance.inputValues();

    // Displays the answer
    distance.displayAnswer();

    /*
     * Note that I didn't have to call the getDistance() function as
     * it's used internally by displayAnswer();
     * */

    // To see results by waiting for input
    std::cin.get();

    return 0;
}

class DistanceBetweenTwoPoints
{
public:
    // Constructor
    DistanceBetweenTwoPoints()
    {}



    // Get input values
    void inputValues()
    {
        std::cout << "Enter the points for the coordinates\n" << "Point x for first coordinates: ";     
        std::cin >> x_;
        std::cout << "\n\nPoint y for first coordinate: ";
        std::cin >> y_;
        std::cout << "\n\nPoint x for the second coordinate: ";
        std::cin >> a_;
        std::cout << "\n\nPoint y for the second coordinate: ";
        std::cin >> b_;

    }   

    // Display the answer calculated
    void displayAnswer()
    {
        std::cout << "\n\nThe answer is " << getDistance();
    }

private:
    // Data members
    double x_, y_, a_, b_;

    // Calculate the distance given the input values    
    // This is "private" because it is not called explicitly
    double getDistance()
    {
        return sqrt(pow(x_ - a_, 2) + pow(y_ - b_, 2));
    }
}

Hope this helps.

\$\endgroup\$
  • \$\begingroup\$ This is a gratuitously unnecessary class, complicating things for no reason other than a misguided idea that everything should be an object. Also, it encourages undefined behaviour: it makes no effort to ensure the user inputValues() before calling displayAnswer(). Default-initialising the members isn't a fix either, as again, this should not be a class. Making something into a class should solve a real-world problem, but this doesn't: a class should be able to perform a specific task well, & ideally to do so in many contexts, not just to maybe take input from console & spit it back out. \$\endgroup\$ – underscore_d Nov 4 '18 at 18:47
1
\$\begingroup\$

In the source code, you can collapse a lot of the output statements to improve readability and so that they take up less vertical space. For example,

    cout << "Enter the points for the coordinates";
    cout << endl;
    cout << "Point x for first coordinates: ";
    cin >> x;
    cout << endl;
    cout << endl;
    cout << "Point y for first coordinate: ";
    cin >> y;
    cout << endl;
    cout << endl;
    cout << "Point x for the second coordinate: ";

can become

    cout << "Enter the points for the coordinates" << endl
         << "Point x for first coordinates: ";
    cin >> x;
    cout << endl << endl;
    cout << "Point y for first coordinate: ";
    cin >> y;
    cout << endl << endl << "Point x for the second coordinate: ";

There is a fair amount of room for art and style in formatting, so don't take any particular suggestion as "you must do it this way" — in fact, I've mixed a couple different styles into the above example. The goal is to write code for which it is easy to see what the intention is, and without much clutter.

You can improve readability further by realizing you don't need the endl, and can simplify to printing newline characters:

    cout << "Enter the points for the coordinates\n"
         << "Point x for first coordinates: " << flush;
    cin >> x;
    cout << "\n\n";
    cout << "Point y for first coordinate: " << flush;
    cin >> y;
    cout << "\n\nPoint x for the second coordinate: ";

The meaning of the things I've changed are:

  • Writing a newline character (\n) allows buffering to continue
  • Inserting endl writes a newline character and then flushes the buffer
  • Inserting flush flushes the buffer without writing a newline character

Generally speaking, buffering is good for performance, so it's better to write newline characters rather than inserting endl when you don't actually need flushes to happen. As a bonus, they're less typing and usually easier to read.

Of course, I/O performance is basically irrelevant here because the human delay far outweighs any other concern — but it's good to get in the habit of writing what you mean early on.

Thus, the above code snippet demonstrates the general pattern: I use newline characters to write newlines, and I do something to flush the stream at the point I want to ensure that all of the output has been written. Using endl to do the flush isn't an option here, if I want the flush to happen at the right place!

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