6
\$\begingroup\$

The program allows user to enter values for 3 of the following

  • initial velocity
  • final velocity
  • acceleration
  • displacement
  • time

and to specify an unknown (one of the previously mentioned variables). The program uses the Kinematic Equations to calculate the unknown.

I would like a review of the entire program. I have only studied programming online as a hobby so i have never received comments on my code. I want to become a better programmer and break any bad habits. Dont hold back on any criticism or praise, please comment anything you think of. What is inefficient/ unclear/ unnecessary? What can be improved or added? What is good? What approach would you have taken to the problem? Is the code understandable at all As far as I can tell the code at least works, does what it should.

OrderedArray.h, container class

#include "stdafx.h"
#include <assert.h>
#include <iostream>

class OrderedArray
{
private:
    double* m_pdValues;
    int* m_pnOrderKeys;
    int m_nValuesLength;
    int m_nKeysLength;
void erase()
{
    delete[] m_pdValues;
    delete[] m_pnOrderKeys;
    m_pdValues = 0;
    m_pnOrderKeys = 0;
    m_nValuesLength = 0;
    m_nKeysLength = 0;      
}
public:
    OrderedArray() : m_pdValues(0), m_nValuesLength(0), m_pnOrderKeys(0), m_nKeysLength(0)
{

}
double operator[](int nIndex)   //returns from values array
{
    assert(nIndex >= 0 && nIndex < m_nValuesLength);
    return m_pdValues[nIndex];
}
void insertLast(double dValue)
{
    assert(nIndex >= 0 && nIndex <= m_nValuesLength);
    double* pdValues = new double[m_nValuesLength + 1];
    for (int i=0; i < m_nValuesLength; i++)
        pdValues[i] = m_pdValues[i];
    pdValues[m_nValuesLength] = dValue;
    delete[] m_pdValues;
    m_pdValues = pdValues;
    m_nValuesLength += 1;
}
void insertLast(int nKey)
{
    assert(nIndex >= 0 && nIndex <= m_nKeysLength);
    int* pnKeys = new int[m_nKeysLength + 1];
    for (int i=0; i < m_nKeysLength; i++)
        pnKeys[i] = m_pnOrderKeys[i];
    pnKeys[m_nKeysLength] = nKey;
    delete[] m_pnOrderKeys;
    m_pnOrderKeys = pnKeys;
    m_nKeysLength += 1;
}
void insertBefore(double dValue, int nIndex)
{
    assert(nIndex >= 0 && nIndex <= m_nValuesLength);
    double* pdValues = new double[m_nValuesLength + 1];
    for (int nBefore=0; nBefore < nIndex; nBefore++)
        pdValues[nBefore] = m_pdValues[nBefore];
    pdValues[nIndex] = dValue;
    for (int nAfter=nIndex; nAfter < m_nValuesLength; nAfter++)
        pdValues[nAfter+1] = m_pdValues[nAfter];
    delete[] m_pdValues;
    m_pdValues = pdValues;
    m_nValuesLength += 1;
}
void insertBefore(int nKey, int nIndex)
{
    assert(nIndex >= 0 && nIndex <= m_nKeysLength);
    int* pnKeys = new int[m_nKeysLength + 1];
    for (int nBefore=0; nBefore < nIndex; nBefore++)
        pnKeys[nBefore] = m_pnOrderKeys[nBefore];
    pnKeys[nIndex] = nKey;
    for (int nAfter=nIndex; nAfter < m_nKeysLength; nAfter++)
        pnKeys[nAfter+1] = m_pnOrderKeys[nAfter];
    delete[] m_pnOrderKeys;
    m_pnOrderKeys = pnKeys;
    m_nKeysLength += 1;
}
void add(double dValue, int nKey)
{
    if(m_nValuesLength >= 3)
    {
        std::cout << "OrderedArray full" << std::endl;
        return;
    }
    if(m_pdValues == 0)
    {
        m_pdValues = new double[1];
        m_nValuesLength = 1;
        m_pdValues[0] = dValue;
        m_pnOrderKeys = new int[1];
        m_nKeysLength = 1;
        m_pnOrderKeys[0] = nKey;
    }
    else
    {
        int keysLengthTemp = m_nKeysLength;
        for(int i = 0; i < keysLengthTemp; i++)
            {
                if(nKey < m_pnOrderKeys[i])
                    {
                        insertBefore(dValue, i);
                        insertBefore(nKey, i);
                        return;
                    }
            }
        insertLast(dValue);
        insertLast(nKey);
    }
}
~OrderedArray()
{
    delete[] m_pdValues;
    delete[] m_pnOrderKeys;
}
int getLength() { return m_nValuesLength; }
double* getValuesArray()
{
    assert(m_nValuesLength == 3);
    return m_pdValues;
}
int getSumOfKeys()
{
    assert(m_nKeysLength == 3);
    return m_pnOrderKeys[0] + m_pnOrderKeys[1] + m_pnOrderKeys[2];
}
};

