C++

Forum

 
If/else....

wharp2 (79)
ok so I don't know why my program is doing this but what happiness is when I enter the package I want and the number of HR used it alway goes through the right stuff but then at the end tells me I have an invalid package witch I don't bellow is my code and the out put.... thanks (line 125 is the only problem)

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
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
 
//  main.cpp
//  Program3
//  Created by William Blake Harp on 6/25/14.

#include <iostream>
#include <cmath>
#include <iomanip>
using namespace std;

char Package(0);
double Hours(0);
double Monthly_Charges(0);

void getpackge();
void ValidPackge();
void GetHR();
void invalidcheck();

int main()
{
    cout << "What package did you purchase: " << endl;
    cin  >> Package;
    cout << "Type how many hours you used this month: " << endl;
    cin  >> Hours;

    getpackge();
    ValidPackge();
    GetHR();
    invalidcheck();
    
    return 0;
}// End main.

void getpackge()
{
    // If lower case letter and HR's are with in the package limits (Get Packge).
    if (Package == 'a' && Hours <= 50)
    {
        Monthly_Charges = 15;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
        }
        else if (Package == 'b' && Hours <= 100)
        {
            Monthly_Charges = 20;
            cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
        }
        else if (Package == 'c' && Hours <= 150)
        {
            Monthly_Charges = 25;
            cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
        }
}// End getpackge.

void ValidPackge()
{
    // If uper case letter and HR's are with in the package limits (Valid Packge).
    if (Package == 'A' && Hours <= 50)
    {
        Monthly_Charges = 15;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }
    else if (Package == 'B' && Hours <= 100)
    {
        Monthly_Charges = 20;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }
    else if(Package == 'C' && Hours <= 150)
    {
        Monthly_Charges = 25;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }

}// End validpackge

void GetHR()
{
    // If lower case letter and HR's are not in the package limits (Lower case get HR).
    if (Package == 'a' && Hours > 50)
    {
        Monthly_Charges = (Hours - 50) * 2.00 + 15;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }
    else if (Package == 'b' && Hours > 100)
    {
        Monthly_Charges = (Hours - 100) * 1.50 + 20;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }
    else if (Package == 'c' && Hours > 150)
    {
        Monthly_Charges = (Hours - 150) * 1.00 + 25;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }

    
    // If uper case letter and HR's are not in the package limits (Uper case Get HR).
    else if (Package == 'A' && Hours > 50)
    {
        Monthly_Charges = (Hours - 50) * 2.00 + 15;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }
    else if (Package == 'B' && Hours > 100)
    {
        Monthly_Charges = (Hours - 100) * 1.50 + 20;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }
    else if (Package == 'C' && Hours > 150 && Hours < 720)
    {
        Monthly_Charges = (Hours - 150) * 1.00 + 25;
        cout << "Your Monthly Charges are = $" << Monthly_Charges << setw(6) << endl;
    }

    }// End GetHR.

void invalidcheck()
{
    // If the number of HR are inputted incorrectly (HR correct).
    if (Hours > 720 || Hours < 0)
    {
        cout << "The number of hours you entered is invalid please try again.";
    }
    
    // If the package is inputted incorrectly (Package correct).
    else if(Package != 'A' || Package != 'a' || Package != 'B' || Package != 'b' || Package != 'C' || Package != 'c')
    {
        cout << "The package you entered is not a real package, please try again.";
    }
 
}//End invalidcheck.
//End Code! 



What package did you purchase: 
A
Type how many hours you used this month: 
50
Your Monthly Charges are = $15
The package you entered is not a real package, please try again.
wildblue (1505)
line 123 make those logical ANDs && instead of logical ORs ||

You need all of those conditions to be true to output the invalid message, not just one.


Edit: Maybe look into the tolower or toupper functions so you don't have to duplicate code looking for 'a' or 'A' etc?

http://www.cplusplus.com/reference/cctype/toupper/
Last edited on
wharp2 (79)
oh wow thanks did not even see that
ResidentBiscuit (4459)
Your program in general needs a lot of cleaning up. Functions are poorly defined, functions seem to have a lot of identicatal behavior, use of global variables, and inconsistent naming.
Last edited on
wharp2 (79)
I mean it works great now what do you mean cleaning up?
wildblue (1505)
You are duplicating a bunch of code. For example - The only difference in these two different functions seems to be the input of a lower case vs an upper case letter? That really shouldn't warrant two different functions to be created and called.

1
2
 
getpackge();
ValidPackge();



And getpackge sounds like it should be collecting the package type from the user, getHR sounds like it should be collecting the hours from the user, but that's not what those functions do.


The more simple and concise you can make your code, even if it otherwise works, the easier it is to update and maintain later on, by you or by someone else :)
AbstractionAnon (6954)
what do you mean cleaning up?

Just because it's working doesn't mean it's a good program.
ResidentBiscuit gave you a list of things that can be cleaned up.

More specifically:
1) getpackge() and Validpackge() are identical except for the case of the variable. Why have two nearly identical functions? wildblue already suggested that you look at toupper() or tolower() to avoid having to deal with different cases.

2) Same issue with getHR except the duplicated logic is all in a single function.

3) Lines 10-12 are global variables. Using global variables is a poor practice.

4) Lines 26-29 flow issues. You do invalidcheck() last. Normal flow is to prompt for a particular input, accept the input, then check that the input is valid. If the input is not valid, display an error message and prompt for and accept input again.

5) You have identical couts for monthly_charges all over the place. There is no reason you need more than one (as long as it is in the right place).

Topic archived. No new replies allowed.