Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have just started to play around with C++ and I want to make my code efficient and clutter free. Please critique this code constructively and give feedback that will help me in my programming career.

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

/*
This structure will hold my 
initialized structure set for
this assignment.
*/
struct Set_Budget
{
double set_housing;
double set_utilities;
double set_household_expenses;
double set_transportation;
double set_food;
double set_medical;
double set_insurance;
double set_entertainment;
double set_clothing;
double set_miscellaneous;
};

/*
This structure will receive 
the values entered through
out this assignment.
*/
struct Asked_Budget
{
double asked_housing;
double asked_utilities;
double asked_household_expenses;
double asked_transportation;
double asked_food;
double asked_medical;
double asked_insurance;
double asked_entertainment;
double asked_clothing;
double asked_miscellaneous;
};

/* 
My prototypes for the functions
which I will use in this program
*/
void amounts (Asked_Budget&); // Input passed by reference
void compare_budgets (Asked_Budget);
void budget_alert (double);

/* 
This will me the main for this
program.  It will only define 
my " Asked_Budget " structure
and send its values to my 
functions.
*/
void main ()
{
Asked_Budget student; // Define my Structure
amounts ( student ); // Sending Structure to function
compare_budgets ( student ); // Sending structure to function
}

/*
This Function will receive the values from
the user and access my Asked.Budget structure
to assign the received values
*/
void amounts ( Asked_Budget &amount) // Reference Parameter
{
cout << "   PLEASE ENTER YOUR MONTHLY HOUSING EXPENSE:  .......... $";
cin >> amount.asked_housing; // Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY UTILITIES EXPENSE:  .......... $";
cin >> amount.asked_utilities;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHKY HOUSE-HOLD EXPENSES:  .......... $";
cin >> amount.asked_household_expenses;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY TRANSPORTATION EXPENSE:  .......... $";
cin >> amount.asked_transportation;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY FOOD EXPENSE:  .......... $";
cin >> amount.asked_food;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY MEDICAL EXPENSE:  .......... $";
cin >> amount.asked_medical;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY INSURANCE EXPENSE:  .......... $";
cin >> amount.asked_insurance;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY ENTERTAINMENT EXPENSE:  .......... $";
cin >> amount.asked_entertainment;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY CLOTHING EXPENSE:  .......... $";
cin >> amount.asked_clothing;// Assigning the value to the structure

cout << "   PLEASE ENTER YOUR MONTHLY MISCELLANEOUS EXPENSE:  .......... $";
cin >> amount.asked_miscellaneous;// Assigning the value to the structure
}

/*
This function will initialize my " Set_Budget " 
structure and compare it to the " Asked_Budget "
structure.
This Function will also display the budget results
for every category. As well as the over all Budget
result for the month.
*/

