4
\$\begingroup\$

This question is a follow-up of a previous question of mine: Genetic Drift Simulator.

Following the advice given in the last question, and some more general improvements, the code has been slimmed down a bit. The major one was implementing Underscore.js, which slimmed down a few of the loops into functions.

The config has identical properties to the last question, however, they've been renamed.

  • numGenerations: the number of generations to iterate over. Can be number input or infinite to go until one population remains.
  • population: an object containing the names of the populations and the amount of each.
  • drawAmount: the amount of population to be surveyed.

var GeneticDriftSimulator = function (user_config) {
    this.config = _.defaults(user_config, {
        numGenerations: '5',
        population: {
            red: 50,
            blue: 50
        },
        drawAmount: 10
    });
};

GeneticDriftSimulator.prototype.run = function () {
    var ratioPopulation = this.config.population;
    var isIndefinite = (this.config.numGenerations == 'infinite');
    for (var i = 0; isIndefinite || i < this.config.numGenerations; i++) {
        if (Object.keys(ratioPopulation).length == 1) {
            console.log('Generation ' + i + ': One trait left: ' + Object.keys(ratioPopulation)[0]);
            break;
        }
        var population = [];
        for (var populationName in ratioPopulation) {
            for (var k = 0; k < ratioPopulation[populationName]; k++) {
                population.push(populationName);
            }
        }
        ratioPopulation = this.ProcessGeneration(population, config.drawAmount);

        var print_result = "Generation " + i + ": ";
        Object.keys(ratioPopulation).forEach(function (key) {
            print_result += key + ':' + ratioPopulation[key] + ' '
        });
        console.log(print_result);
    }
};

GeneticDriftSimulator.prototype.ProcessGeneration = function (population, drawAmount) {
    var temp_drawn = [];
    if (config.drawAmount >= population.length) {
        throw 'SimulationError: The DrawAmount is more than the Population.';
    }
    if (population.length % drawAmount != 0) {
        throw 'SimulationError: The DrawAmount cannot be evenly divided by Population';
    }

    temp_drawn = _.sample(population, drawAmount);
    var multiplier = population.length / drawAmount;
    return this.RatioCount(multiplier, temp_drawn);
};

GeneticDriftSimulator.prototype.RatioCount = function (multiplier, temp_drawn) {
    var final = {};
    _.each(temp_drawn, function (population) {
        for (var i = 0; i < multiplier; i++) {
            if (!(population in final)) {
                final[population] = 0;
            }
            final[population]++;
        }
    });
    return final;
}

with usage like the following:

var config = {
    numGenerations: 'infinite',
    population: {
        cats: 3e4,
        dogs: 7e4,
    },
    drawAmount: 5e4
};
var simulation = new GeneticDriftSimulator(config);
simulation.run();
\$\endgroup\$
4
\$\begingroup\$

Bug

