Looking for some feedback...

Hey all,

I've just finished my horsepower calculator and am looking for input. Since everyone I know who knows C/C++ doesn't check their emails for a week or so, I thought this was the most efficient route for getting feedback. The source is attached and needs to be compiled (It works in both MSVC++ .NET and DevCPP 4, so it should compile for most other compilers).

It works on the premise that the user is finding the output at the flywheel and that they are finding both the horsepower and torque (foot-pounds) at a specific RPM output (meaning that the numbers won't match up exactly to factory-rated specs because normally the torque peak is at a lesser RPM than the HP peak is). I'm thinking about doing more calcs for different engine dynamics based on what I get here, so please don't hesitate to give your input.

oops, I forgot to attach the source... ( ) here, it should work now

1. replace the stars() and wait() macros with functions. There is no reason in C++ to use macro-like functions.
In C, function-like macros are typically used where performance is critical, but in C++ this is achieved by using inline functions. Inline functions offer the speed improvements without the nasty side effects which you can get with function-like macros.

2. Tidy up the indentation - it looks a bit ragged in a few places.

3. Your global vars should be local to each function. Global variables should be avoided where possible.

4. 5252 is a magic number. Define it as
const int magic = 5252;
But give it a proper name, and a comment.

5. The while loop in main is very long, and adding more features will make it longer still. Consider shortening it by making the list of options into a function call, and moving the switch statement to another function
Eg.

Code:

void menu ( void ) {
    cout<<"What operation do you wish to perform?"<<endl<<endl
        <<"1 - Horsepower @ RPM"<<endl
        <<"2 - Torque @ RPM"<<endl
        <<"3 - RPM based on horsepower & torque output"<<endl
        <<"4 - Version info"<<endl
        <<"0 - Quit"<<endl<<endl;
}
void action ( int choice ) {
    switch (choice)
    {
    case 1 : hp_rpm();
        break;
    case 2 : tq_rpm();
        break;
    case 3 : hp_tq();
        break;
    case 4 : ver_info();
        break;
    default:
        cout<<"Error, bad input, quitting"<<endl<<endl;
        break;  // good idea for default to have break as well
    }
}

In main
    menu();
    cin >> choice;  // this should really be validated
    if ( choice != 0 ) {
        action(choice);
    } else {
        cout<<"Please wait, halting program..."<<endl<<endl;
        wait()
        return 0;
    }

Thanks, Salem. I'm not a wizard with C++ (as you can see ), but automobile mathematics has been said to be my forte'... Like I said in my original post, I'm thinking on coding some more like this only just a little more robust... (engine dynamics and aerodynamics for calculating intake curve, injector pulse, etc...)

I think more reading on C++ programming ethics is in order. Just because it compiles and works doesn't mean it is completely correct.

by the way, I'm compiling this in MSVC++.NET so the indentation is a little awry in some places... Sorry for any inconveniences

I've split the menu and the actual action functions, but I encountered a warning - the app still compiled correctly - it's just that warnings annoy me and the fact that the code won't compile correctly on some other compilers leaves that irking feeling at the back of my mind. Anyway, I've narrowed it down to one main function, being the action() or switchmain() as I've labelled it. The error says

Code:

warning C4715: 'switchmain' : not all control paths return a value

Now, I'm thinking that since it has a switch scenario inside the function returning a value (case 0 : return 0) and that anything outside the case scenario is just being left to void(). Since I can't just go and label it void switchmain() because the function itsself is returning a 0 in one part of it, can't I just place a return true; outside the case scenario to remedy that, just as long as I keep the function initiator to something like short int/int switchmain()?

Here's the code:

Code:

short int switchmain(int choice)
{
	switch (choice)
	{
	case 1 : hp_rpm();
		break;
	case 2 : tq_rpm();
		break;
	case 3 : hp_tq();
		break;
	case 4 : ver_info();
		break;
	case 0 : 
		cout<<"Please wait, halting program..."<<endl<<endl;
		wait();
			return 0;;
		break;
	default:
		cout<<"Error, bad input, quitting"<<endl<<endl;
	}
	return true;
}

I just want to know if I'm doing this right

Depends--if one of the cases has a return, they all should, _unless_ you call the menu function to make another choice from within the functions called in the switch from cases other than case 0--I didn't look further into your code than the last post so I don't know how you handle it. In the first scenario you could have everything but case 0 return a 1.

The value 0 is frequently equated to the value false in type boolean, and values not 0 to true.

> warning C4715: 'switchmain' : not all control paths return a value
Probably caused by the return inside the switch, which makes the break; unreachable.

Try something like

Code:

bool switchmain(int choice)
{
    bool result = true;
    switch (choice)
    {
    case 1 : hp_rpm();
        break;
    case 2 : tq_rpm();
        break;
    case 3 : hp_tq();
        break;
    case 4 : ver_info();
        break;
    case 0 : 
        cout<<"Please wait, halting program..."<<endl<<endl;
        wait();
        result = false; // flag we want to exit now
        break;
    default:
        cout<<"Error, bad input, quitting"<<endl<<endl;
    }
    return result;
}