Bad programming practice?

I have wrote some code but I think the record structure might be bad code practice? feedback would be appreciated

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
#include <iostream>
#include <vector>

using namespace std;

struct RECORD
{
public:
    int number;
    int f;
    int fx;
    int fx_squared;
};

vector <RECORD*> record;

void generateTable()
{
    cout << "Number     F       FX      FX_SQUARED" << endl;
    cout << "=====================================" << endl;
    int totalFreq = 0;
    for (unsigned int i = 0; i < record.size(); i++)
    {
        cout << record[i]->number << "      ";
        cout << record[i]->f << "       ";
        cout << record[i]->fx << "      ";
        cout << record[i]->fx_squared << "      ";
        cout << endl;
        totalFreq += record[i]->f;
    }
    cout << "=====================================" << endl;
}

int main()
{
    cout << "Mean and standard divation of grouped data" << endl;
    bool done = false;

    while(!done)
    {
        generateTable();
        int number, frequency;
        cout << "Enter a number: " << endl;
        cin >> number;
        cout << "Enter a frequency: " << endl;
        cin >> frequency;

        RECORD* rec = new RECORD();
        rec->number = number;
        rec->f = frequency;
        rec->fx = number * frequency;
        rec->fx_squared = number * rec->fx;

        int recordArraySize = record.size();
        record.resize(recordArraySize+1);
        record[recordArraySize] = rec;
    }
    return 0;
}
At the very least, fix the memory leaks: the simplest way to achieve that here is to stop using pointers. It's vector<RECORD> record; and, consequently,
1
2
       RECORD rec = {number, frequency, number * frequency, number * number * frequency};
       record.push_back(rec);
or equivalent
Last edited on
Random thoughts:

You are leaking memory. You are creating records with new but you are never deleting them.

General rule of thumb: don't use new unless you have to. In this case, since RECORD is so small, you can get away without dynamically allocating:

1
2
3
4
5
6
7
8
vector <RECORD> record;  // <- make it a vector of RECORDs rather than of pointers


//...

// RECORD* rec = new RECORD(); <- instead of this
RECORD rec;  // <- do this
  // don't use new unless you have to. 


Of course you'll also have to replace a lot of '->'s with '.'s

Also... you can push_back or emplace_back to append something to the end of a vector. You don't need to resize.

1
2
3
4
5
6
7
// This:
        int recordArraySize = record.size();
        record.resize(recordArraySize+1);
        record[recordArraySize] = rec;

// Can be replaced with this:
        record.push_back(rec);



If you really want to dynamically allocate... you can use smart pointers to avoid leaking:

1
2
3
4
5
6
7
8
9
10
11
12
13
#include <memory>

// ..
typedef std::unique_ptr<RECORD> RecordPtr;  // for convenience

//..
vector <RecordPtr> record;

//...
RecordPtr rec(new RECORD());

//...
record.emplace_back( std::move(rec) );



unique_ptr will automatically delete the object when it goes out of scope so you won't have leaks and you won't have to manually delete.
Last edited on
Thanks guys great advice, here is the new code is it good code or bad code? am I right to use a record structure it seems much cleaner than multiple arrays to me..

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
#include <iostream>
#include <vector>

using namespace std;

struct RECORD
{
public:
    int number;
    int f;
    int fx;
    int fx_squared;
};

vector <RECORD> record;

void generateTable()
{
    cout << "Number     F       FX      FX_SQUARED" << endl;
    cout << "=====================================" << endl;
    double totalFreq = 0, totalFx = 0, mean = 0;
    for (unsigned int i = 0; i < record.size(); i++)
    {
        cout << record[i].number << "      ";
        cout << record[i].f << "       ";
        cout << record[i].fx << "      ";
        cout << record[i].fx_squared << "      ";
        cout << endl;
        totalFreq += record[i].f;
        totalFx += record[i].fx;
    }
    cout << "=====================================" << endl << endl;

    if (totalFreq != 0 && totalFx != 0)
        mean = totalFx / totalFreq;

    cout << "Mean: " << mean << endl;
}

int main()
{
    cout << "Mean and standard divation of grouped data" << endl;
    bool done = false;

    while(!done)
    {
        generateTable();
        int number, frequency;
        cout << "Enter a number: " << endl;
        cin >> number;
        cout << "Enter a frequency: " << endl;
        cin >> frequency;

        RECORD rec;
        rec.number = number;
        rec.f = frequency;
        rec.fx = number * frequency;
        rec.fx_squared = number * rec.fx;
        record.push_back(rec);
    }
    return 0;
}
Yes... a structure is typically a much better option than trying to keep multiple arrays in sync.

The only other thing that comes to mind is that some of your data is a little redundant.

IE, you have 'fx' and 'fx_squared'. Having these as two separate variables increases the risk that they'll fall out of sync. That is... fx_squared might not necessarily equal fx*fx.

One thing to mention that isn't very important but structures are by default public so you can simply do
1
2
3
4
5
6
7
struct RECORD
{
    int number;
    int f;
    int fx;
    int fx_squared;
};
classes though on the other hand are by default private so you would have needed to include the public keyword.

You could do something like this: record.push_back({number, frequency, number * frequency, number * number * frequency}); instead of what you have now:
1
2
3
4
5
6
        RECORD rec;
        rec.number = number;
        rec.f = frequency;
        rec.fx = number * frequency;
        rec.fx_squared = number * rec.fx;
        record.push_back(rec);


Though your fx and fx_squared seem a bit odd to me. How is it squared when all you do is multiply by a portion of what made the other half? You are doing
number * number * frequency
instead of
number * number * frequency * frequency
.
Yeah that was a mistake I corrected that a few minutes ago. I don't enjoy this type of math very much and I thought to myself if I wrote a program it would stick in my memory better ready for the exam haha
Topic archived. No new replies allowed.