You accidentally use config instead of this.config sometimes:

    ratioPopulation = this.ProcessGeneration(population, config.drawAmount);

    if (config.drawAmount >= population.length) {

You're actually using the global config variable you have for testing:

var config = {
    numGenerations: 'infinite',
    population: {
        cats: 3e4,
        dogs: 7e4,
    },
    drawAmount: 5e4
};

Change its name or inline it to make sure you're not referencing it directly in GeneticDriftSimulator.

Code Review

if (Object.keys(ratioPopulation).length == 1)

You can use Underscore's size:

if (_.size(ratioPopulation) === 1)

Object.keys(ratioPopulation).forEach(function (key) {

Change to:

for(var key in ratioPopulation) {

The second argument of processGeneration is redundant, you can simply use this.config.


var temp_drawn = [];

There's no need to initialize this variable.


var final = {};

I prefer the name result for a variable that's returned by the function.


The run function both controls the looping and does some of the computation of each iteration (This is the reason it has the temporary variable population inside the loop). It's better to move the computation of population to processGeneration so that each function focuses only on one thing: run on the looping and processGeneration on the computation of the next generation.

This also gives you a nice symmetry: processGeneration takes an object and returns an object (both have the same structure), instead of taking an array and returning and object.


Many of your functions do multiple things:

  • run controls the looping, but it also has some formatting logic.
  • processGeneration calls the functions that constitue the steps of the iteration computation, but it also performs some of logic itself: the checks before the call to sample should really be made by sample itself (wrap the checks and the call to _.sample inside your own sample function).
  • ratioCount both counts the population size of each trait and multiplies the counts.

You should extract each functionality into each own function, so each function does only one thing. Then the main code of GeneticDriftSimulator should look like this:

GeneticDriftSimulator.prototype.run = function () {
    var ratioPopulation = this.config.population;
    var isIndefinite = (this.config.numGenerations == 'infinite');
    for (var i = 0; isIndefinite || i < this.config.numGenerations; i++) {
        if (this.numberOfLeftTraits(ratioPopulation) === 1) {
            this.printOneTraitLeft(i, ratioPopulation);
            break;
        }

        ratioPopulation = this.processGeneration(ratioPopulation);
        this.printGeneration(i, ratioPopulation);
    }
};

GeneticDriftSimulator.prototype.processGeneration = function (ratioPopulation) {
    var population = this.buildPopulationArray(ratioPopulation);

    var populationSample = this.sample(population, this.config.drawAmount);
    var traitsCount = this.countTraits(populationSample);

    var multiplier = population.length / this.config.drawAmount;
    return this.multiplyCounts(traitsCount, multiplier);
};

(sample performs the checks and calls _.sample. multiplyCounts multiplies each value of the traitsCount object and can be implemented with _.mapObject. We presumably lost some performance by splitting ratioCount, but performance is probably not critical here and I'm guessing ratioCount wasn't the bottleneck anyway)

Notice how much easier it becomes to understand the logic of run and processGeneration when all other unrelated logic is in other functions.


After you do this, you'll notice the run function passes i and ratioPopulation to all of the methods it calls. This suggests they are a part of the state of the simulation and thus should be instance variables:

GeneticDriftSimulator.prototype.run = function () {
    var isIndefinite = (this.config.numGenerations == 'infinite');

    this.generationCount = 0;
    this.ratioPopulation = this.config.population;

    while(isIndefinite || this.generationCount < this.config.numGenerations) {
        if (this.numberOfLeftTraits() === 1) {
            this.printOneTraitLeft();
            break;
        }

        this.processGeneration();
        this.printGeneration();

        this.generationCount++;
    }
};

(The methods called by run no longer accept arguments but use this.generationCount and this.ratioPopulation instead. processGeneration was changed to modify this.ratioPopulation instead of returning a value. I recommend that buildPopulationArray still gets ratioPopulation as an argument because it's a helper function for an intermediate step and it's nice when your helper functions are independent of state.)

\$\endgroup\$
6
\$\begingroup\$

A few things to point out in retrospect:

  • There's inconsistent quotation marks everywhere
  • Lines like the following are confusing and hard to read: print_result += key + ':' + ratioPopulation[key] + ' '
  • These (throw) should be throw new Error as it includes a stack trace.
  • Don't use snake case (temp_drawn) use camel case (tempDrawn)
  • Be careful about using truthy value checks like (population.length % drawAmount != 0) as it can cause issues relating to booleans, for example false == 0 will return true, but false === 0 will return false. See Truthy and Falsy: When All is Not Equal in JavaScript for more information and examples.
  • run doesn't match the case of the other functions, it should be Run.
  • k < ratioPopulation[populationName];: declare the length as a variable within the for loop as it gets rid of the constant length checking which can strain when you have a really large array.
  • You should consider using an array here instead var final = {}; as it may reduce logic on the loops.
\$\endgroup\$
2
\$\begingroup\$

Your method names should begin with a small letter e.g processGeneration instead of ProcessGeneration.

You could make the print_result creation arguably a bit more readable as follows:

var format = function (trait) {
    return trait + ":" + ratioPopulation[trait];
};

var traits = Object.keys(ratioPopulation);

var print_result = "Generation " + i + ": " + traits.map(format).join(" ");

I would be tempted to have a method specifically for printing the result. If you changed ProcessGeneration to not return a value, but just set this.ratioPopulation = this.RatioCount(multiplier, temp_drawn); you could try

GeneticDriftSimulator.prototype.printResult = function (generationIndex) {
    var ratioPopulation = this.ratioPopulation;

    var format = function (trait) {
        return trait + ":" + ratioPopulation[trait];
    };

    var traits = Object.keys(ratioPopulation);
    var result = "Generation " + generationIndex + ": " + traits.map(format).join(" ");
    console.log(result);
}

and then you could have

  this.processGeneration(population, config.drawAmount);
  this.printResult(i);

Actually, where does that config come from in the this.processGeneration() call? It doesn't seem to be defined anywhere. Do you need to pass it at all? Why not just access this.config.drawAmount in processGeneration?

You should get into to the habit of using === and !== instead of == and !=. You can read about the difference here.

You can change

if (Object.keys(ratioPopulation).length == 1) {
        console.log('Generation ' + i + ': One trait left: ' + Object.keys(ratioPopulation)[0]);
        break;
    }

to

 var traits = Object.keys(ratioPopulation);
 if (traits.length === 1) {
        console.log('Generation ' + i + ': One trait left: ' + traits[0]);
        break;
    }

so that you only call Object.keys once.

You could move

    var population = [];
    for (var populationName in ratioPopulation) {
        for (var k = 0; k < ratioPopulation[populationName]; k++) {
            population.push(populationName);
        }
    }
    ratioPopulation = this.ProcessGeneration(population, config.drawAmount);   

into its own method.

  //In the constructor 
  this.ratioPopulation = this.config.population;

  // new method
  GeneticDriftSimulator.prototype.buildPopulation = function () {
      var population = [];
      var ratioPopulation = this.ratioPopulation;

      for (var populationName in ratioPopulation) {
        for (var k = 0; k < ratioPopulation[populationName]; k++) {
            population.push(populationName);
        }
      }

      return population;

  }

and call it in processGeneration.

GeneticDriftSimulator.prototype.processGeneration = function () {
    var population = this.buildPopulation();
    var drawAmount = this.config.drawAmount;

    //etc;
}

the call in run is simplified to

this.processGeneration();
this.printResult(i);
\$\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.