1
\$\begingroup\$

This is part of a Particle Swarm Optimization algorithm, a snippet of a Particle class implementation.

/**
 * Particle class constructor
 * @param p - the position of the particle
 * @param v - the velocity of the particle
 */
Particle::Particle(std::array<double, 30> p, std::array<double, 30> v) {
    bestFitness = fitness = rastriginEvaluation(p);
    bestPosition = position = p;
    velocity = v;
}

// setter methods
void Particle::setFitness(double f) { fitness = f; }
void Particle::setBestFitness(double f) { bestFitness = f; }
void Particle::setPosition(std::array<double, 30> p) { position = p; }
void Particle::setBestPosition(std::array<double, 30> p) { bestPosition = p; }
void Particle::setVelocity(std::array<double, 30> v) { velocity = v; }

Notice the constructor directly accesses member attributes yet I have setter methods. Should I be using the setter methods to initialize attributes? Either is fine, I wonder what is more proper.

For example, instead of:

bestFitness = fitness = rastriginEvaluation(p);
bestPosition = position = p;
velocity = v;

I could do:

setFitness(rastriginEvaluation(p));
setBestFitness(rastriginEvaluation(p));
setPosition(p);
setBestPosition(p);
setVelocity(v);

For this example, rastriginEvaluation() is called twice, when the original code only calls it once. It is therefore slower but what is better practice?

I could also wonder about getter methods in the mix:

setFitness(rastriginEvaluation(p));
setBestFitness(getFitness());
setPosition(p);
setBestPosition(getPosition();
setVelocity(v);

If it makes a difference, all attributes are private.

\$\endgroup\$
  • 1
    \$\begingroup\$ Getters and setters are not 'object-oriented', they basically remove the encapsulation idea, and the whole 'class' becomes an empty concept. \$\endgroup\$ – Aganju Nov 15 '19 at 0:33
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Nov 15 '19 at 10:19
4
\$\begingroup\$

As a general rule, if the only thing your getters and setters do is contain a single statement, a return or assignment respectively, there is no need to make the variable private and it should be declared public.

Specifically to your question: Neither of your approaches are considered "best" nor "proper" practice. You should be using an initializer list before the constructor's body:

Note: There is no indication of what order the variables were declared in the class declaration. I've made a guess here, but they are initialized in the order they are declared regardless of what order you put them in the list so if I've got it backwards and fitness is declared first things will silently break at run-time.

Particle::Particle(std::array<double, 30> p, std::array<double, 30> v)
: bestPosition(p)
, position(p)
, velocity(v)
, bestFitness(rastriginEvaluation(p))
, fitness(bestFitness)
{
    /* DO NOTHING */
}
\$\endgroup\$
  • \$\begingroup\$ You can make the breakage non-silent with gcc -Weffc++, which will warn about misleading order of initializers. \$\endgroup\$ – Toby Speight Nov 15 '19 at 10:21
  • \$\begingroup\$ @TobySpeight Similarly, Visual Studio treats this warning as "Off by default": docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/… but can be manually turned back on by compiling with switch /Wall (no, never do this) or treating it as a W4 warning: /w45038 \$\endgroup\$ – Casey Nov 15 '19 at 15:51
0
\$\begingroup\$

@Casey provided a general answer to a clean implementation. Though, decent compilers at optimization steps shouldn't be dependent on that, lest you wrote something really weird in the constructor.

For me the number 30 seems arbitrary. Could it be the case that you frequently use only a portion of the data? Or use a portion depending on situation? Or at times use more? Consider using an image/matrix like allocation method (check out opencv) with columns being the number of Particles and rows being the number of size of data in doubles that each particle allocates. In general if the struct uses 62 doubles then its move/copy operations are somewhat slow. Consider methods of usage that avoids this problem.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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