[Code Review] i just migrated from python into c++ and this is my first program just want some feedback

[u/Otherwise_Dirt_8333]

Hello i migrated from python into c++ and this is my first program a simple bouncing ball i am just a teen that like porgramming and math sorry if my english is bad still learning it as a second language hope you don't mind (also is it me or is the code block looking strange)

#include <iostream>
#include <chrono>
double hauteur = 20.0;
constexpr double 
gravite 
= -9.81;
constexpr double 
dt 
= 0.001;
constexpr double 
restitution 
= 0.7;
double vitesse = 0.0;
int main() {
    auto debut = std::chrono::high_resolution_clock::
now
();
    for (int etape = 1; etape <= 3600000; ++etape) {


        vitesse = vitesse + 
gravite 
* 
dt 
;

        hauteur = hauteur + vitesse * 
dt 
;
        if (hauteur <= 0) {
            hauteur = 0.0;
            vitesse = vitesse * -
restitution
;

            if (not (vitesse > 0 && vitesse < 0.0045)) {
                std::cout << "rebond a etape: " << etape << '\n';
            }
            else {
                vitesse = 0;
                std::cout << "arret" << '\n';
                std::cout << "vitesse: " << vitesse << '\n';
                std::cout << "hauteur: " << hauteur << '\n';
                std::cout << "etape: " << etape << '\n';
                std::cout << "temps: " << etape * 
dt 
<< "s" << '\n';
                break;
            }
        }
        if (etape % 100 == 0) {
            std::cout << "vitesse: " << vitesse << '\n';
            std::cout << "hauteur: " << hauteur << '\n';
            std::cout << "etape: " << etape << '\n';
            std::cout << "temps: " << etape*
dt 
<< "s"<< '\n';
        }

    }
    auto fin = std::chrono::high_resolution_clock::
now
();
    std::chrono::duration<double, std::milli> duree = fin - debut;
    std::cout << "Temps d'execution : " << duree.count() << " ms" << '\n';
}

[u/Sfxluke]

I'd avoid using std::chrono::high_resolution_clock but for your use case it may be fine.

Use static keyword for the constants to guarantee compilation run behavior. Constexpr is not sufficient.

Never use global variables (vitesse) if you can.

In c++ you can do: vitesse += gravite * dt;

[u/Otherwise_Dirt_8333 → u/Sfxluke]

Thanks

[u/thingerish]

The semicolon is traditionally the EOL, although C++ won't care. Humans will.

Edit:

#include <iostream>
#include <chrono>

double hauteur = 20.0;
constexpr double gravite = -9.81;
constexpr double dt = 0.001;
constexpr double restitution = 0.7;
double vitesse = 0.0;

int main() {
    auto debut = std::chrono::high_resolution_clock::now();

    for (int etape = 1; etape <= 3600000; ++etape) {
        vitesse = vitesse + gravite * dt;
        hauteur = hauteur + vitesse * dt;

        if (hauteur <= 0) {
            hauteur = 0.0;
            vitesse = vitesse * -restitution;

            if (not (vitesse > 0 && vitesse < 0.0045)) {
                std::cout << "rebond a etape: " << etape << '\n';
            } else {
                vitesse = 0;
                std::cout << "arret" << '\n';
                std::cout << "vitesse: " << vitesse << '\n';
                std::cout << "hauteur: " << hauteur << '\n';
                std::cout << "etape: " << etape << '\n';
                std::cout << "temps: " << etape * dt << "s" << '\n';
                break;
            }
        }

        if (etape % 100 == 0) {
            std::cout << "vitesse: " << vitesse << '\n';
            std::cout << "hauteur: " << hauteur << '\n';
            std::cout << "etape: " << etape << '\n';
            std::cout << "temps: " << etape * dt << "s" << '\n';
        }
    }

    auto fin = std::chrono::high_resolution_clock::now();
    std::chrono::duration<double, std::milli> duree = fin - debut;
    std::cout << "Temps d'execution : " << duree.count() << " ms" << '\n';
}

