patterncppMinor
Kinematic Equations Calculator
Viewed 0 times
calculatorkinematicequations
Problem
The program allows user to enter values for 3 of the following
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
#include
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 = 0 && nIndex = 0 && nIndex = 0 && nIndex = 0 && nIndex = 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);
- 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
#include
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 = 0 && nIndex = 0 && nIndex = 0 && nIndex = 0 && nIndex = 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);
Solution
This is pretty clever work for someone self taught! In my opinion, it's more work than is necessary, though. Your
Simplify
I think you can do away with the
Then, after collecting your knowns, just do something simple like:
Naming
The other thing I think you could improve is your naming. Function names like
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
Don't reinvent the wheel
You made your own constant
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.Code Snippets
enum {
kUnknown_Displacement,
kUnknown_InitialVelocity,
kUnknown_FinalVelocity,
kUnknown_Acceleration,
kUnknown_Time
};switch(unknown) {
case kUnknown_Displacement:
calculateDisplacement(initialVelocity, Acceleration, time); // or whatever variables you need
break;
case kUnknown_InitialVelocity:
// etc...
break;
};Context
StackExchange Code Review Q#106547, answer score: 8
Revisions (0)
No revisions yet.