C++

Forum

 
Grade my program, 1-10

closed account 3R5fjE8b (226)
I need feedback as I have nowhere else to get feedback, I made this program to record the weight of my snake on specific dates, and save it. Be gentle as I am learning! I appreciate your time.

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
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
 
#include <iostream>
#include <fstream>
#include <windows.h>
#include <stdlib.h>
#include <vector>
/*
This program allows a user to either...
enter a date and weight
read entire date and weight history
or quit
*/
using namespace std;

struct weighday{
string dateAndWeight;
};

void check_if_file_exists(const string& document, vector <weighday>& weight_history);
char get_action();
void execute_action(char action, vector <weighday>& weight_history);
void new_entry(vector <weighday>& weight_history);
void view_history(vector <weighday>& weight_history);
void save_to_file(const string& document, vector <weighday>& weight_history);

int main()
{
    SetConsoleTitle("Snake Checker");
    vector <weighday> weight_history;
    string document = "Snake Save File.txt";

    check_if_file_exists(document, weight_history);

    while ( true )
    {
        cout << endl << endl << endl;
        execute_action(get_action(), weight_history);
    }

    save_to_file(document, weight_history);
    return 0;
}

void check_if_file_exists(const string& document, vector <weighday>& weight_history)
{
    /*Check to see if file exists, if it does, check if file has data, if it does, read data into our vector!*/
    string fileRead;
    int fileData;
    const int emptyFile = 0;
    ifstream fileSearch;
    fileSearch.open( document.c_str() );

    if ( fileSearch.good() )
    {
        getline(fileSearch, fileRead);
        fileData = fileRead.size();
        fileSearch.close(); /*Closes then reopens to start reading from top again*/
        ifstream fileSearch;
        fileSearch.open( document.c_str() );
        if ( fileData != emptyFile )
        {
            cout << "Previous data located, loading!" << endl;

            while ( !fileSearch.eof() )
            {
                /*loads all the data from file into our vector*/
                weighday* loadData = new weighday;
                getline(fileSearch,loadData->dateAndWeight);
                weight_history.push_back(*loadData);
                delete loadData;
            }
            fileSearch.close();
        }
        else
        {
            cout << "Empty document found!" << endl;
        }
    }
    else
    {
        cout << "Creating a new document to store data!" << endl;
        ofstream createFile;
        createFile.open( document.c_str() );
        createFile.close();
    }
}

char get_action()
{
    /*Main menu*/
    bool isDecisionMade = false;
    char yes = 'Y';
    char verify;
    char actionDecided;

    cout << "Select from the following list of choices by typing in the corresponding letter" << endl;
    cout << endl;
    cout << "A) Enter new data" << endl;
    cout << "B) View all data" << endl;
    cout << "C) Quit program" << endl;
    while ( !isDecisionMade )
    {
        cout << "> ";
        cin >> actionDecided;
        actionDecided = toupper(actionDecided);
        cout << "'" << actionDecided << "', is that correct? (Y/N)" << endl;
        cout << "> ";
        cin >> verify;
        verify = toupper(verify);
        if ( verify == yes )
        {
            isDecisionMade = true;
            return actionDecided;
        }
        else
        {
            cout << "Ok, what do you want to do?" << endl;
        }
    }
}

void execute_action(char action, vector <weighday>& weight_history)
{
    /*Parses input from main menu to find an action, if no match is found, exit with error*/
    const char newEntry = 'A';
    const char viewData = 'B';
    const char quit = 'C';

    switch ( action )
    {
        case newEntry: new_entry(weight_history);
            break;
        case viewData: view_history(weight_history);
            break;
        case quit: exit(0);
            break;
            default: exit(1);
    }
}

