Implementing a genetic algorithm to solve knapsack

I am trying to develop a genetic algorithm to solve knapsack problem(0-1). I am new to algorithm and programming as well. Here is my code and it works but I would like to know your suggestions of how to improve it.

Knapsack.h

#ifndef KNAPSACK_H
#define KNAPSACK_H
#include 
#include 

class knapsack
{

public:
    struct population
    {
        int fitness;
        int weight;
        int pop[5];
    };

public:

    knapsack();

    void initPopulation(std::vector &pop, std::vector &initBuffer);
    void calFitness(std::vector &pop, int userInput[][5]);

    void sortByFitness(std::vector &sort);

    void mate(std::vector &mate1, std::vector &mate2);
    void copyNewGeneration(std::vector ©From, std::vector ©To);
    void mutate(std::vector &mutate, std::vector &buffer);

    void swap(std::vector &pop, std::vector &buffer);

    int randGenerate(int start, int end);

    void print(std::vector &pop);

    void solve(int userInput[][5]);

private:
    int popSize;
    int maxB;
    int maxW;
    int bagWeight;

public:
    std::default_random_engine dRandom;
};

#endif

implementation.cpp

```
#include
#include
#include
#include "Knapsack.h"

typedef std::vector chromosomes;

knapsack::knapsack():dRandom(), popSize(10), maxB(0), maxW(0), bagWeight(20){}

void knapsack::initPopulation( chromosomes &initPop, chromosomes &initBuffer)
{
knapsack::population p;

int gene = 0;

for(int i = 0; i genenNum(start, end - 1);

return genenNum(dRandom);
}
void knapsack::calFitness( chromosomes &pop, int userInput[][5])
{
int fit = 0;
int weight = 0;
for(int i = 0; i bagWeight) pop[i].fitness = 0;
}
else
{
weight += 0;
fit += 0;
pop[i].weight = weight;
pop[i].fitness = fit;
}

}
weight = 0;
fit = 0;
}

}

void knapsack::copyNewGeneration( chromosomes ©From, chromosomes

Naming of C++ types:

User defined C++ types are usually named using CamelCase, with the first letter
upper case. Lowe-case names are commonly used by the standard library.
I suggest you rename your types knapsack and population to Knapsack and Population instead.

Make internal stuff private:

I don't see a reason for the random generator dRandom to be public.
Actually, the only method being called by main() is knapsack::solve().
If no other method is meant to be called outside the class, put those in the
private section (except for the constructor and destructor, of course).

Some mix-up with typedef:

In implementation.cpp you have a chromosomes typedef that you consistently
use. However, in Knapsack.h you are using std::vector instead.
Why this mix-up? Place the typedef in the class header, right below struct population and use it everywhere.

Re-implementing swap():

Your class implements its own swap() method. Which is redundant
with std::swap(), the standard swap function. Use the standard one
instead. Chances are that it is much more optimized and efficient than yours.

But wait, there is more! Your swap function is actually incorrect.

void knapsack::swap( chromosomes &pop,  chromosomes &buffer)
{
     chromosomes *temp = &pop; // 1. temp points to pop
     pop = buffer;             // 2. pop is now = buffer
     buffer = *temp;           // 3. buffer is now = whatever temp points at, which is = pop
}

This function is basically a one-way copy. First (1), you save the address, not the value, of pop in the temp pointer. So *temp is the same as pop.

Second (2) you overwrite pop with the contents of buffer. Now pop is equal to buffer.

Third (3) you overwrite buffer with the contents of whatever temp points to. But temp points to pop and pop is already equal to buffer, so you are basically
writing buffer over pop and then writing it back to buffer. So buffer is never actually changed.

Const correctness:

knapsack::print() is only reading from its input parameter. When a function/method
only reads from a param, that param should be made const. This is even more important
when the parameter is passed by reference, as it is the case here. This is called const correctness.

void knapsack::print(const chromosomes &pop) { ... }

srand() is unnecessary in this context:

srand(unsigned(time(NULL)));

Is useless here. std::default_random_engine cannot be seeded that way.
If you want to seed it by the current time, see an example of how to do it here.

5s?

You've used the constant 5 in several places. Is this just a coincidence
or does 5 actually mean something to your program?

for(int j = 0; j < 5; j++)
...
int index1 = randGenerate(0, 5);
int index2 = randGenerate(0, 5);
...
void knapsack::solve(int userInput[][5])

Give that constant a meaningful name (a class scoped static const int)
and replace those raw 5s with it.

Blank lines and some indentation problems:

Your code has a few lines with bad indenting, like these:

if(pop[0].fitness > maxB)
   {
       maxB = pop[0].fitness;
   }

And a few excessive blank lines in some places, like here:

void knapsack::solve(int userInput[][5])
{

     chromosomes initPop, initBuffer, *pop, *buffer;

    initPopulation(initPop, initBuffer);

Fix this to make your code neater and easier to read.

StackExchange Code Review Q#62990, answer score: 6