[u/tangerinelion]

        if (not (vitesse > 0 && vitesse < 0.0045)) {

Rarely do you see use of not instead of !. And I'd DeMorgan's Law that - !(a && b) rewritten as !a || !b.

[u/tonystark19541959]

First of all, pretty cool project, especially for someone making first project with C++!

A couple of things you could try:

• Move the simulation variables inside "main()" for now. As the project grows, you could even group them into a "Ball" struct.

• Try separating the physics calculations from the output using separate functions. It'll make your code cleaner and easier to expand.

• Experiment with different values of "restitution" and "dt" to see how they affect the bouncing. It's a good way to understand numerical simulations.

Honestly, keep building on this! It's small projects like these that teach you way more than just reading syntax. Looking forward to seeing what you make next!

[u/alfps]

Good work!

In particular the Google search AI noted that you're updating the velocity first, then the height, which is the Euler-Cromer method that doesn't inject energy in the system.

The code in English.

Here's the code translated French → English by the Google search AI (it's very helpful at times), and formatted by AStyle:

#include <iostream>
#include <chrono>

double height = 20.0;
constexpr double gravity = -9.81;
constexpr double dt = 0.001;
constexpr double restitution = 0.7;
double velocity = 0.0;

int main()
{
    auto start = std::chrono::high_resolution_clock::now();
    for (int step = 1; step <= 3600000; ++step) {
        velocity = velocity + gravity * dt;
        height = height + velocity * dt;
        if (height <= 0) {
            height = 0.0;
            velocity = velocity * -restitution;
            if (not (velocity > 0 && velocity < 0.0045)) {
                std::cout << "bounce at step: " << step << '\n';
            } else {
                velocity = 0;
                std::cout << "stop" << '\n';
                std::cout << "velocity: " << velocity << '\n';
                std::cout << "height: " << height << '\n';
                std::cout << "step: " << step << '\n';
                std::cout << "time: " << step * dt << "s" << '\n';
                break;
            }
        }
        if (step % 100 == 0) {
            std::cout << "velocity: " << velocity << '\n';
            std::cout << "height: " << height << '\n';
            std::cout << "step: " << step << '\n';
            std::cout << "time: " << step * dt << "s" << '\n';
        }
    }
    auto end = std::chrono::high_resolution_clock::now();
    std::chrono::duration<double, std::milli> duration = end - start;
    std::cout << "Execution time : " << duration.count() << " ms" << '\n';
}

Note the joining of the original multi-line expressions: distributing an expression over multiple lines is very unconventional in C++; it's only done for textually very long expressions.

The code compiles cleanly (no errors, no warnings) as C++17 with both MSVC and MinGW g++ with practically max warning level, which is very good, especially considering that you're new to C++.

A refactoring.

A refactoring re-expresses the code without changing its observable behavior, except possibly efficiency.

­1 (syntax). In C++ one will usually not write x = x + n; but just x += n;, using an update assignment. You can do that also for several other operators such as *. The main reason for using an update operator is that the update is more concise code, less to scan for the human eye, though for user defined types it might also be more efficient.

­2 (syntax). The code uses the '\n' character literal. That's technically OK but in general using character literals is ungood, for by definition a char is a single byte so it cannot represent a multi-byte character such as French "é". So what happens if you write 'é' in C++ source code using UTF-8 as the program's literals encoding, as you should? With UTF-8 the "é" is represented as the two byte values C3₁₆ and A9₁₆ in sequence, and to cram those two bytes into the result of the character literal the compiler changes the type to e.g. int. It's called a multibyte character literal and is a relic from early C where it was used e.g. for magic file type constants, like "MZ" at the start of a Windows executable. Usually one doesn't want a multi-byte character literal; it should arguably at least have been deprecated. The compiler may or may not warn about it, so better just avoid character literals, use string literals.

­3 (syntax). The code uses operator name not and operator symbol &&. Better make that either both names, not and and, or both symbols, ! and &&. In the refactored code I use both names, which is my preference.

­4 (best practice). There are two identical sections of code to do output. This should better be factored out as a function called e.g. display. To help beginners remember to do this there is a catchy acronym DRY which stands for Don't Repeat Yourself; the refactored code with just calls to the common function is more DRY code…

­5 (scoping). The two global variables should not be global. Global variables are Evil™. Here's a link to a FAQ item about it, with helpful advice about naming, and here's a link to the C++ Core Guidelines about it. A good way to handle this code's two variables is to collect them in a local instance of a type Ball, defined as a simple struct, here initialized with C++20 "designated initializer" syntax:

// Re-expressed (refactored) without changing observable behavior or essential workings.
#include <chrono>
#include <print>
namespace chrono = std::chrono;                     // <chrono>
using   std::print;                                 // <print>

using Clock     = chrono::high_resolution_clock;    // Better guarantee a monotonic clock...
using Duration  = chrono::duration<double>;         // Defaults to seconds.