void new_entry(vector <weighday>& weight_history)
{
    string dateToWrite;
    string weightToWrite;
    bool finishedEntries = false;
    char yes = 'Y';
    char verify;

    cout << "Great! Let's make an entry, type in the date (DD/MM/YYYY)" << endl;
    while ( !finishedEntries )
    {
        bool dateCorrect = false;
        bool weightCorrect = false;

        while ( !dateCorrect )
        {
            cout << "> ";
            cin >> dateToWrite;
            cout << dateToWrite << " is that correct? (Y/N)" << endl;
            cout << "> ";
            cin >> verify;
            verify = toupper(verify);

            if ( verify == yes )
            {
                dateCorrect = true;
            }
            else
            {
                cout << "Let's try entering that date again!" << endl;
            }
        }

        cout << "Let's enter a weight in grams!" << endl;

        while ( !weightCorrect )
        {
            cout << "> ";
            cin >> weightToWrite;
            cout << weightToWrite << " is that correct? (Y/N)" << endl;
            cout << "> ";
            cin >> verify;
            verify = toupper(verify);

            if ( verify == yes )
            {
                weightCorrect = true;

                weighday* addEntry = new weighday;
                addEntry->dateAndWeight = "Date - " + dateToWrite + "   Weight - " + weightToWrite;
                weight_history.push_back(*addEntry);
                delete addEntry;

                cout << "Your entry has been added! Do you want to add another entry? (Y/N)" << endl;
                cin >> verify;
                verify = toupper(verify);
                if ( verify == yes )
                {
                    cout << "Great, let's add another entry!" << endl;
                    cout << endl << endl;
                }
                else
                {
                    finishedEntries = true;
                }
            }
            else
            {
                cout << "Let's try entering that weight again!" << endl;
            }
        }
    }
}

void view_history(vector <weighday>& weight_history)
{
    for ( int element = 0; element < weight_history.size(); element++ )
    {
        cout << weight_history[element].dateAndWeight << endl;
    }
}

void save_to_file(const string& document, vector <weighday>& weight_history)
{
    ofstream saveData;
    saveData.open( document.c_str() );
    for ( int element = 0; element < weight_history.size(); element++ )
    {
        if ( element != 0 )/*A temp fix to stop it from inserting an empty line at beginning of file*/
            {
                saveData << "\r\n";
            }
        saveData << weight_history[element].dateAndWeight;
    }
    saveData.close();
    cout << "Data has been saved successfully!" << endl;
}
MiiNiPaa (8886)
Line 4: #include <stdlib.h> in C++ you should use #include <cstdlib>

Line 12: using namespace std; everywhere is a sign of bad style.

Line 31: check_if_file_exists function name is misleading. Nobody expect that it would read file (or create files)

Line 33: infinite loops are not often appropriate choice.

Line 39: this will never be executed.

Lines 49-50: 
1
2
 
    ifstream fileSearch;
    fileSearch.open( document.c_str() );
Open files in constructor. Use C++11 and pass string itself: ifstream fileSearch(document);

As you did not post constraints om file content, I will not comment on data manipulation.

Lines 56-57: 
1
2
 
        fileSearch.close(); /*Closes then reopens to start reading from top again*/
        ifstream fileSearch;
Is not doing what you say it does. You are creating another ifstream with the same name as old one which shadows old one. You did not got problems only because you closed the old one.

Line 63: while ( !fileSearch.eof() ) Looping on eof() is a bad idea. You should loop on input or check stream state after attempt to read but before using results of read.

Line 66 (and related) : weighday* loadData = new weighday; Unneeded and dangerous usage of dynamic allocation. Simple weighday loadData; would do the same, but you will not risk leaks in your program.

Lines 81-83: I guess it is not finished?

Whole function is not intuitive. It does too much and have undesireable side effects ruining its chances to be reused.

I will not repeat on similar problems in oother places. You can find them yourself.
Additionally you should try to declare variables as close to the place of first usage as possible (aside from constants)
closed account 3R5fjE8b (226)
I fixed everything except for the c++11, I attempted to turn it on and I was getting an error.
closed account 3R5fjE8b (226)
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
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
 
