Bad programming practice?

[danielmccarthy]

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

#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;
}

[Cubbi]

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,

       RECORD rec = {number, frequency, number * frequency, number * number * frequency};
       record.push_back(rec);

or equivalent

[Disch]

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:

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.

// 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:

#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.

[danielmccarthy]

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..

#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;
}

[Disch]

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.

[giblit]

One thing to mention that isn't very important but structures are by default public so you can simply do

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:

        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

.

[danielmccarthy]

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