Speed, distance and time calculator

This is a speed distance and time calculator - the comments should explain most of the odd looking code if there is any. I am looking for suggestions to improve the efficiency of the program and how better to structure it - is my approach the best from a 'best practices', efficiency and OOP point of view.

So, specifically:

• How might I improve to fit in with common best practices?

• I this the most efficient method and structure?

• Could I add or improve functions so I could more easily add features like unit conversions?

I don't want to lose any functionality. The current program calculates speed distance and time as well as showing all working which adds quite a lot of strings to store information. I am still looking to improve these parts as well.

The main reason I am asking, is to avoid bad practices and find better ways of doing what I'm doing. I am a C++ beginner but I know quite a lot.

```
#include
#include

using std::cout;
using std::cin;
using std::string;
using std::endl;
using std::ostringstream;

class Math
{ // Math Class
public:
ostringstream s;

// SPEED DISTANCE AND TIME CALCULATION
double calcSpeed(double distance, double time, string& formula) // SPEED ~ Variable formula passed to the ufnction will be redefined
{
s ");
cin >> chosenCalc;

if (chosenCalc == 4)
{
quit = true;
return 0;
}

/ Conditions /

switch (chosenCalc) {
case 1:
p = &Math::calcSpeed; // Redefines p to explicit function at runtime according to conditions
firstParam = "distance"; // Explanation strings
secondParam = "time";
formula = "Speed = distance / time";
chosenCalcStr = "Speed"; // Type in formula calculated
break;
case 2:
p = &Math::calcDistance;
firstParam = "speed";
secondParam = "time";

Seems like you got a good grasp of the basic concepts in C++. Good for you! I assume that you are writing C++03. Here is what I suggest that you change to make the code more clear:

-
Your Math class should be a namespace. Currently, Math only has a single member variable, ostringstream s, which is shared between the various calc functions. This implies that subsequent calls to any calc functions will simply append to whatever is already in s. E.g.,

Math math;

std::string test1, test2, test3;
math.calcTime(1.0, 0.9, test1);
math.calcTime(2.0, 0.8, test2);
math.calcTime(3.0, 0.7, test3);

std::cout << "test1: " << test1 << std::endl;
std::cout << "test2: " << test2 << std::endl;
std::cout << "test3: " << test3 << std::endl;

This will actually output

test1: 0.9 / 1
test2: 0.9 / 10.8 / 2
test3: 0.9 / 10.8 / 20.7 / 3

which is probably not what you want. So, I suggest that you convert Math into a namespace and define ostringstream s locally in each calc function.

-
You're using both std::cout and printf. Stick to one of them. Since you are already invested in string streams, I suggest the former.

-
The quit variable is redundant. You're already returning from main when chosenCalc == 4 is true. Speaking of which, I suggest that you get in the habit of writing 4 == chosenCalc instead. This way, you will get a nice compiler error if you accidentally write 4 = chosenCalc.

-
You're using a switch statement. Excellent! Stick to it. Add a case 4 instead of having the if(chosenCalc == 4) before the switch. Even better, add a default case so that if the user presses 5, he gets a nice error message. E.g.:

default:
    std::cout << "Invalid input. Please try again." << std::endl;
    continue;

-
Cool that you have the hang of function pointers. They have a lot of uses. However, you can do without them in this case. Simply remove FuncChosen and call the calc functions directly inside the switch. Just move the / User Input / section up before the switch and you got all the arguments you need. Calling a function directly is also more efficient than calling it through a pointer (since it is one less layer of indirection).

You've already got some decent C++ code. I hope that the above will get you even further. Just comment if you need some elaboration of my suggestions.

Edit:: As per you comment, I've added a little extra. If you have access to a C++11 compiler you can:

-
Use std::to_string instead of std::ostringstream. E.g.,

double calcSpeed(double distance, double time, string& formula)
{
    formula = std::to_string(distance) + " / " + std::to_string(time);
    return distance / time;
}

-
Use list initialization (also known as uniform initialization). E.g., int chosenCalc{1}; instead of int chosenCalc = 1;. The former does not allow narrowing conversions whereas the latter does. This can catch subtle errors.

-
Prefer using over typedef. The former is more powerful.

Furthermore, you can also take advantage of the following C++14 feature:

-
Use auto return type deduction. E.g.,

auto calcSpeed(double distance, double time, string& formula)
{
    formula = std::to_string(distance) + " / " + std::to_string(time);
    return distance / time; // Return type is deduced to be double.
}

This is a minor detail in you case. It can be really useful for more complex return types.

Lastly, I want to present a modern alternative to using output parameters. Let me emphasize that it is just an alternative. The output parameter approach is fine and which one you chose is completely up to you. The alternative is to return multiple values. Let's look at

double calcSpeed(double distance, double time, string& formula)

which returns a double and outputs to a std::string. We can instead return a std::pair consisting of a double and a std::string. I.e.,

std::pair calcSpeed(double distance, double time)
{
    auto speed = distance / time; // Auto is deduced to double
    auto formula = std::to_string(distance) + " / " + std::to_string(time); // Auto is deduced to std::string.
    return make_pair(speed, formula);
}

Remember to #include . Note that I have also used C++11's auto keyword to deduce the type of speed and formula for us. Like I mentioned before, the auto keyword even works for the return type in C++14. So in C++14, we could even just write

auto calcSpeed(double distance, double time)
{
    auto speed = distance / time; // Auto is deduced to double
    auto formula = std::to_string(distance) + " / " + std::to_string(time); // Auto is deduced to std::string.
    return make_pair(speed, formula); // Return type is deduced to std::pair
}

You could even let go of the temporary variables and write

auto calcSpeed(double distance, double time)
{ return make_pair(distance / time, std::to_string(distance) + " / " + std::to_string(time)); }

This is probably as

StackExchange Code Review Q#68677, answer score: 6