#include <iostream>
#include <fstream>
#include <windows.h>
#include <cstdlib>
#include <vector>
using std::string;
using std::cin;
using std::cout;
using std::vector;
using std::endl;
using std::ifstream;
using std::ofstream;
/*
This program allows a user to either...
enter a date and weight
read entire date and weight history
or quit
*/

struct weighday{
string dateAndWeight;
};

void find_or_create_file(const string& document, vector <weighday>& weight_history);
char get_action();
void execute_action(char action, vector <weighday>& weight_history, bool& readyToQuit);
void new_entry(vector <weighday>& weight_history);
void view_history(vector <weighday>& weight_history);
void save_to_file(const string& document, vector <weighday>& weight_history);

int main()
{
    SetConsoleTitle("Snake Checker");
    vector <weighday> weight_history;
    string document = "Snake Save File.txt";

    find_or_create_file(document, weight_history);

    bool readyToQuit = false;
    while ( !readyToQuit )
    {
        cout << endl << endl << endl;
        execute_action(get_action(), weight_history, readyToQuit);
    }

    save_to_file(document, weight_history);
    return 0;
}

void find_or_create_file(const string& document, vector <weighday>& weight_history)
{
    /*Check to see if file exists, if it does, check if file has data, if it does, read data into our vector!*/
    string fileRead;
    int fileData;
    const int emptyFile = 0;
    ifstream fileSearch;
    fileSearch.open( document.c_str() );

    if ( fileSearch.good() )
    {
        getline(fileSearch, fileRead);
        fileData = fileRead.size();

        if ( fileData != emptyFile )
        {
            cout << "Previous data located, loading!" << endl;
            weighday loadData;
            loadData.dateAndWeight = fileRead;
            weight_history.push_back(loadData);
            while ( getline(fileSearch,loadData.dateAndWeight) )
            {
                /*loads all the data from file into our vector*/
                weight_history.push_back(loadData);
            }
            fileSearch.close();
        }
        else
        {
            cout << "Empty document found!" << endl;
        }
    }
    else
    {
        cout << "Creating a new document to store data!" << endl;
        ofstream createFile;
        createFile.open( document.c_str() );
        createFile.close();
    }
}

char get_action()
{
    /*Main menu*/
    bool isDecisionMade = false;
    char yes = 'Y';
    char verify;
    char actionDecided;

    cout << "Select from the following list of choices by typing in the corresponding letter" << endl;
    cout << endl;
    cout << "A) Enter new data" << endl;
    cout << "B) View all data" << endl;
    cout << "C) Save new data and quit" << endl;
    while ( !isDecisionMade )
    {
        cout << "> ";
        cin >> actionDecided;
        actionDecided = toupper(actionDecided);
        cout << "'" << actionDecided << "', is that correct? (Y/N)" << endl;
        cout << "> ";
        cin >> verify;
        verify = toupper(verify);
        if ( verify == yes )
        {
            isDecisionMade = true;
            return actionDecided;
        }
        else
        {
            cout << "Ok, what do you want to do?" << endl;
        }
    }
}

void execute_action(char action, vector <weighday>& weight_history, bool& readyToQuit)
{
    /*Parses input from main menu to find an action, if no match is found, exit with error*/
    const char newEntry = 'A';
    const char viewData = 'B';
    const char quit = 'C';

    switch ( action )
    {
        case newEntry: new_entry(weight_history);
            break;
        case viewData: view_history(weight_history);
            break;
        case quit: readyToQuit = true;
            break;
            default: exit(1);
    }
}