void compare_budgets (Asked_Budget amount)
{
Set_Budget monthly = { 500.00, 150.00, 65.00, 50.00, 250.00, 30.00, 100.00, 150.00, 75.00, 50.00}; // Initialize my " Set_Budget " Structure with values

// Variables to hold the results for each  budget category
double result_housing;
double result_utilities;
double result_household_expenses;
double result_transportation;
double result_food;
double result_medical;
double result_insurance;
double result_entertainment;
double result_clothing;
double result_miscellaneous;
double result_budget = 0;

// This section will compare both structures and the differences are assign to their corresponding variable
result_housing = monthly.set_housing - amount.asked_housing;
result_utilities = monthly.set_utilities - amount.asked_utilities;
result_household_expenses = monthly.set_household_expenses - amount.asked_household_expenses; 
result_transportation = monthly.set_transportation - amount.asked_transportation;
result_food  = monthly.set_food- amount.asked_food;
result_medical = monthly.set_medical - amount.asked_medical;
result_insurance = monthly.set_insurance - amount.asked_insurance;
result_entertainment = monthly.set_entertainment - amount.asked_entertainment;
result_clothing = monthly.set_clothing- amount.asked_clothing;
result_miscellaneous = monthly.set_miscellaneous - amount.asked_miscellaneous;

// Simple mechanism, asking the user to press enter to continue
cout << " \n\n\n\n\n ";
cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
cout << " \n\n\n\n\n";

/*
    This section will show the comparison results
    for each category. This section will also 
    show the user if they are either " At " , 
    " Over ", or " Under " budget for the month.
    This was done by sending the entered values 
    to another function to compare the result.
*/

cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( HOUSING ) CATEGORY: $" << fixed << setprecision (2) << result_housing; // I used set percision to reduce the output to 2 decimal places
budget_alert ( result_housing); // Sending the comparison variable to another function.
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( UTILITIES ) CATEGORY: $" << fixed <<  setprecision (2) << result_utilities;
budget_alert ( result_utilities);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( HOUSE HOLD EXPENSES ) CATEGORY: $" << fixed <<  setprecision (2) << result_household_expenses;
budget_alert ( result_household_expenses);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( TRANSPORTATION ) CATEGORY: $" << fixed <<  setprecision (2) << result_transportation;
budget_alert ( result_transportation);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( FOOD ) CATEGORY: $" << fixed <<  setprecision (2) << result_food;
budget_alert ( result_food);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( MEDICAL ) CATEGORY: $" << fixed <<  setprecision (2) << result_medical;
budget_alert ( result_medical);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( INSURANCE ) CATEGORY: $" << fixed <<  setprecision (2) << result_insurance;
budget_alert ( result_insurance);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( ENTERTAINMENT ) CATEGORY: $" << fixed <<  setprecision (2) << result_entertainment;
budget_alert ( result_entertainment);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( CLOTHING ) CATEGORY: $" << fixed <<  setprecision (2) << result_clothing;
budget_alert ( result_clothing);
cout << " \n\n";
cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( MISCELLANEOUS ) CATEGORY: $" << fixed <<  setprecision (2) << result_miscellaneous;
budget_alert ( result_miscellaneous);
cout << " \n\n";

// This will add up all the result for the month and give me an over view of the monthly budget.
result_budget = result_housing + result_utilities + result_household_expenses + result_transportation + result_food 
                + result_medical + result_insurance + result_entertainment + result_clothing + result_miscellaneous + result_budget;

// Simple mechanism, asking the user to press enter to continue
cout << " \n\n\n\n\n ";
cout << " \t\t\t    PRESS ENTER TWICE TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
cout << " \n\n\n\n\n";

// This section will show the over all monthly budget.
cout << " \t\t YOUR RESULT FOR THIS MONTH'S BUDGET IS : ....  $" << setprecision (2) << result_budget << " \t\t\t\t\t ";
budget_alert ( result_miscellaneous); // sending the result to my other function
cout << " \n\n ";

system ( " pause ");
}

/*
This function will receive values and compare them to zero
if the value is below 0 it is under budget , over 0 it means
it is over budget, or if it is equal to 0 it means it is 
exactly at budget.

This function will display the results corresponding message
*/

void budget_alert ( double alert)
{
if ( alert == 0 )// comparing result to 0
{
    cout << " \n\t\t\t\t  ( RIGHT AT BUDGET ) "; // message given after result is compared
}

if ( alert > 0 )
{
    cout << " \n\t\t\t\t  ( UNDER BUDGET ) ";
}

if ( alert < 0 )
{
    cout << " \n\t\t\t\t   ( OVER BUDGET ) " ;
}
}
share|improve this question
    
@vnp: You could also make a comment explaining it, in case it was intentional. –  Jamal Sep 2 at 17:57
3  
Be aware that with floating point numbers (doubles or floats) you have to be careful when comparing them. For example in bugdet_alert you compare alert which is a double to 0. This might not work as expected. It is better to compare to an epsilon (very small number) than comparing directly to zero because there are small rounding errors that occur regularly with floating point value operations. (2.0 - 2.0 may not result in 0.0 on all systems, it might result in 0.000000000001 etc.) –  YoungJohn Sep 2 at 19:26
1  
@YoungJohn correct sentiment, but note that a system that subtracts literal 2.0 from itself and gets anything other than zero would be pretty broken :) my usual example is that on an IEEE 754-compliant system, 0.1 + 0.2 - 0.3 is NOT zero. –  AakashM Sep 3 at 10:05
    
The thread title is more than bad... –  xmashallax Sep 3 at 12:56

3 Answers 3

The two structures are the same,

struct Set_Budget
{};

struct Asked_Budget
{};

Don't make them different they are the same so just make one.

struct Budget
{};

Then you can name each object differently;

Budget  set;
Budget  asked;
Budget  result;
... etc

Your input values assume the user is perfect and will make no mistakes.

cout << "   PLEASE ENTER YOUR MONTHLY HOUSING EXPENSE:  .......... $";
cin >> amount.asked_housing;

I would change this to make sure that that you validate the user input (are negative numbers allowed). What happens if they type Blarp? I would then encapsulate that in a function.

