program efficiency if/elseif

Blanchy (25)
C++ noob here, my program works, but how can i make it MUCH less redundant and inefficient? 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
 

#include <iostream>
#include <iomanip>

using namespace std;

int main()
{
    float miles, gallons, ppg, mpg, tripc, cpm;
   
    cout<< "2013 Pure Michigan Winter Vacation Fuel Efficiency Calculation";
    cout<< "\n\nHow mant miles did you travel? ";
    cin>> miles;
    cout<< "How many Gallons of Gas did you use? ";
    cin>> gallons;
    cout<< "What was the price per Gallon of Gas? ";
    cin>> ppg;
    if (ppg <= 0.0)
    {
       cout<<"/nGas price Invalid";
       cin.get();
       cin.get();
       return 0;
    }
   
    mpg = (miles / gallons);
    tripc = (gallons * ppg);
    cpm = (tripc / miles);
   
    cout<< "\n\nYou got "<<fixed <<setprecision(3) <<mpg;
    cout<< " miles per gallon";
    cout<< "\nThis trip cost $"<<fixed <<setprecision(2) <<tripc;
    cout<< " at a cost per mile of $"<<fixed <<setprecision(6) <<cpm;
   
    if ((mpg > 34.75) && (cpm >= .0975))
       {
       cout<<"\n\nThis is a GREEN MACHINE automobile in fuel economy,";
       cout<<"\nand this was a PRICEY Trip.";
       cin.get();
       cin.get();
       return 0;
       }
    else if((mpg > 34.75) && (cpm < .0975))
       {
       cout<<"\n\nThis is a GREEN MACHINE automobile in fuel economy,";
       cout<<"\nand this was a REASONABLE Trip.";
       cin.get();
       cin.get();
       return 0;
       }
    else if((mpg < 34.75) && (mpg > 18.5) && (cpm < .0975))
       {
       cout<<"\n\nThis is a AVERAGE automobile in fuel economy,";
       cout<<"\nand this was a REASONABLE Trip.";
       cin.get();
       cin.get();
       return 0;
       }
    else if((mpg < 34.75) && (mpg > 18.5) && (cpm >= .0975))
       {
       cout<<"\n\nThis is a AVERAGE automobile in fuel economy,";
       cout<<"\nand this was a PRICEY Trip.";
       cin.get();
       cin.get();
       return 0;
       }
    else if((mpg < 18.5) && (cpm >= .0975))
       {
       cout<<"\n\nThis is a GAS GUZZLER automobile in fuel economy,";
       cout<<"\nand this was a PRICEY Trip.";
       cin.get();
       cin.get();
       return 0;
       }
    else if((mpg < 18.5) && (cpm < .0975))
       {
       cout<<"\n\nThis is a GAS GUZZLER automobile in fuel economy,";
       cout<<"\nand this was a REASONABLE Trip.";
       cin.get();
       cin.get();
       return 0;
       }
    
   
cin.get();
cin.get();
return 0;  
}
Disch (13742)
General rule of thumb: If you find yourself copy/pasting blocks of code... you're probably doing it wrong. (This of course is not always the case, but it is a decent guideline for beginners)

1) Don't create redundant 'reverse' conditionals.

If you have an 'if' statement checking one thing, you do not need your future 'else' statements to check the opposite. For example... 

1
2
3
 
    if ((mpg > 34.75) && (cpm >= .0975))  // line 35
    ...
    else if((mpg > 34.75) && (cpm < .0975))  // line 43


There is no need to check if cpm is < 0.0975 on line 43 because the 'else' already ensures that is true.


2) Don't create duplicate conditionals unless necessary.

Again referring to line 35 vs 43. Both are checking if mpg > 34.75. No need to do that twice. A better (but still bad) way is to put them in a single block:
1
2
3
4
5
6
7
 
if(mpg > 34.75)
{
    if(cpm >= 0.0975)
        ....
    else  // else ensures cpm < 0.0975 .. no need to have an 'if' here
        ....
}



3) Don't have every conditional exit

You copy/pasted code into all your if blocks. That's bad. You're also returning in each if block which is also bad. returning exits the function (since the function is main, it means it closes the program). There is no reason to do that here.

If you let all your if blocks "fall through" without returning, then the program will continue after them as normal. You can then have code that is run regardless of the previous 'if's. It's hard for me to explain... but here's an example:

1
2
3
4
5
6
7
8
9
10
 
if( foo > 1 )
{
    cout << "this will print only if foo is > 1\n";
}
else
{
    cout << "this will print only if foo is <= 1\n";
}

cout << "this will print regardless of what foo is";


The point is, each if block does not need to cin.get and return 0... because that is already done AFTER the else/if chain. So just let execution fall through to that point.


4) Don't try to print unrelated things at all once.

There is no reason for the cpm and mpg comparisons to be joined like they are. It would make more sense to do those comparisons each in their own if/else chain and print their message seperately.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
// have one else/if chain for mpg
if( mpg < foo )
  cout << "bad mpg"
else if(mpg < bar)
  cout << "medium mpg"
else
  cout << "good mpg"

// then another for cpm
if( cpm < foo )
  cout << "low cpm";
else
  cout << "high cpm";

// then your quitting code
cin.get();
return 0;
Last edited on
Blanchy (25)
You have no idea how much that helped, thank you!

If you see any other flaws/bad habits please let me know! thanks again

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
 
#include <iostream>
#include <iomanip>

using namespace std;

int main()
{
    float miles, gallons, ppg, mpg, tripc, cpm;
   
    cout<< "2013 Pure Michigan Winter Vacation Fuel Efficiency Calculation";
    cout<< "\n\nHow mant miles did you travel? ";
    cin>> miles;
    cout<< "How many Gallons of Gas did you use? ";
    cin>> gallons;
    cout<< "What was the price per Gallon of Gas? ";
    cin>> ppg;
    if (ppg <= 0.0)
    {
       cout<<"\nGas price Invalid";
       cin.ignore();
    }
   
    mpg = (miles / gallons);
    tripc = (gallons * ppg);
    cpm = (tripc / miles);
    
    
    cout<< "\n\nYou got "<<fixed <<setprecision(3) <<mpg;
    cout<< " miles per gallon";
    cout<< "\nThis trip cost $"<<fixed <<setprecision(2) <<tripc;
    cout<< " at a cost per mile of $"<<fixed <<setprecision(6) <<cpm;
    
    
    cout<<"\nThis is a(n) ";

    // mpg if else

    if (mpg < 18.5)
    {
       cout<<"GAS GUZZLER";
    }
    else if ((mpg > 18.5) && (mpg < 34.75))
    {
         cout<<"AVERAGE";
    }
    else 
    {
         cout<<"GREEN MACHINE";
    }
    cout<<" auto mobile in fuel economy, \nand this was a ";

    // cpm if else

    if (cpm  < .0975)
    {
       cout<<"REASONABLE";
    }
    else
    {
        cout<<"PRICEY";
    }
    
    cout<<" Trip.";
      
   
cin.get();
cin.get();
return 0;  
}
Last edited on
Disch (13742)
You're still adding a redundant "reverse" conditional here:

1
2
3
4
5
6
7
8
 
    if (mpg < 18.5)  // <- line 38
    {
       cout<<"GAS GUZZLER";
    }
    else if ((mpg > 18.5) && (mpg < 34.75))  // line 42
    {
         cout<<"AVERAGE";
    }


line 38 checks to see if mpg < 18.5
Which means the else on line 42 ensures that mpg >= 18.5. There is no reason to check against that number again.


But yes that is definitely a big improvement. Good job =)
Last edited on
Topic archived. No new replies allowed.