void new_entry(vector <weighday>& weight_history)
{
    string dateToWrite;
    string weightToWrite;
    bool finishedEntries = false;
    char yes = 'Y';
    char verify;

    cout << "Great! Let's make an entry, type in the date (DD/MM/YYYY)" << endl;
    while ( !finishedEntries )
    {
        bool dateCorrect = false;
        bool weightCorrect = false;

        while ( !dateCorrect )
        {
            cout << "> ";
            cin >> dateToWrite;
            cout << dateToWrite << " is that correct? (Y/N)" << endl;
            cout << "> ";
            cin >> verify;
            verify = toupper(verify);

            if ( verify == yes )
            {
                dateCorrect = true;
            }
            else
            {
                cout << "Let's try entering that date again!" << endl;
            }
        }

        cout << "Let's enter a weight in grams!" << endl;

        while ( !weightCorrect )
        {
            cout << "> ";
            cin >> weightToWrite;
            cout << weightToWrite << " is that correct? (Y/N)" << endl;
            cout << "> ";
            cin >> verify;
            verify = toupper(verify);

            if ( verify == yes )
            {
                weightCorrect = true;

                weighday* addEntry = new weighday;
                addEntry->dateAndWeight = "Date - " + dateToWrite + "   Weight - " + weightToWrite;
                weight_history.push_back(*addEntry);
                delete addEntry;

                cout << "Your entry has been added! Do you want to add another entry? (Y/N)" << endl;
                cin >> verify;
                verify = toupper(verify);
                if ( verify == yes )
                {
                    cout << "Great, let's add another entry!" << endl;
                    cout << endl << endl;
                }
                else
                {
                    finishedEntries = true;
                }
            }
            else
            {
                cout << "Let's try entering that weight again!" << endl;
            }
        }
    }
}

void view_history(vector <weighday>& weight_history)
{
    for ( int element = 0; element < weight_history.size(); element++ )
    {
        cout << weight_history[element].dateAndWeight << endl;
    }
}

void save_to_file(const string& document, vector <weighday>& weight_history)
{
    ofstream saveData;
    saveData.open( document.c_str() );
    for ( int element = 0; element < weight_history.size(); element++ )
    {
        if ( element != 0 )/*A temp fix to stop it from inserting an empty line at beginning of file*/
            {
                saveData << "\r\n";
            }
        saveData << weight_history[element].dateAndWeight;
    }
    saveData.close();
    cout << "Data has been saved successfully!" << endl;
}
MiiNiPaa (8886)
Problem with excess heap allocation in function new_entry() is not fixed. Additionally if you enter more than one entry, you are not prompted to enter date.

And you do not need to close file manually. fstream destructor takes care of that.
Last edited on
closed account 3R5fjE8b (226)
Oh ok. I will take care of that. Besides these flaws does it look... decent?
MiiNiPaa (8886)
Just another one: use just "\n". You do not need \r unless you relly need carriage return symbol.

Well, it gets the job done. There is no glaring mistakes and although it is not the best example of programming, it works, it complies to standard. So it is fine.
closed account 3R5fjE8b (226)
Under what circumstances do you want to allocate memory on the heap?
MiiNiPaa (8886)
1) When object[s] size is too large to be allocated on stack
2) When you need to extend lifetime of an object past current function
3) When you do not know size of allocated memory beforehand
4?) When you need to use polymorfism (usually falls under 2 too)

Even then, you should not use raw pointers. In C++11 there is now a standard pointer-like RAII class: unique_ptr.
MiiNiPaa (8886)
Example of slight changes to your code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
//Threw out empty file check as it served no purpose but to print a message
//No entries is a correct data file too
std::vector<weighday> load_data(const std::string& document)
{
    std::vector<weighday> data;
    std::ifstream fileSearch( document.c_str() );
    if ( fileSearch.is_open() ) {
        std::cout << "Previous data located, loading!" << std::endl;
        weighday loadData;
        while ( getline(fileSearch,loadData.dateAndWeight) ) //Handles empty file gracefully
            data.push_back(loadData);
    } else {
        std::cout << "Creating a new document to store data!\n";
        std::ofstream createFile( document.c_str() );
    }
    return data;
}

//...

int main()
{
    const std::string document = "Snake Save File.txt";
    std::vector<weighday> weight_history = load_data(document);
Topic archived. No new replies allowed.