1
\$\begingroup\$

I have written the code for a genetic algorithm and have the crossover function, mutation function, parent selector function and a function to transfer genes. Now I want to put it all together and would like to know if the following code is good programming practice or not.

Species Parents[popSize];
Species Children[popSize];

for(int gen = 0 ; gen < 100 ; gen++){
for(int i = 0; i < popSize ; i ++)
{
    int parentA = chooseParent(Parents);
    int parentB = chooseParent(Parents);
    crossOver(Parents[parentA] , Parent[ParentB] , Children[i]);
    Children[i].mutate();
}
for(int i = 0; i < popSize ; i ++)
{
    transfereGenes(Children[i], Parents[i]);
}
}
\$\endgroup\$
  • \$\begingroup\$ Using != for the end conditions, and pre-increment instead of post-increment is perhaps more idiomatic c++. Maybe don't hard code the 100 limit, and use a const variable. Perhaps use std::array instead of C-style arrays. Besides that there's not much here to actually comment on... it's just a few for loops. \$\endgroup\$ – user673679 Oct 26 '15 at 22:15
  • 2
    \$\begingroup\$ You should probably include crossOver and transfereGenes \$\endgroup\$ – Quill Oct 27 '15 at 0:10
  • \$\begingroup\$ @user673679 you should post that as an answer. \$\endgroup\$ – RubberDuck Nov 1 '15 at 13:13
1
\$\begingroup\$

Style

Species Parents[popSize];
Species Children[popSize];

It's more common for variables to start with lower case letters.

Species parents[popSize];
Species children[popSize];

That way you can easily tell that Species is a type while parents is a variable. As it is, I have no way to tell why Parents is capitalized and parentA, etc. are not.

for(int gen = 0 ; gen < 100 ; gen++){
for(int i = 0; i < popSize ; i ++)
{

You're mixing two styles here. Both are valid, but it's generally preferred to pick one and use it consistently throughout your code. More of the code is using { on a separate line, so it may be best to standardize on that. Personally, I prefer it the other way, but as I said, both are valid.

for (int gen = 0; gen < 100; ++gen)
{
    for (int i = 0; i < popSize; ++i)
    {

Also, you indent inside the inner loops but not the outer. It's unclear if this was intentional or a copy/paste error. Note that if you indented the outer lines extra to make them appear in a code block, that it's often better to select the whole code and use Ctrl+K to indent them. That will increase the indent of the entire code, not just the outermost lines.

I also changed the spacing to a more common form.

Preincrement may be defined more efficiently than postincrement. It's a tiny difference, but some people prefer always using preincrement when either would work functionally. Note that a compiler may compile out the difference entirely anyway.

< versus !=

A comment argued that it is more common to use != rather than <, but I think that they missed the point. C++ favors using iterators in this situation. Since many iterators don't have a meaningful < operator, they use something that looks sort of like

for (auto i = iterator.begin(); i != iterator.end(); iterator++)

However, that doesn't apply if your for loop is over an int. Because int values do have meaningful <, these are more robust. There's no need to maintain consistency at the price of robustness. Consider

for (int i = 0; i != 5; i +=2)

This code will crash with an integer overflow (or loop forever) because i will never equal 5. And this code might otherwise be a sensible replacement for

for (int i = 0; i < 5; ++i)
{
    if (i % 2 == 0)

Note that if you were using std::array or std::vector and C++11, you could use the new range-based for loop.

for (auto child : Children)

Then you don't have to manually manage your iteration at all.

Missing code

You use Parents[i] but you never set it to anything. Perhaps chooseParent or crossOver does that for you, but it's not clear. It's also not clear why you transfereGenes (should that be transfer rather than transfere?) with Children[i] and Parents[i]. Why are those two related now when they weren't earlier?

This is probably also a violation of the Single Responsibility Principle. Initialization of the Parents and Children arrays presumably occurs somewhere. But since it doesn't happen here or in a method called something like initialization, it's not obvious where. Nor is it obviously correct. We're essentially relying on the function implementations being correct. But I don't even know what they are supposed to do. I can guess a bit from the names, but this code won't work at all unless one of them handles initialization.

Is your code correct? Without knowing what the code it calls does, I have no way of knowing. If you want more of a review, you may want to post a new question with more context. Preferably with runnable code.

    crossOver(Parents[parentA] , Parent[ParentB] , Children[i]);

Is this a typo? Should it be

    crossOver(Parents[parentA], Parents[ParentB], Children[i]);

I can't be sure. Maybe the missing code has a Parent array as well. And of course if not, a runnable version would find that trivially with a compile.

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