Physics Formulae.cpp

#include "stdafx.h"
#include <math.h>
#include <iostream>
#include <string>
#include "OrderedArray.h"
#define PI 3.14159265358979323846
//func name = original leftside variable in formula + variable to solve for
int translate(std::string s) 
{
    if(s == "d")
        return 0;
    if(s == "vf")
        return 1;
    if(s == "a")
        return 2;
    if(s == "t")
        return 3;
    if(s == "vi")
        return 4;
    return -1;
}
double* shit(double x, double y, double z)          //placeholder function     to fill out funcarray
{                                                   
    static double pdArray[2];
    pdArray[0] = PI;
    pdArray[1] = PI;
    return pdArray;
}                                                   //vf does not contain d
double* vfvf(double a, double t, double vi)
{
    static double pdArray[2];
    pdArray[0] = vi + a * t;
    pdArray[1] = PI;
    return pdArray;
}

double* vfa(double vf, double t, double vi)
{
    static double pdArray[2];
    pdArray[0] = (vf - vi) / t;
    pdArray[1] = PI;
    return pdArray;
}
double* vft(double vf, double a, double vi)
{
    static double pdArray[2];
    pdArray[0] = (vf - vi) / a;
    pdArray[1] = PI;
    return pdArray;
}
double* vfvi(double vf, double a, double t)
{
    static double pdArray[2];
    pdArray[0] = vf - a * t;
    pdArray[1] = PI;
    return pdArray;
}   
//d does not contain vf
double* dd(double a, double t, double vi)
{
    static double pdArray[2];
    pdArray[0] = vi * t + 0.5 * a * t * t;
    pdArray[1] = PI;
    return pdArray;
}
double* da(double d, double t, double vi)
{
    static double pdArray[2];
    pdArray[0] = (d - vi * t) / pow(t, 2) * 2;
    pdArray[1] = PI;
    return pdArray;
}
double* dt(double d, double a, double vi)   //RETURNS ARRAY
{
    static double pdArray[2];
    pdArray[0] = 0 - (vi / a) + sqrt(pow(vi/a, 2) + 2 * d / a);
    pdArray[1] = 0 - (vi / a) - sqrt(pow(vi/a, 2) + 2 * d / a);
    return pdArray;
}
double* dvi(double d, double a, double t)
{
    static double pdArray[2];
    pdArray[0] = (d - 0.5 * a * t * t) / t;
    pdArray[1] = PI;
    return pdArray;
}
//dd does not contain a
double* ddd(double vf, double t, double vi)         
{
    static double pdArray[2];
    pdArray[0] = (vi + vf) / 2 * t;
    pdArray[1] = PI;
    return pdArray;
}
double* ddvf(double d, double t, double vi)
{
    static double pdArray[2];
    pdArray[0] = d / t * 2 - vi;
    pdArray[1] = PI;
    return pdArray;
}
double* ddt(double d, double vf, double vi)
{
    static double pdArray[2];
    pdArray[0] = d / (vi + vf) * 2;
    pdArray[1] = PI;
    return pdArray;
}
double* ddvi(double d, double vf, double t)
{
    static double pdArray[2];
    pdArray[0] = d / t * 2 - vf;
    pdArray[1] = PI;
    return pdArray;
}
//vf2 does not contain t
double* vf2d(double vf, double a, double vi)
{
    static double pdArray[2];
    pdArray[0] = (pow(vf, 2) - pow(vi, 2)) / 2 / a;
    pdArray[1] = PI;
    return pdArray;
}
double* vf2vf(double d, double a, double vi)        //RETURNS ARRAY
{
    static double pdArray[2];
    pdArray[0] = sqrt(pow(vi, 2) + 2 * a * d);
    pdArray[1] = 0 - sqrt(pow(vi, 2) + 2 * a * d);
    return pdArray;
}
double* vf2a(double d, double vf, double vi)
{
    static double pdArray[2];
    pdArray[0] = (pow(vf, 2) - pow(vi, 2)) / 2 / d;
    pdArray[1] = PI;
    return pdArray;
}
double* vf2vi(double d, double vf, double a)        //RETURNS ARRAY
{
    static double pdArray[2];
    pdArray[0] = sqrt(pow(vf, 2) - 2 * a * d);
    pdArray[1] = 0 - sqrt(pow(vf, 2) - 2 * a * d);
    return pdArray;
}

