8
\$\begingroup\$

I made a simple simulation of a falling ball. Is it possible to somehow improve or optimize this code? What tips can you give for development?

#include <iostream>
#include <cstdlib>
#include <cmath>
using namespace std;

int timeFallingBall() {
    static int time(0);
    return ++time;
}
void fallingBall() {
    cout << "From what height do we drop the ball? (In meters): ";
    double h;                                           
    cin >> h;
    double v = 0, high = 0, s = 0, maxv = 0, g = 9.8;   
    int t = 0;
    double hi = h;
    while (h) {
        t = timeFallingBall();
        v = static_cast<double>(t) * g;
        high = (v * static_cast<double>(t)) / 2;
        h = hi - high;
        if (h <= 0) {
            switch (t) {
            case 1:
                cout << "After " << t << " second the ball has reached the ground! Maximum speed: " << maxv << " m/s";
                break;      
            default:
                cout << "After " << t << " seconds the ball has reached the ground! Maximum speed: " << maxv << " m/s";
            }
            break;
        }
        switch (t) {
        case 1:
            cout << "After " << t << " second, the ball is at a distance of " << h << " m from the ground at a speed: " << v << " m/s" << endl;
            break;
        default:
            cout << "After " << t << " seconds, the ball is at a distance of " << h << " m from the ground at a speed: " << v << " m/s" << endl;
            break;
        }
        if (v > maxv) {
            maxv = v;
        }
    }
}
int main() {
    system("chcp 1251>nul");
    fallingBall();
    return 0;
}
\$\endgroup\$
2
  • 5
    \$\begingroup\$ Note that the time it takes for a ball to hit the ground can be calculated exactly: t = std::sqrt(2 * h / g). \$\endgroup\$
    – G. Sliepen
    Commented May 2, 2021 at 11:19
  • \$\begingroup\$ I had this task in my Bechelor's. To quote my professor "this problem seems обжиоус but is actually somewhat difficult because of the nonlinearity". \$\endgroup\$
    – Vorac
    Commented May 3, 2021 at 6:46

4 Answers 4

11
\$\begingroup\$
  • using namespace std; is a bad habit to get into, as it can lead to name-collisions and other issues. It's best to explicitly qualify the names we need where necessary, e.g. std::cout.

  • A few more line-breaks would be useful in the code. Think of code with a specific purpose as a paragraph when writing text. For example, this could be considered one "paragraph", so should have an empty line after it.

     std::cout << "From what height do we drop the ball? (In meters): ";
     double h;                                           
     std::cin >> h;
    
  • We have several similarly named variables h, high and hi. This makes the code harder to understand.

  • Variables that don't change should be marked const. (e.g. g).

  • Variables should generally be declared as close to the point of use as possible, set to immediately useful values, and not reused (unless they consume significant resources). For example, t, v, high and h can all be declared where they are set inside the loop.

  • s does not appear to be used.

  • We have no way to reset the static time variable in timeFallingBall(). This means we can't call fallingBall a second time, to drop another ball. We can easily just increment t in the fallingBall function instead.

  • while (h) translates to while (h != 0). Since h is a double with a calculated value, it's a bad idea to compare it with an exact number. We should use while (h > 0.0) instead, or while (true) since we handle the exit condition inside the loop.

  • In C++, number literals have specific types: 0 is an int, 0.f is a float, 0.0 is a double. When comparing or assigning to variables, it's best to use the literal style of the correct variable type, rather than relying on conversions.

  • std::endl outputs a newline, but also flushes the output stream unnecessarily. We can output "\n" to get a newline without the flush.

  • Note that given the constant downwards acceleration, the maximum velocity is always going to be the velocity when the ball hits the ground. So we don't really need to keep track of it.

  • (Obviously the 1 second resolution makes the max speed output rather incorrect).


Applying the above, I'd probably go with something like:

void fallingBall()
{
    std::cout << "From what height do we drop the ball? (In meters): ";
    double h_start;
    std::cin >> h_start;

    if (h_start <= 0.0)
    {
        std::cout << "The ball is already on the ground!\n";
        return;
    }

    const double g = 9.81;

    double t = 0.0;

    while (true)
    {
        t += 1.0;

        const double h_current = h_start - 0.5 * g * t * t;
        const double v_current = g * t;

        if (h_current <= 0.0)
        {
            std::cout << "After second " << t << ", the ball has reached the ground! Maximum speed: " << v_current << " m/s\n";
            break;
        }

        std::cout << "After second " << t << ", the ball is at a distance of " << h_current << " m from the ground at a speed: " << v_current << " m/s\n";
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thanks for the advice. This is really very important to me. At the expense of using namespace std, it is said in the material in which I learn the language, why it is not worth doing this. I found out about this only after writing this code and could not notice it before posting. The rest of the tips I will try to apply them and improve the understanding of the code. Thanks for helping me improve my style code. \$\endgroup\$
    – TuleKr1s
    Commented May 2, 2021 at 10:45
  • 3
    \$\begingroup\$ This Stack Overflow question explains in detail why using namespace std is bad. \$\endgroup\$ Commented May 2, 2021 at 13:17
  • 2
    \$\begingroup\$ Why not put the logic inside if (h_current <= 0.0) after the while loop and instead change the condition to while (h_current > 0)? \$\endgroup\$ Commented May 2, 2021 at 18:13
3
\$\begingroup\$

@user673679's comments about style, var names, and the weirdness of having a static int inside a special function are all good so I won't repeat most of that.

You over-estimate the final speed by up to 0.9999... seconds of extra falling time past ground level, i.e. worst case error of almost 9.8 m/s if the previous timestep had the object above the ground by the smallest representable double that can result from the rounding error in hi - high when you subtract two nearby numbers1. A smaller simulation timestep, or some kind of interpolation when you detect the collision, would be much better. (The physics is simple here so you could interpolate exactly back from the height below ground, but even linear interpolation, i.e. assuming that the object moves at constant speed since the last step, would let you make a closer estimate of the time when it hit the ground, and thus to correct total falling time and thus speed.)

Or just solve the equation and calculate the correct final speed for this simple case; as @G. Sliepen commented, t = std::sqrt(2 * h / g), and from that t with constant acceleration you can simply calculate velocity. So v = sqrt(2 * h / g) * g, or taking g inside the sqrt and simplifying: v = std::sqrt(2 * h * g);

Note 1: Subtracting two nearby FP numbers is bad for numerical precision in general; the rounding error is called "catastrophic cancellation". But in this case it appears you only want to know when to stop, so hi <= high would avoid that. Or is it high <= hi? Without going back to check your code, I forget which one is which because their names don't distinguish them at all.

switch with duplicate code except for one letter bloats your code a lot.

A common lazy technique is ... second(s) to indicate that the reader should infer singular or plural based on the number. That seems appropriate and good enough when you're mostly interested in the physics simulation. @user673679 points out that in this case you could phrase it with ordinal numbers, using the number as a label for the interval to avoid the need to pluralize. "after second 2" is ok; a bit clunky to read.

switch is definitely the wrong approach for selecting between two things; that's what if/else is for. If you ever have a switch with one case and one default:, it should be an if/else instead. would be more compact, but you'd still be repeating yourself.

   if (t == 1)
       std::cout << ... << '\n';
   else
       std::cout << ...s << "blah blah\n";

Another approach is to just select the message, and keep the code the same. (The switch or if hopefully compile to asm that works this way, if your compiler is smart and notices that it's just different data passed to the same functions.)

    auto seconds_unit = (t == 1) ? "second" : "seconds";
    std::cout << "After " << t << seconds_unit << ", the ball is at a distance of " << h << " m from the ground at a speed: " << v << " m/s\n";

std::endl is only useful if you want to force a flush, even if output is fully buffered (e.g. redirected to a file). Output to a terminal will already flush automatically when you output a newline, so you can simply include a "\n" in a string literal if your output ends with a constant string instead of a number anyway. Even if you were doing something slow between prints, there'd be no need for std::endl to make sure interactive "progress updates" got shown when you wanted. That's why cout / stdout is line-buffered when it's connected to a terminal, not a file.

\$\endgroup\$
1
  • \$\begingroup\$ I don’t even know why I didn’t figure out such a simple way, thanks for reminding me about it! And also thanks for your comments, this is very important to me. \$\endgroup\$
    – TuleKr1s
    Commented May 3, 2021 at 15:11
3
\$\begingroup\$

In addition to what's been posted previously, I like to stress that such problems should separate input, "the real problem", and output into separate functions.

In your case, the print-as-you-go is more trouble than it's worth to isolate, but you can easily separate the user prompt and have a function accept a parameter. This makes it easy to call multiple times with fixed inputs for easy testing.

void falling_ball (double h_start);

int main()
{
    std::cout << "From what height do we drop the ball? (In meters): ";
    double h_start;
    std::cin >> h_start;
    falling_ball (h_start);
}

If you did also separate out the generated values from the formatted printing, it would facilitate automatic testing, and be more like "real code" where this is calculated for use in a game or simulation.

\$\endgroup\$
4
  • \$\begingroup\$ a lot of words that I do not understand. while I'm just starting and can't understand why all this is, but when I learn and become a smarter developer, I will definitely return to this post and understand. Thank you! \$\endgroup\$
    – TuleKr1s
    Commented May 3, 2021 at 22:26
  • \$\begingroup\$ What didn't you understand? Maybe we can give you some pointers. \$\endgroup\$
    – JDługosz
    Commented May 4, 2021 at 13:51
  • \$\begingroup\$ I do not fully understand why it is better to write to the variable in the main function and for the fallingBall function to take a parameter than to do everything at once in the fallingBall function \$\endgroup\$
    – TuleKr1s
    Commented May 4, 2021 at 13:55
  • 1
    \$\begingroup\$ Functions should do "one thing". Handling prompting the user and reading the terminal is a different problem, that has nothing to do with the falling ball calculation. Consider test code: you can write calls directly to fallingBall(3.5); fallingBall(12); without having to type them each time you test. \$\endgroup\$
    – JDługosz
    Commented May 4, 2021 at 14:14
1
\$\begingroup\$

Others have given you good advice on your style. I am far to rusty at C++ to comment either way. I suggest the approach below to actually solve the problem presented, and then apply the advice from others to make it good C++.

#include <iostream>
#include <cstdlib>
#include <cmath>

void falling_ball(double height)
{
    const double g = 9.81;
    double h = height;
    /* report progress of object falling under gravity 
       from height h every second until it reaches the ground
       Use the equations of motion: 
         v = u+at
         s = ut + 1/2 at^2  // assume u = 0
         => at^2 = 2s
         => t^2 = 2s/a
         => t = sqrt(2s/a)
         
     */
         
    double v = 0.0;
    double t = 0.0;
    double s = 0.0;

    do 
    {
        std::cout << "After " << 
             t << " seconds, the ball is at a distance of " << 
             h << " m from the ground at a speed: " << 
             v << " m/s" << std::endl;
        t ++; 
        v = (g*t);
        s = (0.5*(g*(t*t)));
        h = height - s;
    } while (h > 0.0); 
    t = std::sqrt(height*2/g);  //  calculate exact time of impact

    std::cout << "The ball reached the ground after " <<
         t << " seconds, at a speed of " <<
         (g*t) << " m/s" << std::endl;
}   

int main()
{
    std::cout << "From what height do we drop the ball? (In meters): ";
    double h_start;
    std::cin >> h_start;
    falling_ball (h_start);
}

The output looks like this:

From what height do we drop the ball? (In meters): 30
After 0 seconds, the ball is at a distance of 30 m from the ground at a speed: 0 m/s
After 1 seconds, the ball is at a distance of 25.095 m from the ground at a speed: 9.81 m/s
After 2 seconds, the ball is at a distance of 10.38 m from the ground at a speed: 19.62 m/s
The ball reached the ground after 2.4731 seconds, at a speed of 24.2611 m/s
\$\endgroup\$
1
  • 1
    \$\begingroup\$ don't use using namespace std; \$\endgroup\$
    – JDługosz
    Commented May 4, 2021 at 13:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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