Data accumulator collecting monthly expenditure

The idea of the program is to serve as a data accumulator collecting monthly expenditure from a user and later display it.

I wanted to implement classes/methods and just basic stuff that I understand to help me get better at writing better C++ programs.

This program works and does what I want it to do, but I figure since this being my "first" program, I'm sure there are bad habits or practices that I'm implementing that I would want to avoid later on. So if anyone can pick the flaws in my code, please feel free to highlight them.

I would like to correct my flaws early on while I can. I'll add that I'm aware that my current design is not meant to store the data once the program quits. I will eventually use GUI design and expand this program to store data permanently, but for now, this what I wrote.

```
#include
#include
#include
#include
#include
#include
#include

using namespace std;

string concat_date(int m, int d, int y){

string tot_dt,st_m;

if (m == 0) {
string st_temp("January ");
st_m = st_temp;
}
if (m == 1) {
string st_temp("February ");
st_m = st_temp;
}
if (m == 2) {
string st_temp("March ");
st_m = st_temp;
} if (m == 3) {
string st_temp("April ");
st_m = st_temp;
} if (m == 4) {
string st_temp("May ");
st_m = st_temp;
} if (m == 5) {
string st_temp("June ");
st_m = st_temp;
} if (m == 6) {
string st_temp("July ");
st_m = st_temp;
} if (m == 7) {
string st_temp("August ");
st_m = st_temp;
} if (m == 8) {
string st_temp("September ");
st_m = st_temp;
} if (m == 9) {
string st_temp("October ");
st_m = st_temp;
} if (m == 10) {
string st_temp("November ");
st_m = st_temp;
}
if (m == 11) {
string st_temp("December ");
st_m = st_temp;
}

string st_d = to_string(d);
string st_y = to_string(y);
tot_dt = st_m + st_d + "," + "'" + st_y[1] + st_y[2];

return tot_dt;
}

string tot_exp(string md,string cs,string st,string fd,string as){

string intEx;

if (md.length() == 0) {
md = "0";
}
if (cs.length() == 0) {
cs = "0";
}
if (s

Here are some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Fix your formatting

There are inconsistent spaces at the beginning of lines, inconsistent indentation and inconsistent use and placement of curly braces {}. Being consistent helps others read and understand your code.

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority. In this code, main uses neither argc nor argv and so that function should be int main().

Eliminate unused routine

The code defines a peculiarly useless function datedisplay but it is not actually used and can be eliminated from the code. Smaller code typically runs faster, so there's a small chance that this could improve speed as well.

Think of the user

The program issues a prompt "... quit(Press 0 to exit or Y/y to continue)?" but a response of "N" also continues. This is annoying at best. Better would to make the choices similar, such as 0 or 1 or Y or N.

Consider using range-for syntax

The code currently prints the resulting list with this code:

for (iter = FP.begin(); iter != FP.end(); iter++) {
    std::cout last_name first_name date expn  << std::endl << std::endl;
}

This could be written much more simply with a range-for:

for (const auto &item : FP) {
    std::cout << item.last_name << ", " << item.first_name 
        << "\nExpenditure for "<< item.date << " is \n" 
        << item.expn  << "\n\n";
}

Declare variables as late as possible

Rather than using the old C-style of declaring all variables at the top of a function, use the modern C++-style and declare variables as late as possible. Doing so can sometimes help the compiler figure out register allocation, resulting in faster, smaller code.

Use the appropriate types

Is an expense really best represented as a std::string? Right now, one can answer all of the questions with text. Leading to an output like this:

> Transistor, Edward
> Expenditure for December 6,'16 is 
> Medical:two chickens$.
> Cosmetics:one pig$.
> Stationery:nothing at all$.
> Food & Drinks:more than I should have$.
> Assorted:pocket change$.

Also, a std::vector is likely to be a much more appropriate data structure than std::list for this program.

Make data members private

The user_data class has all of its members public which is probably a sign that the class design is not very good. Instead, add member functions to the class so that internal access is not required.

Improve your constructor

The user_data class has four data elements; first_name, last_name, date and expn. The constructor should create and initialize those four things. A more modern style for your constructor might be this:

user_data(std::string fn, std::string ln, std::string dt, std::string ex) :
    first_name{fn},
    last_name{ln},
    date{dt},
    expn{ex}
{ }

Make better use of objects

The user_data class, in addition to be rather poorly named, is not really doing much in this code. First, using the right kind of data items (as mentioned above) would help. Second, write some friend functions to handle input and output. For example:

friend std::ostream& operator<<(std::ostream& out, const user_data &ud) {
    return out << ud.last_name << ", " << ud.first_name 
        << "\nExpenditure for "<< ud.date << " is \n" 
        << ud.expn;
}

My quick rewrite of your user_data class looks like this:

class user_data{
private:
    static constexpr size_t expense_count{5};
    static const std::array labels;

    std::string first_name;
    std::string last_name;
    std::tm date;
    std::array expn;

public:
    user_data();

    friend std::istream& operator>>(std::istream& in, user_data &ud);
    friend std::ostream& operator user_data::labels {
    "Medical", "Cosmetics", "Stationery", "Food & Drinks", "Assorted"
};

With it, main looks like this:

int main() {
    std::vector FP;

    while (addmore()) {
        user_data udone;
        std::cin >> udone;
        FP.push_back(udone);
    }
    for (const auto &item : FP) {
        std::cout << item << "\n\n";
    }
}

Consider expandability

Are the particular expense classes going to be the ones that will always be used? Maybe not. Also, it would make sense to encapsulate the name and the amout in a single object. I'd suggest making the expn member of user_data a std::array or std::vector of integers instead, and have the names supplied via the output inserter functon (a modification of the one just above would work.)

Don't make pointless copies of variables

The code currently contains these lines:

m = tm_pointer -> tm_mon;
d = tm_pointer -> tm_mday;
y = tm_pointer -> tm_year;

dt = concat_date(m, d, y);

However, there's no real point to making those copies. It cou

StackExchange Code Review Q#149118, answer score: 2