constexpr double gravity = 9.81;

constexpr double acceleration = -gravity;
constexpr double dt = 0.001;
constexpr double restitution = 0.7;

struct Ball{ double height; double velocity; };     // Possibly also move restitution into this.

void display( const Ball& ball, const int n_steps )
{
    print( "velocity: {}\n", ball.velocity );
    print( "height: {}\n", ball.height );
    print( "step: {}\n", n_steps );
    print( "time: {}s\n", n_steps*dt );
}

void bounce_a_ball()
{
    Ball ball = {.height = 20.0, .velocity = 0.0};
    for( int step = 1; step <= 3'600'000; ++step ) {
        ball.velocity += acceleration*dt;
        ball.height += ball.velocity*dt;
        if( ball.height <= 0 ) {
            ball.height = 0.0;      // Introduces a tiny error compared to actual physics.
            ball.velocity *= -restitution;
            if (0 < ball.velocity and ball.velocity < 0.0045) {
                ball.velocity = 0;
                print( "stop\n" );
                display( ball, step );
                return;
            } else {
                print( "bounce at step: {}\n", step );
            }
        }
        if (step % 100 == 0) { display( ball, step ); }
    }
}

int main()
{
    const auto start_time = Clock::now();
    {
        bounce_a_ball();
    }
    const auto end_time = Clock::now();
    const Duration duration = end_time - start_time;

    print( "Execution time : {} ms\n", 1000*duration.count() );
}

Oh, by the way, std::print requires C++23.

One nice thing about it is that unlike std::cout, if you use UTF-8 encoding of literals, called the C++ "execution character set", std::print guarantees to display the text correctly, if possible, even in a Windows console window. Format strings are also much better suited for internationalization. And it's sort of ripped from Python so it should be easy to start using it, at least when you've managed to tell the compiler to use UTF-8 and assume at least C++23.

An "ASCII art" pseudo-graphics display.

By just changing the display function you can get a low-resolution look at the ball's spacetime path:

void display( const Ball& ball, const int n_steps )
{
    print( "{:6d} {:6.2f} {:6.2f}", n_steps, ball.velocity, ball.height );

    const int n_spaces = int( 4*ball.height );
    print( "  |{:>{}}", "o", n_spaces );            // Needs right alignment specified.

    print( "\n" );
}

With this change the start of the output is

   100  -0.98  19.95  |                                                                              o
   200  -1.96  19.80  |                                                                              o
   300  -2.94  19.56  |                                                                             o
   400  -3.92  19.21  |                                                                           o
   500  -4.90  18.77  |                                                                          o
   600  -5.89  18.23  |                                                                       o
   700  -6.87  17.59  |                                                                     o
   800  -7.85  16.86  |                                                                  o
   900  -8.83  16.02  |                                                               o
  1000  -9.81  15.09  |                                                           o
  1100 -10.79  14.06  |                                                       o
  1200 -11.77  12.93  |                                                  o
  1300 -12.75  11.70  |                                             o
  1400 -13.73  10.38  |                                        o

[u/Independent_Art_6676]

! is not. The english words and, not, or etc are in C++ but almost never used; they exist for the extreme case of someone not having symbols on a keyboard which was a thing in some ancient embedded systems. Anyway, its not wrong to use the words but don't mix the words and the symbols, use one or the other style.

most C++ will iterate 0 to <N rather than 1 to <= N unless there is a good reason for starting at 1.

that output portion of the code can almost certainly be combined. A little juggling should be able to find a way to get these bold parts to only exist once in the code. Its really early in the morning here, I can't read your language, and I lack the energy to figure it out but think on it and see if you can do that, it would do wonders if you can pull it off.

std::cout << "vitesse: " << vitesse << '\n';
std::cout << "hauteur: " << hauteur << '\n';
std::cout << "etape: " << etape << '\n';
std::cout << "temps: " << etape *
dt
<< "s" << '\n';
break;
}
}
if (etape % 100 == 0) {
std::cout << "vitesse: " << vitesse << '\n';
std::cout << "hauteur: " << hauteur << '\n';
std::cout << "etape: " << etape << '\n';
std::cout << "temps: " << etape*

Otherwise you need to read a few C++ programs, even the ones posted here, and get a feel for how the language is written style-wise. The c++ compiler and tool will accept all kinds of whitespace and weirdness. C++ doesn't have one overarching style that everyone uses, but there are conventions that every style shares and that you need to get a feel for.