amount.housing  = askUser("   PLEASE ENTER YOUR MONTHLY HOUSING EXPENSE:  .......... $");

Here you use a lot of variables that have the same names as structure.

// Variables to hold the results for each  budget category
double result_housing;
double result_utilities;
double result_household_expenses;
double result_transportation;
double result_food;
double result_medical;
double result_insurance;
double result_entertainment;
double result_clothing;
double result_miscellaneous;

I would just use another Budget object.

Budget   result;

A lot of repeated code.

cout << "  RESULTING BUDGET DIFFERENCE FOR YOUR ( HOUSING ) CATEGORY: $" << fixed << setprecision (2) << result_housing; // I used set percision to reduce the output to 2 decimal places
budget_alert ( result_housing); // Sending the comparison variable to another function.
cout << " \n\n";

Encapsulate it into a function. That way fixes to the code only need to be applied to a single place (you don't need to do a find and replace to fix a bug).

share|improve this answer

Using namespace std

This is a common issue. Don't do it.

main signature

C++ specifies that main() must be declared returning an int. void main() is not valid C++.

Variable declarations

In compare_budgets() you declare all your variables at the beginning of the function. It is better to declare them as late as possible. Also, the values which are initialized and then only read should be marked const. This will guarantee that the variable cannot be accidentally modified, and makes your intent clearer. Thus, variables declared and set like this:

double result_housing;
// ...
result_housing = monthly.set_housing - amount.asked_housing;

Can be written on one line:

const double result_housing = monthly.set_housing - amount.asked_housing;

Limit output locations

In general it's a good idea to limit the number of places you write to an output stream. budget_alert() contains a lot of repetition, and directly outputs to cout. This makes it difficult to use a different stream, if you ever want to change the output location. I would rewrite the function to return a string indicating your alert message:

std::string budget_alert(const double alert)
{
    std::string budget_position;
    if (alert == 0) {
        budget_position = "RIGHT AT";
    } else if (alert > 0) {
        budget_position = "UNDER";
    } else {
        budget_position = "OVER";
    }
    return budget_position + " BUDGET";
}

Then you can adjust the call site to output the correct whitespace and parentheses. Also note that I use else if for mutually exclusive conditions. Again, this makes your intention clearer.

Meaningless comments

Some of your comments contain redundant information which already exists in the code.

void amounts (Asked_Budget&); // Input passed by reference

The comment here is completely unnecessary and just clutters your code. The function signature already indicates the parameter is taken by reference -- there's no need to say so in a comment.

share|improve this answer

I feel some points to be addressed,

  1. As discussed, we can declare only one structure and do the job.

You can define the structure by two ways, a) By defining the instance of the structure. b) By defining a pointer to the structure and dynamically allocating the pointer.(do not forget to free the pointer after its use.)

For initializing the structure use the following for a),

budget instBudget = {0};

for b)

budget *psBudget = new budget();
memset(psBudget, 0, sizeof(budget));

you can also define the structure in C++ way i.e. use a constructor to initialize the members.

  1. Use a specific variable naming convention i.e. use mixedcase or full lowercase or whatever you want to use but uniform for the whole program.

  2. Try to return funtion execution status as return value for each routine.

share|improve this answer
    
There is absolutely no need to dynamically allocate a budget. Please don't advise it. memset() can work on stack-allocated objects too, but leads to UB if the struct isn't a POD. A better solution would be to define a constructor for budget. –  Aurelius Sep 3 at 16:18
1  
Also note new budget(); zero initializes the object so all members have been set to zero. new budget; on the hand does not do any initialization. (notice the difference in paren). You should never use this on objects in in C++ memset(psBudget, 0, sizeof(budget)); I would also vote your down but I think -2 is enough punishment for somebody with 49 rep. –  Loki Astari Sep 3 at 16:28
    
@Loki, really how will you initialize your object to avoid "enough punishment" :) ? –  Subhajit Sep 3 at 17:04
    
@Aurelius, I advised to use both ways if he is required to, how memset() "leads to UB if the struct isn't a POD"? –  Subhajit Sep 3 at 17:06
1  
@Subhajit: Its not just that it may cause UB. But the fact that it becomes a maintenance problem. Somebody comes along after you (maybe a year later) and modifies the class (adds a constructor) but does not notice you have been haphazardly using memset through your code (because why would he look for every occurrence of you r object in 10,000,000 lines of code). You now have an object that is initialized and then scribbled over with memset. So its mostly a maintenance issue. –  Loki Astari Sep 3 at 17:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.