I'm currently implementing some algorithms into an existing program. Long story short, I created a new class, "Adder". An Adder is a member of another class representing the physical object actually doing the calculus , which calls adder.calc()
with its parameters (merely a list of objects to do the maths on).
To do these maths, I need some parameters, which do not exist outside of the class (but can be set, see below). They're neither config parameters nor members of other classes. These parameters are D1 and D2, distances, and three arrays of fixed size: alpha, beta, delta.
I know some of you are more comfortable reading code than reading text so here you go:
class Adder
{
public:
Adder();
virtual Adder::~Adder();
void set( float d1, float d2 );
void set( float d1, float d2, int alpha[N_MAX], int beta[N_MAX], int delta[N_MAX] );
// Snipped prototypes
float calc( List& ... );
// ...
inline float get_d1() { return d1_ ;};
inline float get_d2() { return d2_ ;};
private:
float d1_;
float d2_;
int alpha_[N_MAX]; // A #define N_MAX is done elsewhere
int beta_[N_MAX];
int delta_[N_MAX];
};
Since this object is used as a member of another class, it is declared in a *.h:
private:
Adder adder_;
By doing that, I couldn't initialize the arrays (alpha/beta/delta) directly in the constructor ( int T[3] = { 1, 2, 3 };
), without having to iterate throughout the three arrays. I thought of putting them in static const, but I don't think that's the proper way of solving such problems.
My second guess was to use the constructor to initialize the arrays:
Adder::Adder()
{
int alpha[N_MAX] = { 0, -60, -120, 180, 120, 60 };
int beta[N_MAX] = { 0, 0, 0, 0, 0, 0 };
int delta[N_MAX] = { 0, 0, 180, 180, 180, 0 };
set( 2.5, 0, alpha, beta, delta );
}
void Adder::set( float d1, float d2 ) {
if (d1 > 0)
d1_ = d1;
if (d2 > 0)
d2_ = d2;
}
void Adder::set( float d1, float d2, int alpha[N_MAX], int beta[N_MAX], int delta[N_MAX] ) {
set( 开发者_StackOverflow中文版d1, d2 );
for (int i = 0; i < N_MAX; ++i) {
alpha_[i] = alpha[i];
beta_[i] = beta[i];
delta_[i] = delta[i];
}
}
Would it be better to use another function (init()
) which would initialize the arrays? Or is there a better way of doing that?
Did you see some mistakes or bad practice along the way?
You have chosen a very wide subject, so here is a broader answer.
- Be aware of your surroundings
Too often I have seen code doing the same thing as elsewhere in the codebase. Make sure that the problem you are trying to solve has not already been solved by your team-mates or predecessors.
- Try not to reinvent the wheel
An extension of my previous point.
While everyone should probably write a linked-list or a string class as an exercise, there is no need to write one for production code. You will have access to MFC, OWL, STL, Boost, whatever. If the wheel has already been invented, use it and get on with coding a solution to the real problem!
- Think about how you are going to test your code
Test Driven Development (TDD) is one way (but not the only way) to ensure that your code is both testable and tested. If you think about testing your code from the beginning, it will be very easy to test. Then test it!
- Write SOLID code
The Wikipedia page explains this far better than I could.
- Ensure your code is readable
Meaningful identifiers are just the beginning. Unnecessary comments can also detract from readability as can long functions, functions with long argument lists (such as your second setter), etcetera. If you have coding standards, stick to them.
- Use
const
more
My major gripe with C++ is that things aren't const by default! In your example, your getters should be declared const
and your setters should have their arguments passed in as const
(and const
-reference for the arrays).
- Ensure your code is maintainable
Unit tests (as mentioned above) will ensure that the next person to change your code doesn't break your implementation.
- Ensure your library is usable
If you follow Principle of least astonishment and document your library with unit-tests, programmers using it will have fewer issues.
- Refactor mercilessly
An extension of the previous point. Do everything you can to reduce code duplication. Today I witnessed a bug-fix that had to be executed in fifteen separate places (and was only executed in thirteen of these).
When creating an object I would advise to always give the user an complete object, with all member properly initialized. An Init method fails in doing that, making room for a common error, failing to calling the initializing function in a two phase initialization object.
To prevent this either make your constructor private and use a builder function or a factory which in turn has access to the init method, or make init private and use it in the constructor. The last advice is generally the same as doing the initialization in the constructor, but it allows several constructors to use the same initializing action.
Okay. I would:
- Set the arrays to a known state by
using
memset
to clear all values to 0 (or some other value) within the constructor before they are used. - Change the constructor to allow the passing of array pointers that can be used to initialise the arrays to some other values.
- Retain the Set function that you have to change the values within the arrays and ariables that you're using.
- Don't use virtual functions, unless your design actually requires them.
- In a class that exists primarily to execute one function, that function is canonically named
operator()
. I.e. you'd call yours asadder_(params)
, notadder_.calc(params)
. - If you're initializing three arrays, it's more efficient to use three for-loops. (cache friendly)
精彩评论