This is the code for my first CS1 project, I hope to be posting my code here to track my progress of learning the language of C++. I shouldn't be too shabby, since I know C considerably well and have reviewed C++ code here before.
Write a program that allows the user to enter a time in seconds and then outputs how far an object would drop if it is in freefall for that length of time. Assume that the object starts at rest, there is no friction or resistance from air, and there is a constant acceleration of 32 feet per second due to gravity. Use the equation:
$$ \text{distance} = \dfrac{\text{acceleration} \cdot\text{time}^2}{2} $$
rock.cpp
:
/**
* @file rock.cpp
* @brief Determines the height that a rock falls (in feet) after a given time
* @author syb0rg
* @date 9/8/14
*/
#include <iostream>
#include <cmath>
int main(void)
{
// initialize variables on declaration
double time = 0;
double acceleration = 32.174;
std::cout << "Enter the time in seconds that a rock will fall: ";
std::cin >> time;
double feetFallen = (acceleration * pow(time, 2)) / 2;
std::cout << "The rock fell " << feetFallen << " feet." << std::endl;
}
-
7\$\begingroup\$ Really, you need to add comments for each line. ;-) \$\endgroup\$rolfl– rolfl2014年09月11日 19:56:27 +00:00Commented Sep 11, 2014 at 19:56
5 Answers 5
Your code is both short and good, so there's not much to say.
Header organization
It's a good idea to keep your headers organized. Keeping them alphabetically ordered allows you to spot accidental duplicates.
main()
signature
You don't need to put void
in the parentheses. It's very much a C-ism, and is not necessary in C++. Empty parentheses also means the function takes no arguments.
const
variables and constants
acceleration
is a constant, and should be marked constexpr
to indicate it is a compile-time constant. Please note that constexpr
is only available in C++11 or later. If you have to use an earlier version, you can replace it with static const
for largely the same effect. feetFallen
should be marked const
, since you only read from it after it is initialized.
Marking variables const
and constants constexpr
limits the number of moving parts in your application, making it easier to reason about and keep correct.
Unnecessary comment
Your comment about initialization is not necessary. The fact that you initialize variables the same time they are declared is obvious from reading the code.
Invalid user input
time
will contain the value 0
if your user inputs non-numeric input. If you want to constrain the user to numeric-only input, you can loop until the user enters only a valid number. Inspired by this StackOverflow answer, your solution could look like this:
while(!(std::cin >> time)){
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
std::cout << "Invalid input. Please enter a number: ";
}
-
\$\begingroup\$ Didn't know about
constexpr
, good tip! As for that one comment... \$\endgroup\$syb0rg– syb0rg2014年09月11日 20:14:01 +00:00Commented Sep 11, 2014 at 20:14 -
\$\begingroup\$ @syb0rg Ah. I don't hang out in chat, so I didn't see that. You have my condolences. \$\endgroup\$Aurelius– Aurelius2014年09月11日 20:24:43 +00:00Commented Sep 11, 2014 at 20:24
-
\$\begingroup\$ It's fine, I didn't expect you to see that. But it is definitely a place you should hang out more ;) \$\endgroup\$syb0rg– syb0rg2014年09月11日 20:26:11 +00:00Commented Sep 11, 2014 at 20:26
Thanks to the associative property of multiplication, and the fact that you're working with all double
values here, you don't need parentheses around acceleration * pow(time, 2)
, so instead of:
double feetFallen = (acceleration * pow(time, 2)) / 2;
You can write:
double feetFallen = acceleration * pow(time, 2) / 2;
Of course, if some of the operands had been int
values, you'd have to be careful.
No
void
parameters are needed in C++, unlike in C.You can improve variable scope a bit more. If
time
is just to be inputted by the user, then you can declare it between the prompt and input.std::cout << "Enter the time in seconds that a rock will fall: "; double time; std::cin >> time;
In addition, consider accepting a command line argument if one is given (other than the executable). Otherwise, you can still have the user input a
time
viacin
.int main(int argc, char* argv[]) { double time; // the executable is considered an arg if (argc == 1) { std::cout << "Enter the time in seconds that a rock will fall: "; std::cin >> time; } // other args are given else { // take the second arg only // the string has to be converted time = std::atof(argv[1]); } // ... }
Assuming
acceleration
is not meant to change, you can make itconst
:const double acceleration = 32.174;
You probably can avoid a slight performance hit with
pow()
here by just doingtime * time
:double feetFallen = (acceleration * time * time) / 2;
-
\$\begingroup\$ For your
time * time
point - I avoided doing this becausepow()
handled "malicious" input better (such as me inputting a letter, a negative number, etc.) \$\endgroup\$syb0rg– syb0rg2014年09月11日 20:10:59 +00:00Commented Sep 11, 2014 at 20:10 -
2\$\begingroup\$ @syb0rg: Fair enough, and I did sort of figure you already knew this. Going by rolfl's (joking) comment, you could've commented about that. \$\endgroup\$Jamal– Jamal2014年09月11日 20:14:10 +00:00Commented Sep 11, 2014 at 20:14
If you really want the code to execute efficiently, you should write
double feetFallen = 0.5 * acceleration * pow(time, 2);
instead of
double feetFallen = (acceleration * pow(time, 2)) / 2;
This lets the compiler embed the constant (16.087) instead of two constants (0.5 and 32.174) in the binary, and saves a multiplication at runtime. This is true whether or not you declare acceleration
as const
or constexpr
— although it's good practice to mark it as a constant anyway. Personally, I would also rename acceleration
in a way that indicates its units.
You might think that writing time * time
would be more efficient than pow(time, 2)
, but it turns out that both Clang and GCC optimize pow(time, 2)
as a simple multiplication instead of a function call when -O1
is enabled. So, it's really just a matter of preference.
A small point added to what has already been said: don't use std::endl
, use '\n'
unless you are really sure that you want to force std::cout
to flush; and in that case also think about why you want it to flush.
std::cout << "The rock fell " << feetFallen << " feet.\n";
-
1\$\begingroup\$ Why wouldn't I want it to flush there? \$\endgroup\$syb0rg– syb0rg2014年09月12日 12:14:46 +00:00Commented Sep 12, 2014 at 12:14
-
3\$\begingroup\$ Might not make much difference here, but if a program prints 100,000 lines of text, it makes a difference in speed. \$\endgroup\$gnasher729– gnasher7292014年09月12日 13:19:13 +00:00Commented Sep 12, 2014 at 13:19