int _tmain(int argc, _TCHAR* argv[])
{
    double* (*funcArray[4][5])(double, double, double) = //[missing][unknown]
    {
        {shit, vfvf, vfa, vft, vfvi},
        {dd, shit, da, dt, dvi},
        {ddd, ddvf, shit, ddt, ddvi},
        {vf2d, vf2vf, vf2a, shit, vf2vi}
    };
    OrderedArray OA = OrderedArray();
    int nAccessVarMissing = -1;
    int nAccessUnknownVar = -1;
    using namespace std;
    string s;
    cout << "Which variable is unknown? (d, vf, vi, a or t)" << endl;
    cin >> s;
    nAccessUnknownVar = translate(s);
    double dValueTemp = -1;
    string strVarNameTemp = " ";
    for(int i = 0; i < 3; i++)
    {
        cout << "Enter a known variable (d, vf, vi, a or t) followed by 'Enter' followed by the value of the variable:  ";
        cin >> strVarNameTemp;
        cin >> dValueTemp;
        OA.add(dValueTemp, translate(strVarNameTemp));
        dValueTemp = -1;
        strVarNameTemp = " ";
    }
    nAccessVarMissing = 1 + 2 + 3 + 4 - OA.getSumOfKeys() - nAccessUnknownVar;
    if(nAccessVarMissing == 4)
    {
        cout << "Invalid combination of variables, vi must be either known or unknown" << endl;
        cin.get();
        exit(0);
    }
    cout << "unknown is: " << funcArray[nAccessVarMissing][nAccessUnknownVar](OA[0], OA[1], OA[2])[0] << endl;
    double dTemp = funcArray[nAccessVarMissing][nAccessUnknownVar](OA[0], OA[1], OA[2])[1];
    if(dTemp != PI)
        cout << "unknown is: " << dTemp;
    int x = 0;
    cin >> x;
    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ I have a very similar project I'm working on. The code I use for the Kinematics part of it is here. I took a fairly different approach from you on it I think. I haven't looked it over too thoroughly but it looks like your code contains a lot of work that it doesn't have to. \$\endgroup\$
    – Ulthran
    Dec 13, 2015 at 4:45

1 Answer 1

8
\$\begingroup\$

This is pretty clever work for someone self taught! In my opinion, it's more work than is necessary, though. Your OrderedArray class looks like a cross between std::map and std::vector, but it also looks like it will never hold more than 4 elements, if I understand it correctly.

Simplify

I think you can do away with the OrderedArray class and the funcArray, and just use some enums and a switch statement. For example, something like this:

enum {
    kUnknown_Displacement,
    kUnknown_InitialVelocity,
    kUnknown_FinalVelocity,
    kUnknown_Acceleration,
    kUnknown_Time
};

Then, after collecting your knowns, just do something simple like:

switch(unknown) {
    case kUnknown_Displacement:
        calculateDisplacement(initialVelocity, Acceleration, time); // or whatever variables you need
    break;

    case kUnknown_InitialVelocity:
        // etc...
    break;
};

Naming

The other thing I think you could improve is your naming. Function names like vfvf, vfa, and ddd don't tell a reader of the code (including you 6 months from now) what these functions actually do. Renaming them as I did in my example above to things like calculateDisplacement(), calculateAcceleration(), etc. would go a long way towards helping this.

Types

Each of your functions returns 2 values, and so you're returning them as an array. It's not clear to me what the meaning of each value is, but if it's something like an x and y of a point, you should make a Point type and use that. If it's a magnitude and angle, then you should make a structure with those members and use that. Another alternative is to return a std::tuple, though when the values are clearly related (like for a Point), it's better to put it into a structure with named members.

Don't reinvent the wheel

You made your own constant PI. You should use M_PI because it's portable and will have enough digits for the precision you need.

\$\endgroup\$
5
  • \$\begingroup\$ Sorry, the returning of two values might be a bit unclear. Most of the functions only return one "legit" value and one rubbish value, PI. If you compile and test the program you will see that only the "legit" values are printed. The reason for needing to return two values is that some of the equations (marked by //RETURNS ARRAY) are quadratic and have 2 solutions. How would i go about collecting the knowns? The program does not know which 3 out of 5 possible knowns to collect. \$\endgroup\$ Oct 4, 2015 at 20:51
  • \$\begingroup\$ From your example switch block: assuming case kUnknownDisplacement is true, how should i implement checking which of the 3 then possible functions to call? (dd, ddd or vf2d) would not that become a very complex switch block? \$\endgroup\$ Oct 4, 2015 at 20:52
  • 1
    \$\begingroup\$ Regarding some functions returning 1 value and some returning 2 - if that's the case, then you should create a structure or class which represents that. For example, you could have an object which tells how many values it contains, and allows you to get each solution. At the very least, it should be a std::vector so callers can determine how many values it contains. Though I think a custom class with appropriately named methods would be more clear. \$\endgroup\$ Oct 4, 2015 at 21:15
  • 2
    \$\begingroup\$ In terms of not wanting to create a huge switch statement - just break it into functions. For example, if the user is looking for kUnknown_Displacement, and there are 3 functions it could call, then make a function which further narrows it down to one of those 3 and call that function from the original switch statement. It's much easier to read, and maintain than a function table which needs to be edited any time you change a function name or whatever. \$\endgroup\$ Oct 4, 2015 at 21:17
  • \$\begingroup\$ I know we are not supposed to say thanks, but I really appreciate this guys! Thank you! \$\endgroup\$ Oct 5, 2015 at 6:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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